-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Default memoize hasher returns invalid key for multiple arguments #575
Comments
I'm testing performance of an alternative hasher that would just do function(x) {
if (arguments.length <= 1) {
return x;
} else {
return Array.prototype.slice.call(arguments);
}
}; with hope that the check for arguments.length will limit the exposure to the slow(?) Array.prototype.slice.call invocation. |
https://jsperf.com/async-js-memoize-hashers doing #575 (comment) seems to be 5% slower on chrome. I think the correctness justifies the performance hit |
When using things that are easily stringified, using all the args won't be that much slower. The real performance hits when you have to stringify large object arguments. It is also extremely common in other memoize implementations (such as Underscore/Lodash) to only use the first argument by default. That's what the custom hasher is for. |
There is also tons of existing code that relies on async.memoize only using the first arg. This would cause a huge regression. It would have to be a 1.0.0 type of change. |
@lxe We just ran into this same problem. One issue with your solution is that effectively what it's doing is a What about if we just did a As proof, I altered @lxe JSperf to show an example of what I mean https://jsperf.com/async-js-memoize-hashers/2. I also changed some of the loops because they previously had some minor async errors. And really, is the performance of the hasher really that important? The entire intent of async.memoize is to save the cost of an expensive To @aearly can we really expect anyone out there to have written code such that they want |
Fixed the docs through #631 |
Currently, the default memoize hasher takes only the first argument to create the memo key:
https://github.com/caolan/async/blob/master/lib/async.js#L1000-L1002
This will cause memoized functions that take multiple arguments to call back with invalid results:
The text was updated successfully, but these errors were encountered: