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

Code splitting across reexports using symbol data by splitting dependencies #8432

Merged
merged 49 commits into from Sep 9, 2022

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Aug 31, 2022

Closes #7392

Symbol propagation already does the traversals anyway, so now it just always tracks for each symbol in a dependency, what that symbol should resolve to.

So for each used symbol, it now says where that symbol should resolve to. This is not exectly the same as getSymbolResolution though, as it stops at non-sideeffect-free assets (while getSymbolResolution does resolve through them).

This information is then used in BundleGraph.fromAssetGraph to split up dependencies to be per symbol (and point them at the resolved asset according to the previous analysis). The old dependency is kept in case some transformer used its placeholder. This changes the execution order because the old dependency is replaced with dep.usedSymbolsUp.map(... => new Dependency(...)) so the arbitrary (sorted alphabetically) order of the symbols is used for this.

Since it's possible to rename symbols via reexports, I've had to add an additional map to the dependency node, e.g. a dependency to a side-effect-free asset that does export {foo as bar} from "y"; would then be rewritten to point directly to y instead, and the symbolTarget map then has foo -> bar which is applied transparently in bundleGraph.getUsedSymbols and bundleGraph.getSymbols (which I think is a better solution compared to mutating the actual DependencyValue symbols entry).

  • Maybe get rid of dependency.symbolTarget

Comment on lines 12 to +15
if (
node.type === 'dependency' &&
node.value.specifier === '@parcel/service-worker'
node.value.specifier === '@parcel/service-worker' &&
!bundleGraph.isDependencySkipped(node.value)
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 was needed because there are now two such dependencies, one of the being excluded.

@mischnic mischnic marked this pull request as ready for review September 3, 2022 13:59
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Would like to do some testing on some large apps to make sure we don't break anything.

@parcel-benchmark
Copy link

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.40s -27.00ms
Cached 342.00ms -1.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

React HackerNews 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

AtlasKit Editor 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description Time Difference
Cold 6.80s +275.00ms
Cached 314.00ms +21.00ms ⚠️

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Let's get this into nightly releases to test.

@devongovett devongovett merged commit 3092d3c into v2 Sep 9, 2022
@devongovett devongovett deleted the symbols-codesplit-split-core branch September 9, 2022 05:23
@marcins
Copy link
Contributor

marcins commented Sep 11, 2022

Code looks good to me. Would like to do some testing on some large apps to make sure we don't break anything.

FYI - I have this change patched in to our very large app for a few days now without build issues, and no runtime issues we're yet aware of (dog-fooding is currently limited for the Parcel builds). It's provided an improvement in runtime performance (hard to quantify as we had a workaround in place rewriting imports with a transform, but it's provided a further 10% improvement over the workaround - I think we're roughly at 30-40% improvement over the baseline we had).

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.

Parcel does not split side effect free re-exports across bundles
4 participants