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

Optimized replacement for _.memoize() #330

Closed
wants to merge 2 commits into from

Conversation

addyosmani
Copy link

I took the work we've been doing on an optimal memoization routine in [1][2] and integrated it with underscore to offer an implementation which is both:

  1. Significantly more performant than the current version
  2. Supports more cases likely to be used in production
  3. Passes the test suite tests for the old memoizer

The implementation in [2] comes with it's own tests in that repo, however what you're probably more interested in looking at are the performance figures. Here's a jsPerf test (others are available) which demonstrates the improvements in performance from the existing implementation when compared to this one:

http://jsperf.com/comparison-of-memoization-implementations/12

The work here has gone through multiple rounds of further optimization by @mathias, @DmitryBaranovskiy, @JamieMason and others and is currently the fastest (thus benchmarked) memoizer we're currently aware of (as you'll see in the tests, we also compare against other implementations by John Hann, Thomas Fuchs etc).

The only real difference between the current approach and mine/ours is that the optional hash function is dropped. I've thoroughly reviewed use of memoization in JS projects out there at the moment and I haven't seen a great number of people actually rely on such optional functions, they just want the main memoizer itself.

It would be great if we could get this in Underscore, but definitely open to thoughts on whether it's worth including.

Notes:

The new memoizer has dependencies on JSON support being natively available [4]. I think this is okay, but we're happy to spend more time on support issues if needed. I'm currently looking at putting together a compact stringify method that could be substituted to resolve any compat issues for a future update.

You may also ask whether the perf test is a fair comparison of access/usage times. We've also separately benchmarked these in the earlier stages of development with the newer implementation outperforming the others there too. Happy to create another test to demonstrate if needed.

Thanks!

Refs:

[1] http://addyosmani.com/blog/faster-javascript-memoization/
[2] https://github.com/addyosmani/memoize.js
[3] http://jsperf.com/comparison-of-memoization-implementations/12
[4] http://caniuse.com/json (browser support levels for JSON)

@jashkenas
Copy link
Owner

Thanks for the effort, but I'm afraid that relying on the JSON object isn't going to cut it. (And I'm not aware of any serious problems with hasOwnProperty in IE6 -- it's ES3).

The optional hasher function is necessary -- especially without JSON object support -- because sometimes a blind stringification of the arguments won't give you a proper value on which to memoize ... or sometimes, when dealing with side effects, there's a unique argument you can memoize on, and the rest are irrelevant to the returned result.

I'm very curious as to where the performance difference between different versions is coming from -- it seems a bit strange. Is it the "use strict"? The JSON.stringify vs. implicit Array.join?

@addyosmani
Copy link
Author

We didn't start off with the JSON object as a hard dependency, but using stringification on the arguments did prove faster over some of the alternatives. Agreed that this is at cost of browsers without native JSON support. To be honest, I think some further micro-perf testing may be needed to qualify whether this is where the bulk of our improvements weighs.

More than open to moving back to a two argument signature if we can maintain most of the the performance improvements (and of course, we'll see how well backing JSON use out affects the comparison).

@jashkenas
Copy link
Owner

Closing for the time being, since this particular patch can't be merged. Update this pull request when you have a new version figured out, and we'll reopen this ticket.

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

Successfully merging this pull request may close these issues.

None yet

2 participants