Package on main thread for small rebuilds #8491
Conversation
|
|
||
| // Create shared reference in WorkerFarm if we need to change multiple bundles and if we | ||
| // can't skip the subrequests for all the bundles | ||
| if ( |
There was a problem hiding this comment.
Not sure if I got this logic quite right.. are there any other cases to look for here?
There was a problem hiding this comment.
I'm not sure either, but I'm confused that the comment says "can't skip all the bundles = at least 1 bundle can't be skipped" but then the if condition checks "there are at least 2 bundles which can't be skipped".
Benchmark ResultsKitchen Sink ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. React HackerNews ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. AtlasKit Editor ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
packages/core/types/index.js
Outdated
| @@ -1754,6 +1755,7 @@ export type PackagingProgressEvent = {| | |||
| +type: 'buildProgress', | |||
| +phase: 'packaging', | |||
| +bundle: NamedBundle, | |||
| +bundleGraph?: InternalBundleGraph, | |||
There was a problem hiding this comment.
- Do we really want to add the bundle graph to this event? (This is the public plugin API)
- If yes, then it certainly needs to be the public bundle graph type
There was a problem hiding this comment.
Hmm I think the main reason why I added this was because the ReporterRunner gets the BundleGraph from the WorkerFarm, which wouldn't exist in this case since we don't serialize the BundleGraph, so I sent the BundleGraph through the packaging/optimizing events.
parcel/packages/core/core/src/ReporterRunner.js
Lines 67 to 73 in cf5b3ff
The invariant here also looks like it uses the private BundleGraph type?
There was a problem hiding this comment.
Maybe we should make an internal ReporterEvent type that has this instead?
thebriando
changed the title
| export type BundleGraphEvent = {| | ||
| +type: 'bundleGraph', | ||
| +bundleGraph: BundleGraph<NamedBundle>, | ||
| |}; |
There was a problem hiding this comment.
What Devon meant in #8491 (comment) is that there could an internal event between PackagerRunner and ReporterRunner (which the reporter runner then just uses for setting the bundle graph but doesn't send it out to the reporter plugins).
But I think it's strange to have to split it up into two events in the first place.
Why don't you use the same idea of passing either the bundlegraph ref or the bundlegraph object itself in the packaging event? The PackagerRunnner is already running on the main thread, so calling report(something) wouldn't perform any serialization anyway.
|
I've refactored the WorkerFarm in a way that should make this a bit simpler to implement: #8589. |
↪️ Pull Request
This change prevents
WriteBundlesRequestfrom serializing across all workers with small rebuilds which should improve save/warm build times when small changes that only impact a single bundle are made to the project.Tested saving a file with a small change in a large project
Before:
After:
✔️ PR Todo