Improves _.memoize performance
#2461
Conversation
|
Here the perf results: Here the code with the perf benchmark: |
| var memoize = function(key) { | ||
| var cache = memoize.cache; | ||
| var address = '' + (hasher ? hasher.apply(this, arguments) : key); | ||
| if (!_.has(cache, address)) cache[address] = func.apply(this, arguments); | ||
| if (typeof cache[address] === 'undefined') cache[address] = func.apply(this, arguments); |
There was a problem hiding this comment.
Wouldn't this need to be forked a bit more for enviros without Object.create which still need the own property checks.
|
This is the kind of thing a generic |
Yes! \o/ Lodash does this with Related to #1862. |
|
Sure, perhaps it's time to add it. |
|
I totally agree a generic @jdalton Already updated the code to properly check for keys in objects with and without prototype. |
|
With my last change, still getting something around 3x more performance. |
| if (cache.constructor === Object) { | ||
| return _.has(cache, address); | ||
| } else { | ||
| return (typeof cache[address] !== 'undefined'); |
There was a problem hiding this comment.
The issue with this is undefined is a valid return value that should be memoized. We'll need to use _.has in every case.
There was a problem hiding this comment.
Makes sense!
Should I add a test for this case?
Right now everything is passing.
|
Just added the test case to |
|
Did a rebase as well. |
| var memoize = function(key) { | ||
| var cache = memoize.cache; | ||
| var address = '' + (hasher ? hasher.apply(this, arguments) : key); | ||
| if (!_.has(cache, address)) cache[address] = func.apply(this, arguments); | ||
| if (!(_.has(cache, address))) cache[address] = func.apply(this, arguments); | ||
| return cache[address]; |
There was a problem hiding this comment.
Are you still seeing a perf win?
If you aren't don't close the PR. I'll walk you through some things to improve it.
There was a problem hiding this comment.
No perf win using _.has =/
Can you spot something we could improve over here?
There was a problem hiding this comment.
Hey @caiogondim!
I created a screencast to dig into MapCache a bit more.
You can follow along at lodash._mapcache/index.js.
Update:
Note: The Hash and HASH_UNDEFINED bits are something I learned from Ember.
Creating an object without prototype proves to be faster than a literal object, since we don't have to go through the prototype chain to check if a key already exists. With a memoized recursive fibonacci implementation, we could do on a MacBook 13 Retina Late 2013 ~9.6M ops/sec, while with the improved implementation we could get ~26M ops/sec. Almost a 3x improvement.
Now checking properly the existence of keys in objects with and without prototype.
|
Just resolved the conflict. |
|
Closing this as it seems the performance gain has been lost in the process of making this work correctly, and as a performance enhancement based on #1862 would seem preferable. |
Creating an object without prototype proves to be faster than a literal
object, since we don't have to go through the prototype chain to check
if a key already exists.
With a memoized recursive fibonacci implementation, we could do on a
MacBook 13 Retina Late 2013 ~9.6M ops/sec, while with the improved
implementation we could get ~26M ops/sec. Almost a 3x improvement.