Incremental Symbol Propagation #8723
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mischnic
force-pushed
the
symbol-propagation-changedAssets
branch
from
5606d3c
to
f9e47dd
Compare
Benchmark ResultsKitchen Sink ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. React HackerNews ✅
Timings
Cold Bundles
Cached Bundles
AtlasKit Editor ✅
Timings
Cold Bundles
Cached Bundles
Three.js ✅
Timings
Cold BundlesNo bundle changes detected. Cached BundlesNo bundle changes detected. |
mischnic
force-pushed
the
symbol-propagation-changedAssets
branch
from
3f2252e
to
c441fd2
Compare
|
Excited for this. Sort of hard to see the diff of what changed since it moved to a new file, but we can test it out on some apps. Any performance numbers you've seen so far? |
mischnic
commented
Dec 31, 2022
packages/core/utils/src/hash.js
Outdated
Comment on lines
32
to
47
| let testCache: {|[string]: Promise<string>|} = { | ||
| /*:: ...null */ | ||
| }; | ||
| export function hashFile(fs: FileSystem, filePath: string): Promise<string> { | ||
| if (process.env.PARCEL_BUILD_ENV === 'test') { | ||
| // Development builds of these native modules are especially slow and slow to hash. | ||
| if ( | ||
| /parcel-swc\.[^\\/]+\.node$|lightningcss.[^\\/]+.node$/.test(filePath) | ||
| ) { | ||
| let cacheEntry = testCache[filePath]; | ||
| if (cacheEntry) return cacheEntry; | ||
| let v = hashStream(fs.createReadStream(filePath)); | ||
| testCache[filePath] = v; | ||
| return v; | ||
| } | ||
| } |
There was a problem hiding this comment.
(This is the workaround to speedup the integration tests)
|
I've moved the code back (be sure to hide whitespace changes!), will do that again in a separate follow up PR.
Testing with ak-editor (on initial builds, both passes were ~200ms each before):
I haven't tested the case where a large dependency is added. |
mischnic
force-pushed
the
symbol-propagation-changedAssets
branch
from
a8e6e10
to
812bce3
Compare
mischnic
force-pushed
the
symbol-propagation-changedAssets
branch
from
aebbf04
to
792aee9
Compare
mischnic
force-pushed
the
symbol-propagation-changedAssets
branch
from
792aee9
to
4b47954
Compare
mischnic
force-pushed
the
symbol-propagation-changedAssets
branch
from
ff2873f
to
2a72bb5
Compare
mischnic
force-pushed
the
symbol-propagation-changedAssets
branch
from
2a72bb5
to
166a043
Compare
devongovett
approved these changes
Mar 26, 2023
| @@ -747,110 +889,150 @@ export class AssetGraphBuilder { | |||
| } else if (childNode.type === 'dependency') { | |||
| childDirty = childNode.usedSymbolsDownDirty; | |||
| } | |||
| if (!visited.has(child) || childDirty) { | |||
| if (childDirty) { | |||
There was a problem hiding this comment.
Does this mean nodes may be visited more than once now?
There was a problem hiding this comment.
Unfortunately yes, I've added another comment:
parcel/packages/core/core/src/requests/AssetGraphRequest.js
Lines 846 to 863 in d7d0d01
(But the hope is that this won't happen often. If it does become a problem, then we should tweak the traversal order)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hide whitespace changes when viewing the diff
This removes the previous two unconditional graph traversals (which in many cases didn't do anything because there were no changes) and instead runs the symbol propagation starting from changes (so
changedAssetsanddependenciesWithRemovedParents, which isn't covered by changedAssets).(If more than 50% of the assets changed, it uses the old full top-down traversal approach in one case, to prevent some revisiting. This 50% threshold was arbitrary, not sure if it's too high or low.)
Additionally:
does not exporterrors in development*symbol to assets without exports (just like with scope-hoisting)*symbol to dynamic import dependenciesassetNode.valuefor changed assets, as opposed to the whole node (which also contains the asset's used symbols)Followup tasks:
import * as xisn't analyzed in dev mode, so there are no CSS module symbol import errors