Skip to content

Usage of reduce spread #46

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

Closed
schester44 opened this issue Jun 25, 2019 · 3 comments
Closed

Usage of reduce spread #46

schester44 opened this issue Jun 25, 2019 · 3 comments

Comments

@schester44
Copy link

schester44 commented Jun 25, 2019

There could be noticeable perf improvements if we were to stop using the reduce spread pattern in the diffing methods.

Some benchmarks from changing the diff method to use basic for loops:

original x 377,694 ops/sec ±0.62% (86 runs sampled)
new x 788,699 ops/sec ±0.55% (91 runs sampled)

Reference: https://www.richsnapp.com/blog/2019/06-09-reduce-spread-anti-pattern

I have most of the work done in a fork. Would you be open to a PR?

@anko
Copy link
Contributor

anko commented Jun 29, 2019

I assume you mean schester44@7caf8a1. I like it. I think you should PR!

Comments so far:

  • The pattern of getting Object.keys(l) and then iterating it comes up a lot. I think it could be extracted to a function to clarify. I imagine it should still perform well?

  • How did you benchmark? A benchmark script would be handy for other optimisation also, and for checking we're on the same page, and re-checking when Node updates, and etc.

  • I'd omit the version bump commit (schester44@13f1964) from the PR. It's usually clearest for everyone when the maintainer does those, just prior to publishing that version.

@schester44
Copy link
Author

  • What are your thoughts on the Object.keys function? It being a native one liner, i'm not seeing the benefit but definitely not opposed to going that route if thats something we want to do.

  • Benchmarking was done using this package.

  • version bump was a mistake, i'll fix that.

I'll go ahead and continue with this and create PR soon.

@mattphillips
Copy link
Owner

Closing as I've merged #64 and published in v1.1.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants