Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: upgrade watchpack & split timestamp by file/dependency & only call collectTimeInfoEntries once per invalid #14728

Merged
merged 6 commits into from Nov 25, 2021

Conversation

@markjm
Copy link
Contributor

@markjm markjm commented Nov 14, 2021

Integrate changes in watchpack to ensure fileTimestamps and contextTimestamps are seperate (smaller) maps. Also, since we fill both maps in one call, we limit calling into the watcher twice to fill out those lists (by sharing the fileMap and directoryMap within all watcher calls).

Also includes the other features, bugfixes, and perf improvements listed here
https://github.com/webpack/watchpack/releases/tag/v2.3.0

What kind of change does this PR introduce?
perf & feature

Did you add tests for your changes?
on watchpack side, yes. For webpack, no, ensure current tests pass, there is no expected change in functionality

Does this PR introduce a breaking change?
no

What needs to be documented once your changes are merged?
yes, with watchpack update
allow functions passed to the ignored option

@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Nov 14, 2021

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

fileTimeInfoEntries ||
(this.pausedWatcher && this.pausedWatcher.getFileTimeInfoEntries());
this.compiler.contextTimestamps =
Copy link
Contributor Author

@markjm markjm Nov 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is where, before, contextTimestamps and fileTimestamps are both created as separate copies of eachother.

@markjm markjm force-pushed the markjm/context-timestamps branch from 0ae8c6d to 358f071 Nov 14, 2021
@markjm markjm marked this pull request as ready for review Nov 14, 2021
@markjm
Copy link
Contributor Author

@markjm markjm commented Nov 14, 2021

I see the benchmark repository has a build to compare 2 branches - I wonder if it is possible to queue with this branch or how to acquire queue permissions to do so.

@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Nov 14, 2021

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra
Copy link
Member

@sokra sokra commented Nov 15, 2021

contextTimestamps and fileTimestamps are nearly identical in their entries.

Better improve the watcher implementation so that they are not identical. Actually the watcher should provide different maps for files and directories, but that was never implemented.

@markjm
Copy link
Contributor Author

@markjm markjm commented Nov 15, 2021

It seems that would also be a valid solution, but I wonder what ways that would be better than the proposed solution here. This approach seems less error-prone (not possible to add an entry to the wrong set, though I guess could be confusing that two variables must share their reference), and perhaps more importantly, is entirely backwards compatible.

Am I missing some reason (perhaps performance of large sets?) why actually splitting these sets would be preferable?

I can take a look at watchpack to incorporate that suggestion instead, but am hoping for some clarity on why the approach is preferable just for my knowledge and learning.

@sokra
Copy link
Member

@sokra sokra commented Nov 15, 2021

is entirely backwards compatible.

It's not, since you are ignoring all context timestamps from the watcher. If a (e. g. custom) watcher provides different values here, they would be ignored now. compiler.contextTimestamps can also be accessed by plugins, and now it's always empty...

@markjm
Copy link
Contributor Author

@markjm markjm commented Nov 15, 2021

is entirely backwards compatible.

It's not, since you are ignoring all context timestamps from the watcher. If a (e. g. custom) watcher provides different values here, they would be ignored now. compiler.contextTimestamps can also be accessed by plugins, and now it's always empty...

compiler.contextTimestamps is never empty, it is set as a reference to the merged map (see L128 of Watching.js).

I did not consider the custom watcher case. I do think there is a strategy to make that backward compatible (by still accepting comtextTimestamp param in _go, but not passing it in built in watchers).

Let me spend some time tomorrow to see if I can grasp the required watchpack changes you suggested as well.

@markjm
Copy link
Contributor Author

@markjm markjm commented Nov 17, 2021

See the splitting approach implemented in webpack/watchpack#205

@markjm markjm changed the title perf: merge backing timestamp map for context and file dependencies perf: correctly split timestamp by file/dependency and only call getTimeInfoEntries once Nov 17, 2021
@markjm markjm force-pushed the markjm/context-timestamps branch from 7451ffb to 72e12ff Nov 17, 2021
@markjm markjm marked this pull request as draft Nov 17, 2021
@markjm
Copy link
Contributor Author

@markjm markjm commented Nov 17, 2021

Marked as draft, as new changes are dependent on review, merge, and release of webpack/watchpack#205

cc @sokra

@markjm markjm force-pushed the markjm/context-timestamps branch from 72e12ff to c2f5724 Nov 17, 2021
@markjm markjm force-pushed the markjm/context-timestamps branch from dbd9e9c to 8867bc1 Nov 24, 2021
@markjm markjm marked this pull request as ready for review Nov 24, 2021
@markjm markjm changed the title perf: correctly split timestamp by file/dependency and only call getTimeInfoEntries once perf: upgrade watchpack & split timestamp by file/dependency Nov 24, 2021
@markjm markjm changed the title perf: upgrade watchpack & split timestamp by file/dependency perf: upgrade watchpack & split timestamp by file/dependency & only call collectTimeInfoEntries once per invalid Nov 24, 2021
@markjm
Copy link
Contributor Author

@markjm markjm commented Nov 25, 2021

@sokra PR updated with watchpack@2.3.0

sokra added 2 commits Nov 25, 2021
use new collectTimeInfoEntries method from watchpack

add more efficient Watcher.getInfo method
sokra
sokra approved these changes Nov 25, 2021
@sokra sokra merged commit 68c4a2a into webpack:main Nov 25, 2021
44 of 47 checks passed
@sokra
Copy link
Member

@sokra sokra commented Nov 25, 2021

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants