HasteMap with native maps#6960
Conversation
😲 That's awesome! |
|
Can you update the changelog? A question: is this change observable from the outside (aka a breaking change)? |
|
@SimenB sorry I was meant to talk about that in the description. I'll add it now. |
| ); | ||
| } | ||
|
|
||
| static createEmpty() { |
There was a problem hiding this comment.
just create. It's implied that if you create one without arguments it would be empty.
| const EMPTY_MAP = {}; | ||
|
|
||
| export opaque type SerializableModuleMap = { | ||
| // There isn't an easy way to extract the type of the entries of a Map |
| const moduleMetadata = hasteMap.map[fileMetadata[H.ID]]; | ||
| const fileMetadata = hasteMap.files.get(filePath); | ||
| if (!fileMetadata) { | ||
| // Should never happen |
| const fileMetadata = hasteMap.files.get(filePath); | ||
| if (!fileMetadata) { | ||
| // Should never happen | ||
| throw new Error('File to process was not found in the haste map'); |
There was a problem hiding this comment.
Prefix with jest-haste-map: , end with a .
| files: copy(hasteMap.files), | ||
| map: copy(hasteMap.map), | ||
| mocks: copy(hasteMap.mocks), | ||
| clocks: new Map(hasteMap.clocks), |
There was a problem hiding this comment.
This creates a shallow copy, right?
There was a problem hiding this comment.
Yes, like copy did.
cpojer
left a comment
There was a problem hiding this comment.
This is super solid work @rubennorte. For historical context, when this code was initially written, v8.serialize did not exist and plain objects were much faster than Maps. I'm glad this change now not only improves startup time but also lookup time when resolving modules. Thanks for sending this PR :)
| ### Features | ||
|
|
||
| - `[babel-jest]` Add support for `babel.config.js` added in Babel 7.0.0 ([#6911](https://github.com/facebook/jest/pull/6911)) | ||
| - `[jest-haste-map]` Replaced internal data structures to improve performance ([#6960](https://github.com/facebook/jest/pull/6960)) |
742f16d to
342f76a
Compare
Codecov Report
@@ Coverage Diff @@
## master #6960 +/- ##
==========================================
- Coverage 66.97% 66.89% -0.09%
==========================================
Files 250 250
Lines 10381 10422 +41
Branches 4 3 -1
==========================================
+ Hits 6953 6972 +19
- Misses 3427 3449 +22
Partials 1 1
Continue to review full report at Codecov.
|
| }); | ||
|
|
||
| const pearMatcher = path => /pear/.test(path); | ||
| const createMap = obj => new Map(Object.keys(obj).map(key => [key, obj[key]])); |
There was a problem hiding this comment.
Random question from a stranger here:
Is Object.entries compiled / usable here?
const createMap = obj => new Map(Object.entries(obj));There was a problem hiding this comment.
I used it before but we don't include polyfills and we support Node 6 (which doesn't support it), so I had to remove it. Object.entries was first available in Node 7.
|
@rubennorte In what versions of Node do you see perf wins in? |
|
@jdalton we mainly tested this in Node v9.11.2, but we saw it's also better in v10.10.0. |
|
Very interesting. |
|
"The performance of Map is much better than Object to implement dictionaries when they contain a large amount of entries" — Are there any public data showing when Map should be used over Object? |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This changes the data structure used to store the metadata for files, modules, mocks and duplicated modules in the Haste Map from
Object(without prototypes) toMap.The performance of
Mapis much better thanObjectto implement dictionaries when they contain a large amount of entries, which is usually what we have here. After testing this change in Facebook we've seen a ~20% reduction of the time to run all tests, which is a massive performance improvement for us.This only modifies the internal data structures used by Jest and its end users will not be affected by it (other than performance). The change in the
jest-haste-mappackage is breaking though, as it might affect other packages depending on it (including jest packages, which have been updated here).Test plan
I've updated all tests related to this change (including a change to make the data structures more encapsulated for the rest of packages). I've also tested this in the Facebook infrastructure (including running all tests, running a subset of the tests, watch mode and coverage reporting).