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

Optimise the Inline Requires plugin #9664

Merged
merged 4 commits into from Apr 23, 2024

Conversation

marcins
Copy link
Contributor

@marcins marcins commented Apr 22, 2024

↪️ Pull Request

In our large application build, the Inline Requires plugin adds about 25% to the build time (6 min) - so we have looked at some quick optimisation wins.

While the ideal solution for inline requires would be to implement it in the ScopeHoistingPackager, a good time to do that would be when we move the packager to native code rather than working on a complex change to it now.

With a combination of tracing, profiling, and eyeballing (thanks @yamadapc!) we identified a few small performance improvements to the current implementation

  • remove potentially expensive object allocation for comparing argument values and replace with a verbose if (this saves some GC overhead and provides a 10-15% perf improvement in our tests)
  • remove unnecessary usage of an array from an earlier iteration where a boolean would suffice (this saves GC overhead and reduces time spent in the visitor)
  • remove verbose logging - even when not logging, there was still significant overhead from this due to the volume (another 10-15%)

The main contributor now to this plugin's run time is the SWC print of the AST back to JS. We have considered a couple of other potential optimisations (that really still just avoid moving the code into the pacakger where it should be...):

  • replace the SWC code with regex matching and replacement - not sure if this would be faster without trying it, but it would complicate source maps for sure.
  • move the JS code to Rust - this will avoid serialising/deserialising the AST that SWC has to do with the parse/print pattern.
  • implement minification support in the plugin - this means that we can skip running the SWC minifier, and just have this plugin produce the minified output - saving another print/parse that plugin has to do.

🚨 Test instructions

Verified application still works (unit tests for the visitor pass)

✔️ 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

@yamadapc yamadapc self-requested a review April 22, 2024 07:47
@marcins
Copy link
Contributor Author

marcins commented Apr 22, 2024

Thought of another idea while folding laundry, but not sure if it'd make a difference - there's no need to maintain a Map of public asset ids -> side effects + filePath. The filePath is only used as a convenience for logging, so this could easily be a Set of only public asset ids that have side effects. This turns a map.has + map.get to work out if the asset has side-effects, into just a set.has.

publicIdToAssetSideEffects = new Map<string, SideEffectsMap>();

@marcins
Copy link
Contributor Author

marcins commented Apr 23, 2024

I want to cut another dev release with the asset map -> set change and test it before merging this.. currently blocked by the OpenSSL cross-compile issue though, and I don't want to "revert" the Sentry PR in this branch again just to test.

Just waiting for a dev release to build now, so I can test this change in CI.

@marcins marcins enabled auto-merge (squash) April 23, 2024 06:52
@marcins marcins merged commit 71a8493 into v2 Apr 23, 2024
25 of 26 checks passed
@mischnic mischnic deleted the mszczepanski/inline-requires-perf-optimisation branch April 23, 2024 08:35
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