fix(index.js): avoid creating a new object in reduce loop #14
Conversation
Partially reverts e583186
| ...acc, | ||
| [usedFilepath]: usedFilepath, | ||
| }), {}); | ||
| const fileDepsBy = compilation.fileDependencies.reduce((acc, usedFilepath) => { |
There was a problem hiding this comment.
const fileDepsBy = new Set(compilation.fileDependencies) should theoretically be much faster. I'd be very interested to see the results!
There was a problem hiding this comment.
Yeah, even this approach is now 1ms vs. ~2s
There was a problem hiding this comment.
the mutating reduce is 1ms, or the Set is 1ms?
There was a problem hiding this comment.
Mutating reduce. O(n) mutation isn't terrible compared to O(n^2) Object.assign! Though you might get sub milliseconds on native Set usage... haven't tried it.
There was a problem hiding this comment.
It might be worth using Set if only to avoid the eslint override :-) but it'd be great to try it and determine if it's significantly better.
There was a problem hiding this comment.
Where is an example of this new Set method you refer to?
| @@ -26,6 +26,8 @@ | |||
| // Variables | |||
| "no-use-before-define": [2, "nofunc"], | |||
| "no-unused-vars": [2, {"args": "none"}], | |||
| // Performance Considerations | |||
| "no-param-reassign": [0], | |||
There was a problem hiding this comment.
Although I'd prefer disable this using single line comment
| [usedFilepath]: usedFilepath, | ||
| }), {}); | ||
| const fileDepsBy = compilation.fileDependencies.reduce((acc, usedFilepath) => { | ||
| acc[usedFilepath] = usedFilepath; |
There was a problem hiding this comment.
Maybe switching to _.set(acc, usedFilepath, usedFilepath) could just skipping the eslint error
There was a problem hiding this comment.
I think @ljharb's suggestion of using Set is right move long term. Just wanted fast rebuilds back first so thanks for merging!
|
Released v3.0.2 |
As outlined here, avoiding parameter reassign by creating a new object occasionally can cause performance issues. This happens to be one of those cases!
This partially reverts e583186
Please see below for performance implications for an 1800 module webpack run!