feat: improve tree-shaking for dynamic import

[TrickyPi]

This PR contains:

• [ ] bugfix
• [x] feature
• [ ] refactor
• [ ] documentation
• [ ] other

Are tests included?

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

Breaking Changes?

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

List any relevant issue numbers:

related #5410

Description

~~Work in progress~~

[vercel[bot]]

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	May 12, 2024 2:06pm

[github-actions[bot]]

Thank you for your contribution! ❤️

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

npm install rollup/rollup#feat/tree-shaking-5410

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-bygdhnuk6-rollup-js.vercel.app/repl/?pr=5420

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 99.32432% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 98.81%. Comparing base (3520da5) to head (83b36e1).

Files	Patch %	Lines
src/ast/nodes/ImportExpression.ts	93.33%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5420      +/-   ##
==========================================
- Coverage   98.81%   98.81%   -0.01%     
==========================================
  Files         238      238              
  Lines        9540     9589      +49     
  Branches     2437     2454      +17     
==========================================
+ Hits         9427     9475      +48     
  Misses         48       48              
- Partials       65       66       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[lukastaegert]

That being said, I would really like to encourage you to do this. This could become a feature I literally wanted to implement for years. Basically what you need to do is

• replace include(...) with includePath(EMPTY_PATH, ...) everywhere
• use something better than EMPTY_PATH where it makes sense (e.g. in MemberExpression), but do not try to overdo this for the first release to not extend the scope of the feature too much
• Add special handling for includePath in ImportExpression so that it only includes all fields if it does not receive a specific field.

In another step, we could add special handling to ObjectExpression so that we can start tree-shaking for objects, but I fear there might be implications and edge cases to consider, so starting with ImportExpression would be nice.

One question is what to do with destructuring. At the moment, destructuring will not be able to track paths, which is something you also see when tracking values like here https://rollup-ciqghdigc-rollup-js.vercel.app/repl/?pr=5420&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMmNvbnN0JTIwb2JqJTIwJTNEJTIwJTdCZm9vJTNBJTIwdHJ1ZSUyQyUyMGJhciUzQSUyMGZhbHNlJTdEJTNCJTVDbmlmJTIwKG9iai5iYXIpJTIwY29uc29sZS5sb2coJ3JlbW92ZWQnKSUzQiU1Q24lNUNuY29uc3QlMjAlN0JiYXIlN0QlMjAlM0QlMjBvYmolM0IlNUNuaWYlMjAoYmFyKSUyMGNvbnNvbGUubG9nKCdub3QlMjByZW1vdmVkJTIwZXZlbiUyMHRob3VnaCUyMGl0JTIwc2hvdWxkJTIwYmUnKSUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTJDJTIybmFtZSUyMiUzQSUyMm1haW4uanMlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyb3V0cHV0JTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlN0QlMkMlMjJ0cmVlc2hha2UlMjIlM0F0cnVlJTdEJTdE

But again, this could be a separate improvement.

[TrickyPi]

Wow, thank you so much for your professional and patient guidance! I will thoroughly digest this knowledge (object property tree-shaking) and then work on implementing it.

[lukastaegert]

My pleasure! You are doing great work and it turns out sharing knowledge and ideas with you is always a good investment 😉

[github-actions[bot]]

Performance report!

Rough benchmark

Command	Mean [s]	Min [s]	Max [s]	Relative
node _benchmark/previous/bin/rollup -i ./perf/entry.js -o _benchmark/result/previous.js	9.056 ± 0.083	8.960	9.111	1.01 ± 0.01
node _benchmark/current/bin/rollup -i ./perf/entry.js -o _benchmark/result/current.js	8.932 ± 0.034	8.893	8.957	1.00

Internal benchmark

• BUILD: 7900ms, 810 MB (+8%)
  • initialize: 0ms, 26.2 MB
  • generate module graph: 3124ms, 583 MB
    • generate ast: 1499ms, 576 MB
  • sort and bind modules: 449ms, 626 MB
  • mark included statements: 4332ms (-164ms, -3.6%), 810 MB (+8%)
    • treeshaking pass 1: 2765ms (+1251ms, +82.7%), 794 MB (+11%)
    • treeshaking pass 2: 486ms (-250ms, -34.0%), 806 MB (+9%)
    • treeshaking pass 3: 325ms (+38ms, +13.0%), 809 MB (+8%)
    • treeshaking pass 4: 258ms (-12ms, -4.6%), 809 MB (+9%)
    • treeshaking pass 5: 245ms (-68ms, -21.6%), 809 MB (+8%)
    • treeshaking pass 6: 243ms (-14ms, -5.4%), 810 MB (+8%)
    • treeshaking pass 7: 240ms, 749 MB, removed stage
    • treeshaking pass 8: 234ms, 749 MB, removed stage
    • treeshaking pass 9: 213ms, 755 MB, removed stage
    • treeshaking pass 10: 215ms, 753 MB, removed stage
    • treeshaking pass 11: 212ms, 751 MB, removed stage
• GENERATE: 874ms, 1.08 GB (+5%)
  • initialize render: 0ms, 969 MB (+6%)
  • generate chunks: 81ms, 982 MB (+6%)
    • optimize chunks: 0ms, 978 MB (+6%)
  • render chunks: 775ms, 1.05 GB (+8%)
  • transform chunks: 16ms, 1.08 GB (+5%)
  • generate bundle: 0ms, 1.08 GB (+5%)

[TrickyPi]

Hi, Lukas! I reopened this PR. There are three main changes.

• The first one is replacing all include(...) with includePath(...).

• The second change is extending includePath function in the Identifier, ImportExpression, MemberExpression, ObjectPattern and LocalVariable. The change in Identifier should be NOTICED, I introduced a new field includedPaths to avoid maximum call stack size exceeded if there are cyclic imports (a related cyclic imports test test/function/samples/cycles-defaults/_config.js). However, this will cause high memory consumption and minor time consumption, as indicated by the performance report. What do you think?

• The last change is that including all exports from dynamic dependencies by default at the end of tree-shaking (see Graph.ts), so we don't need to worry about assigning namespace to global variable, ~~passing the namespace to a function~~, and other places that we may overlook.

[lukastaegert]

Very excited for this one, will give it a proper review in the next days.