Map optimization : each boiler #1708
Conversation
megawac
changed the title
megawac
changed the title
|
I'm against changes of this nature, because I value the readability of Underscore's source code above performance, provided the performance is acceptable (which it is). Underscore's source code was once a good example of how to write readable JavaScript code using Underscore functions. That said, since we're already trading readability for performance gains in many places, perhaps there's no reason to stop. |
|
❤️ |
|
The technique @megawac applies to My implementation goes a different route opting for |
| } | ||
| var results = Array(length); | ||
| for (var index = 0; index < length; index++) { | ||
| currentKey = keys ? keys[index] : index; |
There was a problem hiding this comment.
You shouldn't test keys in the loop. For optimum performance, we should pull the keys test out and have two loops: one that accesses keys[index] and one that uses index directly.
There was a problem hiding this comment.
You've brought this up with other parts of Underscore in the past & I believe at the time it was proven that the check was negligible. We can check again to validate but avoiding the second loop is a big win for avoiding code dup.
There was a problem hiding this comment.
Oh, I didn't realise. I'm not super opinionated either way. We'll stick with the simpler style.
|
I like this. If anything, I'd say this version is more readable. |
| _.map = _.collect = function(obj, iterator, context) { | ||
| var results = []; | ||
| if (obj == null) return results; | ||
| _.map = _.collect = function(obj, iterator, context) { |
There was a problem hiding this comment.
It looks as though there's an extra space at the beginning of this line.
Map optimization : each boiler
|
Cool, looks like we're all in agreement here. @megawac: Want to open a PR to apply this pattern everywhere? |
|
Sure, which functions do you want it for? I'm thinking it makes sense for filter: http://jsperf.com/pr-1689/6 I figured this pattern would be most beneficial for |
Makes sense because in this case you're doing more work in the array path than the current fast path. |
Was playing with this pattern a couple weeks ago and look forward to hearing thoughts from other contributors as we're sacrificing terseness for speed via with boilerplate. I've applied this pattern to some of the other methods in this branch + some thoughts from @jdalton (see comments at the bottom for the relevant jsperf post changes for other functions ).
Figured I would open the conversation as its one of the most used functions in the lib and optimizations here pay dividends. Relevant jsperf below
http://jsperf.com/pr-1689/9
Also, I'm mixed about this change and can agree if we reject this :)