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

Add constants inlining optimization #9241

Merged
merged 4 commits into from
Sep 17, 2023
Merged

Add constants inlining optimization #9241

merged 4 commits into from
Sep 17, 2023

Conversation

mattcompiles
Copy link
Contributor

↪️ Pull Request

This PR adds a new optimization that inlines modules that only expose constant literals.
e..g.

export const MY_STRING = 'my-string'';
export const MY_NUM = 3;
export const MY_NULL = null;
export const MY_BIGINT = 3n;
export const MY_BOOLEAN = false;

The idea here is that in almost all cases it's better to inline (not wrap) these modules in all their uses and allow the optimizer to decide how to best handle their usage. This allows skipping the parcel requires and named symbols when accessing constants in most cases. Duplicating constants in a single bundle is not a big issue as compression will optimize for the repeat usage.

Using this feature, very large strings can possibly be duplicated across multiple bundles which may be unideal, potentially we could add a limit in future if a constant was too large to optimize in this way.

We've added a feature switch to the js transformer to enable this feature, currently named unstable_inlineConstants. Not sure we need the unstable prefix here as it's disabled by default, if we agree I can remove it.

🚨 Test instructions

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

Co-authored-by: gorakong <gora.kong1@gmail.com>
Co-authored-by: celinanperalta <celinanperalta@gmail.com>
@@ -101,6 +101,9 @@ const CONFIG_SCHEMA: SchemaEntity = {
},
],
},
unstable_inlineConstants: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure we need the unstable prefix here. Should I remove?

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on how confident we are in the implementation, or do we want to test it in Jira first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've already tested Jira locally. I think let's keep it as unstable for now as it gives us the most flexibility.

@parcel-benchmark
Copy link

parcel-benchmark commented Sep 13, 2023

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.45s +16.00ms
Cached 278.00ms -1.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/index.html 826.00b +0.00b 408.00ms +21.00ms ⚠️
dist/modern/index.html 749.00b +0.00b 408.00ms +22.00ms ⚠️
dist/legacy/index.b8ae99ba.css 94.00b +0.00b 289.00ms +29.00ms ⚠️
dist/modern/index.31cedca9.css 94.00b +0.00b 289.00ms +29.00ms ⚠️

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/index.b8ae99ba.css 94.00b +0.00b 262.00ms -23.00ms 🚀
dist/modern/index.31cedca9.css 94.00b +0.00b 262.00ms -23.00ms 🚀

React HackerNews ✅

Timings

Description Time Difference
Cold 4.13s +92.00ms
Cached 427.00ms -11.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/PermalinkedComment.3145598b.js 3.94kb +0.00b 313.00ms -58.00ms 🚀
dist/UserProfile.b37bbaff.js 1.38kb +0.00b 314.00ms -56.00ms 🚀
dist/NotFound.c08212ea.js 265.00b +0.00b 314.00ms -56.00ms 🚀
dist/logo.8dd07848.png 244.00b +0.00b 225.00ms -43.00ms 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/index.js 460.91kb +0.00b 1.02s -200.00ms 🚀
dist/logo.8dd07848.png 244.00b +0.00b 236.00ms -25.00ms 🚀

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 37.38s -423.00ms
Cached 2.38s +13.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/media-viewer.bc1a2415.js 537.32kb +0.00b 12.98s +757.00ms ⚠️
dist/archive.fe044de4.js 60.16kb +0.00b 9.36s -3.17s 🚀
dist/index.html 248.00b +0.00b 6.77s -5.82s 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/media-viewer.bc1a2415.js 537.32kb +0.00b 8.74s -3.79s 🚀
dist/component-lazy.aeb22f50.js 59.50kb +0.00b 5.69s -889.00ms 🚀
dist/pdfRenderer.6335b9a2.js 12.08kb +0.00b 11.22s +2.22s ⚠️
dist/index.runtime.1064c960.js 7.29kb +0.00b 11.95s -730.00ms 🚀
dist/index.b16227d6.css 4.08kb +0.00b 11.97s -720.00ms 🚀
dist/codeViewerRenderer.7d374cd5.js 2.61kb +0.00b 11.22s +2.22s ⚠️
dist/pt_BR.1db6fd92.js 2.06kb +0.00b 8.05s +1.40s ⚠️
dist/heading3.82217cc7.js 1.35kb +0.00b 5.69s -887.00ms 🚀
dist/heading4.bc1ea347.js 1.12kb +0.00b 5.69s -886.00ms 🚀
dist/ro.ee42c980.js 478.00b +0.00b 8.05s +1.40s ⚠️
dist/index.html 248.00b +0.00b 12.00s -734.00ms 🚀

Three.js ✅

Timings

Description Time Difference
Cold 2.96s +3.00ms
Cached 336.00ms +15.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/Three.js 572.22kb +0.00b 1.04s +106.00ms ⚠️

Click here to view a detailed benchmark overview.

@mattcompiles mattcompiles merged commit 52254bb into v2 Sep 17, 2023
15 of 16 checks passed
@mattcompiles mattcompiles deleted the inline-constants branch September 17, 2023 23:45
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