perf: refactor various timestamp caches into ES6 Maps #6365
Conversation
This change satisfies webpack#6234 which desires to change the timestamp caches from plain objects to the ES6 Map type to avoid deopts, since v8 expects things to be added to Maps but objects are only fast if properties aren't added dynamically after initial assignment.
test/NodeWatchFileSystem.unittest.js
Outdated
| @@ -68,7 +68,7 @@ describe("NodeWatchFileSystem", function() { | |||
| if(err) throw err; | |||
| filesModified.should.be.eql([fileDirect]); | |||
| dirsModified.should.be.eql([]); | |||
| Object.assign({}, fileTimestamps).should.have.property(fileDirect).have.type("number"); | |||
| fileTimestamps.get(fileDirect).should.have.type("number"); | |||
There was a problem hiding this comment.
Change to:
(typeof fileTimestamps.get(fileDirect)).should.be.eql("number");The previous line would throw a TypeError.
lib/CachePlugin.js
Outdated
| callback(); | ||
| }); | ||
| }, err => { | ||
| if(err) return callback(err); | ||
| Object.keys(fileTs).forEach(key => { | ||
| fileTs[key] += this.FS_ACCURENCY; | ||
| Array.from(fileTs.keys()).forEach(key => { |
There was a problem hiding this comment.
You don't need this:
for(const [file, ts] of fileTs) {
fileTs.set(file, ts + this.FS_ACCURENCY);
}|
It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff). A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future). @probablyup Please check if this is appliable to your PR and if you can add more test cases. Read the test readme for details how to write test cases. |
|
@probablyup Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ooflorent Please review the new changes. |
c74a23f to
3adf072
Compare
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
Thanks |
What kind of change does this PR introduce?
refactoring
Did you add tests for your changes?
modified existing unit tests
If relevant, link to documentation update:
n/a
Summary
This change satisfies #6234 which desires to change the timestamp caches from plain objects to the ES6 Map type to avoid deopts, since v8 expects things to be added to Maps but objects are only fast if properties aren't added dynamically after initial assignment.
Does this PR introduce a breaking change?
I think yes if anything is downstream is using
compiler.fileTimestampsorcompiler.contextTimestamps.