Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change .reduce to for...of loop to improve performance #203

Closed
fabian-hiller opened this issue Oct 7, 2023 · 2 comments
Closed

Change .reduce to for...of loop to improve performance #203

fabian-hiller opened this issue Oct 7, 2023 · 2 comments
Assignees
Labels
enhancement New feature or request performance Its performance related

Comments

@fabian-hiller
Copy link
Owner

In several places, for example in partial, we currently still use .reduce. For performance reasons, it may make sense to change this to a for...of loop. Since we use for...of in several other places in the library, this change should not affect the bundle size due to compression and should make the code more consistent.

Idea for this change comes from @kurtextrem in #202

@fabian-hiller fabian-hiller added enhancement New feature or request performance Its performance related labels Oct 7, 2023
@fabian-hiller fabian-hiller self-assigned this Oct 7, 2023
@lo1tuma
Copy link
Contributor

lo1tuma commented Oct 9, 2023

Is the usage of .reduce really a bottleneck in applications that use valibot? I usually recommend to avoid micro-optimizations, especially when it is about language features, because the performance could change with every new release of the engine (e.g. v8).
So I think a data-driven approach would be better. E.g. having a benchmark suite, which tests the real-world usage of valibot. This could then be used to identify the performance bottlenecks. Optimizing something that is not the bottleneck is usually waste or in the worst case it increases the complexity or maintainability of the codebase without any benefits.

@fabian-hiller
Copy link
Owner Author

Thank you for your feedback! I will take it into consideration. We plan to add performance benchmarks to our CI in #100.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Its performance related
Projects
None yet
Development

No branches or pull requests

2 participants