feat: improve tree-shaking for dynamic import

[TrickyPi]

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:

related #5410

Description

Work in progress

[vercel]

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 8, 2024 1:01pm

[github-actions]

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

[codecov]

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 (00f0681) to head (22baef2).

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

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     

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

[lukastaegert]

Thank you for picking this up! This already works quite well in many cases, but I found a few issues that should be resolved before we merge this. See this example

https://rollup-ciqghdigc-rollup-js.vercel.app/repl/?pr=5420&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMmNvbnN0JTIwbW9kdWxlJTIwJTNEJTIwYXdhaXQlMjBpbXBvcnQoJy4lMkZtb2R1bGUnKSUzQiU1Q24lMkYlMkYlMjBJZiUyMHdlJTIwYWNjZXNzJTIwb25lJTIwZmllbGQlMjBkaXJlY3RseSUyQyUyMGl0JTIwZG9lcyUyMG5vdCUyMHRyYWNrJTIwb3RoZXIlMjB3YXlzJTIwb2YlMjBhY2Nlc3NpbmclMjBwcm9wZXJ0aWVzJTVDbm1vZHVsZS5mb28oKSUzQiU1Q24lNUNuJTJGJTJGJTIwbGlrZSUyMHVzaW5nJTIwYW4lMjAlNUMlMjJ1bmtub3duJTVDJTIyJTIwdmFsdWUlNUNubW9kdWxlJTVCZ2xvYmFsVGhpcy50aGlzTWlnaHRCZUJhciU1RCgpJTNCJTVDbiU1Q24lMkYlMkYlMjBwYXNzaW5nJTIwaXQlMjB0byUyMGElMjBmdW5jdGlvbiU1Q25yZWFkQmFyKG1vZHVsZSklM0IlNUNuZnVuY3Rpb24lMjByZWFkQmFyKG1vZHVsZSklMjAlN0IlNUNuJTIwJTIwbW9kdWxlLmJhcigpJTVDbiU3RCU1Q24lNUNuJTJGJTJGJTIwYXNzaWduaW5nJTIwaXQlMjB0byUyMGFub3RoZXIlMjB2YXJpYWJsZSU1Q25jb25zdCUyMG1vZHVsZUNvcHklMjAlM0QlMjBtb2R1bGUlM0IlNUNubW9kdWxlQ29weS5iYXIoKSUzQiU1Q24lNUNuJTJGJTJGJTIwZGVzdHJ1Y3R1cmluZyU1Q25jb25zdCUyMCU3QiUyMGJhciUyMCU3RCUyMCUzRCUyMG1vZHVsZSUzQiU1Q25iYXIoKSUzQiU1Q24lNUNuJTJGJTJGJTIwdXNpbmclMjBpdCUyMGluJTIwYSUyMHNlcXVlbmNlJTIwZXhwcmVzc2lvbiU1Q24oJ2ZvbyclMkMlMjBtb2R1bGUpLmJhcigpJTNCJTVDbiU1Q24lMkYlMkYlMjB1c2luZyUyMGl0JTIwaW4lMjBhJTIwY29uZGl0aW9uYWwlMjBleHByZXNzaW9uJTVDbih0cnVlJTIwJTNGJTIwbW9kdWxlJTIwJTNBJTIwJ2ZvbycpLmJhcigpJTNCJTVDbiU1Q24lMkYlMkYlMjB1c2luZyUyMGl0JTIwaW4lMjBhJTIwbG9naWNhbCUyMGV4cHJlc3Npb24lNUNuKHRydWUlMjAlMjYlMjYlMjBtb2R1bGUpLmJhcigpJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCUyQyU3QiUyMmNvZGUlMjIlM0ElMjJleHBvcnQlMjBjb25zdCUyMGZvbyUyMCUzRCUyMCgpJTIwJTNEJTNFJTIwY29uc29sZS5sb2coJ2ZvbycpJTNCJTVDbmV4cG9ydCUyMGNvbnN0JTIwYmFyJTIwJTNEJTIwKCklMjAlM0QlM0UlMjBjb25zb2xlLmxvZygnYmFyJyklM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlMkMlMjJuYW1lJTIyJTNBJTIybW9kdWxlLmpzJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMm91dHB1dCUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTdEJTJDJTIydHJlZXNoYWtlJTIyJTNBdHJ1ZSU3RCU3RA==

Basically at the moment if you access some properties of the namespace by name, it will ignore all indirect property accesses, which include

• via unknown/statically not resolvable computed properties
• via destructuring
• via passing the namespace to a function
• via assigning it to another variable
• via accessing it through a sequence, conditional or logical expression

Not sure if this list is complete. In all of these cases, one should probably include all exports. On the other hand if we can handle all of those cases, there is no reason to include any exports by default if the namespace is not accessed at all.

The question is how to do this elegantly. One way of addressing this could be to try to handle all edge cases. Another could be to try to implement the bigger feature of object property tree-shaking.

To do that, one could basically replace the current include method with an includePath(path, context, includeChildrenRecursively, options) method. By default, this would still include the whole object of which we access a single path here

rollup/src/ast/nodes/shared/Expression.ts

Line 76 in 10bdaa3

return true;

However, we can override the method in some strategic locations. I.e. if we include a path ['foo'] of a member expression bar.baz, instead it includes the path ['baz', 'foo'] of bar . We can also shorten paths to prevent some circularity issues here as it is never a problem to include "too much", especially if some path segments are unknown.

[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]

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.707 ± 0.044	9.657	9.738	1.02 ± 0.01
node _benchmark/current/bin/rollup -i ./perf/entry.js -o _benchmark/result/current.js	9.524 ± 0.106	9.418	9.630	1.00

Internal benchmark

• BUILD: 8425ms, 814 MB (+8%)
  • initialize: 0ms, 26.2 MB
  • generate module graph: 3310ms, 583 MB
    • generate ast: 1567ms (+31ms, +2.0%), 576 MB
  • sort and bind modules: 464ms, 628 MB (+3%)
  • mark included statements: 4650ms (-129ms, -2.7%), 814 MB (+8%)
    • treeshaking pass 1: 3004ms (+1378ms, +84.7%), 795 MB (+11%)
    • treeshaking pass 2: 516ms (-272ms, -34.5%), 807 MB (+10%)
    • treeshaking pass 3: 343ms (+35ms, +11.3%), 810 MB (+9%)
    • treeshaking pass 4: 267ms (-17ms, -6.0%), 810 MB (+9%)
    • treeshaking pass 5: 252ms (-77ms, -23.4%), 812 MB (+8%)
    • treeshaking pass 6: 252ms (-19ms, -6.9%), 813 MB (+9%)
    • treeshaking pass 7: 253ms, 747 MB, removed stage
    • treeshaking pass 8: 244ms, 746 MB, removed stage
    • treeshaking pass 9: 221ms, 750 MB, removed stage
    • treeshaking pass 10: 225ms, 755 MB, removed stage
    • treeshaking pass 11: 220ms, 751 MB, removed stage
• GENERATE: 926ms, 1.08 GB (+6%)
  • initialize render: 0ms, 969 MB (+6%)
  • generate chunks: 88ms, 980 MB (+6%)
    • optimize chunks: 0ms, 979 MB (+6%)
  • render chunks: 820ms, 1.05 GB (+6%)
  • transform chunks: 18ms, 1.08 GB (+6%)
  • generate bundle: 0ms, 1.08 GB (+6%)

[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.