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

Incremental Symbol Propagation #8723

Merged
merged 47 commits into from
Mar 26, 2023
Merged

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Dec 28, 2022

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 changedAssets and dependenciesWithRemovedParents, 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:

  • always run symbol propagation (i.e. in development and/or without scope-hoisting).
    • So now there's more subgraph exclusion happening
    • does not export errors in development
  • no-scope-hoist to work with symbol propagation:
    • add * symbol to assets without exports (just like with scope-hoisting)
    • add * symbol to dynamic import dependencies
  • There were a few test cases where the fixture failed once symbol propagation was enabled.
  • Unrelated: speed up test cases by caching native module hashing
  • In the case of safeToIncrementallyBundle, we previously only copied over assetNode.value for changed assets, as opposed to the whole node (which also contains the asset's used symbols)

Followup tasks:

  • Move propagateSymbols() et al. into the prepared separate file
  • Consolidate test cases related to symbols into a separate file (as opposed to scope-hoisting.js / javascript.js)
  • Improve transformer: import * as x isn't analyzed in dev mode, so there are no CSS module symbol import errors

@mischnic mischnic force-pushed the symbol-propagation-changedAssets branch from 5606d3c to f9e47dd Compare December 28, 2022 16:05
@parcel-benchmark
Copy link

parcel-benchmark commented Dec 28, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.49s +24.00ms
Cached 349.00ms -38.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

React HackerNews ✅

Timings

Description Time Difference
Cold 6.97s -20.00ms
Cached 432.00ms -7.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/PermalinkedComment.60e78a07.js 4.18kb +0.00b 453.00ms -57.00ms 🚀
dist/UserProfile.c18819ee.js 1.57kb +0.00b 454.00ms -57.00ms 🚀
dist/NotFound.cfeedbab.js 427.00b +0.00b 453.00ms -58.00ms 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/PermalinkedComment.60e78a07.js 4.18kb +0.00b 457.00ms +23.00ms ⚠️
dist/UserProfile.c18819ee.js 1.57kb +0.00b 457.00ms +23.00ms ⚠️
dist/NotFound.cfeedbab.js 427.00b +0.00b 457.00ms +23.00ms ⚠️

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.04m -1.21s
Cached 2.11s -51.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/refractor.c460668c.js 601.81kb +0.00b 9.32s -1.77s 🚀
dist/mobile-upload.f055fc7f.js 66.66kb +0.00b 5.12s -270.00ms 🚀
dist/component.e578d640.js 58.27kb +0.00b 5.11s -268.00ms 🚀
dist/Modal.4be3b837.js 28.46kb +0.00b 5.11s -273.00ms 🚀
dist/component.1c22aee9.js 18.81kb +0.00b 5.11s -269.00ms 🚀
dist/js.324be058.js 17.34kb +0.00b 5.10s -273.00ms 🚀
dist/mobile-upload.93995326.js 8.08kb +0.00b 5.10s -273.00ms 🚀
dist/Modal.4d03aeec.js 3.79kb +0.00b 5.10s -274.00ms 🚀
dist/component.9a535981.js 3.42kb +0.00b 5.10s -273.00ms 🚀
dist/png-chunks-extract.c54842d7.js 3.19kb +0.00b 5.10s -273.00ms 🚀
dist/ru.896915b9.js 2.94kb +0.00b 6.04s -2.42s 🚀
dist/workerHasher.be59eb41.js 1.72kb +0.00b 5.10s -273.00ms 🚀
dist/16.87c743d1.js 1.48kb +0.00b 5.11s -283.00ms 🚀
dist/16.dd50aef4.js 1.41kb +0.00b 5.11s -280.00ms 🚀
dist/16.9e7cc0d9.js 1.13kb +0.00b 5.11s -283.00ms 🚀
dist/16.8d078bd1.js 1.08kb +0.00b 5.11s -280.00ms 🚀
dist/16.bb53313d.js 1.08kb +0.00b 5.11s -283.00ms 🚀
dist/16.88e24f19.js 1.06kb +0.00b 5.11s -272.00ms 🚀
dist/16.db9c75f1.js 1.03kb +0.00b 5.11s -282.00ms 🚀
dist/16.c0880b62.js 992.00b +0.00b 5.11s -283.00ms 🚀
dist/16.99296be0.js 964.00b +0.00b 5.11s -282.00ms 🚀
dist/16.c16ee42d.js 957.00b +0.00b 5.11s -283.00ms 🚀
dist/16.26c3d518.js 912.00b +0.00b 5.11s -283.00ms 🚀
dist/16.f76b9cae.js 906.00b +0.00b 5.11s -279.00ms 🚀
dist/16.fb327623.js 906.00b +0.00b 5.11s -280.00ms 🚀
dist/16.f2056258.js 905.00b +0.00b 5.11s -283.00ms 🚀
dist/16.4e7dec68.js 904.00b +0.00b 5.11s -269.00ms 🚀
dist/16.400116d9.js 903.00b +0.00b 5.11s -271.00ms 🚀
dist/16.24326b68.js 855.00b +0.00b 5.11s -272.00ms 🚀
dist/16.0285f4b2.js 827.00b +0.00b 5.11s -282.00ms 🚀
dist/simpleHasher.395b29e6.js 687.00b +0.00b 5.10s -273.00ms 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/media-viewer.bd165005.js 542.15kb +0.00b 8.10s -3.08s 🚀

Three.js ✅

Timings

Description Time Difference
Cold 4.87s -5.00ms
Cached 283.00ms +9.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@mischnic mischnic force-pushed the symbol-propagation-changedAssets branch from 3f2252e to c441fd2 Compare December 29, 2022 11:20
@devongovett
Copy link
Member

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?

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;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

(This is the workaround to speedup the integration tests)

@mischnic
Copy link
Member Author

mischnic commented Dec 31, 2022

I've moved the code back (be sure to hide whitespace changes!), will do that again in a separate follow up PR.
Logic changes (surprisingly few):

  • In propagateSymbolsDown, it now starts with changedAssets and dependenciesWithRemovedParents as opposed to the root node.
  • in propagateSymbolsUp
    • runFullPass is the heuristic for determining whether to use the old approach (full traversal + incremental for cycles) or not (incremental for changes), the only difference here is performance. The result should be the same
    • the incremental traversal can start with changedDepsUsedSymbolsUpDirtyDown which are the nodes that were changed in propagateSymbolsDown.
    • the incremental traversal already existed previously because of cyclic dependencies, but there was a missing if (dep.usedSymbolsUpDirtyDown)

  • In the integration tests, the only difference between scopehoisting and non-scopehoisting now is that scopehoisting can skip intermediate assets while nonscopehoisting has to still include them for the proper reexport statements (so it can only exclude whole subgraphs).

Any performance numbers you've seen so far?

Testing with ak-editor (on initial builds, both passes were ~200ms each before):

  • propagateSymbolsDown is slightly faster than before in general.
  • If runFullPass comes into effect (e.g. initial build), then the performance for propagateSymbolsUp is the same. Then for single-asset changes the time for propagation is effectively 0 because it just updates that one single asset.

I haven't tested the case where a large dependency is added.

@mischnic mischnic force-pushed the symbol-propagation-changedAssets branch from a8e6e10 to 812bce3 Compare February 27, 2023 11:21
@mischnic mischnic force-pushed the symbol-propagation-changedAssets branch from aebbf04 to 792aee9 Compare February 28, 2023 23:10
@mischnic mischnic force-pushed the symbol-propagation-changedAssets branch from 792aee9 to 4b47954 Compare March 1, 2023 09:26
@mischnic mischnic force-pushed the symbol-propagation-changedAssets branch from ff2873f to 2a72bb5 Compare March 1, 2023 22:35
@mischnic mischnic force-pushed the symbol-propagation-changedAssets branch from 2a72bb5 to 166a043 Compare March 1, 2023 22:42
packages/core/core/src/SymbolPropagation.js Show resolved Hide resolved
@@ -747,110 +889,150 @@ export class AssetGraphBuilder {
} else if (childNode.type === 'dependency') {
childDirty = childNode.usedSymbolsDownDirty;
}
if (!visited.has(child) || childDirty) {
if (childDirty) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean nodes may be visited more than once now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately yes, I've added another comment:

// In the worst case, some nodes have to be revisited because we don't want to sort the assets
// into topological order. For example in a diamond graph where the join point is visited twice
// via each parent (the numbers signifiying the order of re/visiting, `...` being unvisited).
// However, this only continues as long as there are changes in the used symbols that influence
// child nodes.
//
// |
// ...
// / \
// 1 4
// \ /
// 2+5
// |
// 3+6
// |
// ...
// |
//

(But the hope is that this won't happen often. If it does become a problem, then we should tweak the traversal order)

packages/transformers/js/src/JSTransformer.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants