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

[markjm]

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]

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)

[markjm]

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

[markjm]

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]

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

[sokra]

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]

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]

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]

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]

See the splitting approach implemented in webpack/watchpack#205

[markjm]

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

cc @sokra

[markjm]

@sokra PR updated with watchpack@2.3.0

[sokra]

Thanks