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

Register symbols even without scopehoisting #7222

Merged
merged 14 commits into from Nov 16, 2021
Merged

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Oct 31, 2021

This enables

  • asset deferring
  • paves the way to enable symbol propagation in dev mode (once the perf problems are resolved), which would enable
    • excluding even more subgraphs during bundling (and packaging)
    • "does not export" errors

Known limitations for now:

  • async imports aren't analyzed (because this happens in the Hoist visitor, not Collect)

Close T-733


Testing an app in dev/serve mode with import { Provider, defaultTheme, Button } from "@adobe/react-spectrum":

(so before, every component was transformed and bundled. now it's only the ones that are actually imported)

JS bundle CSS bundle cold start no-change warm start cache size (data.mdb)
before 2788kb 753kb 3s 46ms 10465kb
after 1605kb 147kb 2.54s 23ms 5402kb

IIRC in #5528, the performance improvement more significant because the JS transformer pipeline was slower.


For https://github.com/mischnic/parcel-plugin-browser, many functions in date-fns and react-use now are skipped. The dev bundle went from 1.92mb to 1.31mb, the sourcemap from 3.04mb to 1.96mb. Cold startup into dev goes from 2.2s to 1.8s

@height
Copy link

height bot commented Oct 31, 2021

This pull request has been linked to and will mark 1 task as "Done" when merged:

  • T-733 also collect symbols data without scope-hoisting and defer in dev-mode (unlink task)

💡Tip: You can link multiple Height tasks to a pull request.

@parcel-benchmark
Copy link

parcel-benchmark commented Oct 31, 2021

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.97s +64.00ms
Cached 309.00ms -5.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/index.c1bc86aa.css 94.00b +0.00b 1.12s +66.00ms ⚠️
dist/modern/index.57a95cbe.css 94.00b +0.00b 1.12s +67.00ms ⚠️

React HackerNews ✅

Timings

Description Time Difference
Cold 9.80s -16.00ms
Cached 443.00ms -24.00ms 🚀

Cold Bundles

Bundle Size Difference Time Difference
dist/index.js 487.72kb +11.00b ⚠️ 5.30s +85.00ms

Cached Bundles

Bundle Size Difference Time Difference
dist/index.js 487.72kb +11.00b ⚠️ 5.20s -24.00ms

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.07m -167.00ms
Cached 1.56s +20.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/index.047df881.js 1.78mb +156.00b ⚠️ 18.44s +273.00ms
dist/index.f0cb9522.js 693.77kb +73.00b ⚠️ 49.07s -775.00ms
dist/EmojiPickerComponent.07ae4435.js 147.12kb +10.00b ⚠️ 30.34s -275.00ms
dist/dropzone.e0209bf8.js 12.15kb +11.00b ⚠️ 48.60s -725.00ms

Cached Bundles

Bundle Size Difference Time Difference
dist/index.90fbf3c9.js 1.78mb +156.00b ⚠️ 18.28s -377.00ms
dist/index.f0cb9522.js 693.77kb +73.00b ⚠️ 49.19s -360.00ms
dist/EmojiPickerComponent.07ae4435.js 147.12kb +10.00b ⚠️ 30.18s -241.00ms
dist/dropzone.e0209bf8.js 12.15kb +11.00b ⚠️ 48.71s -296.00ms

Three.js ✅

Timings

Description Time Difference
Cold 6.76s -35.00ms
Cached 343.00ms -22.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 noscopehoist-symbols-swc branch 3 times, most recently from 1c6de95 to 460d0ee Compare November 6, 2021 17:27
@mischnic mischnic marked this pull request as ready for review November 6, 2021 18:19
packages/transformers/js/core/src/collect.rs Outdated Show resolved Hide resolved
includeNodeModules: {
'@parcel/transformer-js': true,
} else {
if (symbol_result) {
Copy link
Member

Choose a reason for hiding this comment

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

Is the following duplicated? Should we combine hoist_result and symbol_result somehow?

@devongovett
Copy link
Member

Would be great to test this in a large app too. cc @lettertwo @thebriando @AGawrys @gorakong

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.

Seems good overall. One question

exports: collect
.exports
.into_iter()
.map(
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to map these to a different struct with (almost) the same shape? Should we just expose the original? The less copying the better...

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be better, but they all contain IdentId (so (JsWord, SyntaxContext)) and the letter prevents serialization.

Copy link
Member

Choose a reason for hiding this comment

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

Ah... ok

@devongovett devongovett merged commit 46fe00f into v2 Nov 16, 2021
@devongovett devongovett deleted the noscopehoist-symbols-swc branch November 16, 2021 03:28
lettertwo added a commit that referenced this pull request Nov 18, 2021
* upstream/v2:
  Upgrade Flow to 0.164.0 (#7297)
  Chore: fix typo initialise -> initialize in core/Parcel.js (#7309)
  Register symbols even without scopehoisting (#7222)
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.

None yet

3 participants