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

Improve performance by directly constructing AST from buffer #5391

Merged
merged 27 commits into from Feb 16, 2024

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Feb 14, 2024

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

At long last, this is the next step in improving Rollup's performance. Previously when parsing code, Rollup would first construct a JSON AST and then convert this into an internal AST structure. Now with this change, Rollup skips the JSON step and directly constructs the internal AST from the buffer.
One reason this took a while is also that I needed to figure out what to do

  • If the AST is requested in moduleParsed/this.getModuleInfo: We construct it lazily for now, just like when this.parse is called.
  • If a plugin returns an AST from its transform hook: For compatibility reasons we use the old algorithm to parse it. However, I think we should deprecate and possibly remove this ability in the next major version (but we could provide an alternative so that plugins can return a buffer-based AST)
  • When the cache is used, we also parse and store the JSON AST. In the next major version, I want to change the cache to become binary, at which point it should become much faster and more useful, but I am sure this would be a breaking change for some.

So how much do we gain? To that end, I completely rewrote the perf script included with Rollup. Now when you run npm run perf, it will basically rebuild the esbuild benchmark of bundling ThreeJS ten times but without plugins, i.e. no minification. This will then be run 5 times for the current build and 5 times for what is installed in node_modules, taking turns between the two to compensate for changes in machine utilization. The largest two results from both are dropped and the rest is averaged.
Then we are running Rollup with the perf option to get detailed timings so that it is possible to see where changes come from. This is the result:

# BUILD: 2929ms (-471ms, -13.9%), 742 MB (-16%)
## initialize: 0ms, 23.1 MB (+8%)
## generate module graph: 1009ms (-494ms, -32.9%), 557 MB (-21%)
generate ast: 493ms (-4953ms, -90.9%), 557 MB (-21%)
## sort and bind modules: 181ms, 596 MB (-20%)
## mark included statements: 1737ms (+23ms, +1.3%), 742 MB (-16%)
treeshaking pass 1: 577ms, 706 MB (-17%)
treeshaking pass 2: 288ms, 724 MB (-17%)
treeshaking pass 3: 113ms, 728 MB (-17%)
treeshaking pass 4: 106ms, 736 MB (-16%)
treeshaking pass 5: 120ms, 737 MB (-17%)
treeshaking pass 6: 103ms, 736 MB (-17%)
treeshaking pass 7: 92ms, 736 MB (-17%)
treeshaking pass 8: 88ms, 738 MB (-17%)
treeshaking pass 9: 80ms, 737 MB (-17%)
treeshaking pass 10: 81ms, 741 MB (-16%)
treeshaking pass 11: 81ms, 742 MB (-16%)
# GENERATE: 363ms, 1.01 GB (+5%)
## initialize render: 0ms, 901 MB
## generate chunks: 35ms, 903 MB
optimize chunks: 0ms, 904 MB
## render chunks: 321ms, 985 MB (+5%)
## transform chunks: 7ms, 1.01 GB (+5%)
## generate bundle: 0ms, 1.01 GB (+5%)

So you can see

  • build time (i.e. running const bundle = rollup(inputOptions) is reduced by 14%. This can be attributed to module graph generation, which has been reduced by 34% (the generate AST line is an artifact as I changed how it is measured, the old value was not helpful). This is a lot!
  • The biggest remaining part is tree-shaking. Of course the goal is to move this to Rust eventually as well, but unless we deprecate the old JSON-based logic, this would also mean duplicating a lot of code.
  • Plugins will not be able to benefit from these improvements properly. One vision I have is adding new plugin APIs that can do asynchronous AST generation, lazy JSON serialization and optimized tree-walking APIs to improve here.
  • build peak memory consumption has also been reduced by 16%. I think this can easily be attributed to not generating the JSON AST.
  • peak memory consumption is still dictated by the output generation step, and there has even been a slight increase. I wonder if there is some low-hanging fruit.

Copy link

vercel bot commented Feb 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rollup ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 16, 2024 1:04pm

Copy link

github-actions bot commented Feb 14, 2024

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#ast-consume-buffer

Notice: Ensure you have installed Rust nightly. If you haven't installed it yet, please first see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust, then see https://rust-lang.github.io/rustup/concepts/channels.html to learn how to install Rust nightly.

or load it into the REPL:
https://rollup-icwc9g5qg-rollup-js.vercel.app/repl/?pr=5391

Copy link

codecov bot commented Feb 14, 2024

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (5493159) 98.81% compared to head (16edcea) 98.80%.

Files Patch % Lines
src/ast/bufferParsers.ts 99.13% 0 Missing and 3 partials ⚠️
src/ast/nodes/ArrowFunctionExpression.ts 50.00% 1 Missing ⚠️
src/ast/nodes/shared/FunctionBase.ts 90.00% 1 Missing ⚠️
src/ast/nodes/shared/Node.ts 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5391      +/-   ##
==========================================
- Coverage   98.81%   98.80%   -0.02%     
==========================================
  Files         232      236       +4     
  Lines        9019     9418     +399     
  Branches     2355     2396      +41     
==========================================
+ Hits         8912     9305     +393     
- Misses         46       48       +2     
- Partials       61       65       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* Do not generate cache on CLI unless requested
* Keep AST if cache is not explicitly disabled
This does not work on non-x86 architectures
@lukastaegert lukastaegert merged commit 14c9662 into master Feb 16, 2024
27 of 28 checks passed
@lukastaegert lukastaegert deleted the ast-consume-buffer branch February 16, 2024 13:17
Copy link

This PR has been released as part of rollup@4.12.0. You can test it via npm install rollup.

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

1 participant