feat: implement object tree-shaking #5420
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
c8a0fc6 to
2f541ee
Compare
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install rollup/rollup#feat/tree-shaking-5410Notice: Ensure you have installed the latest stable Rust toolchain. If you haven't installed it yet, please see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust. or load it into the REPL: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5420 +/- ##
==========================================
- Coverage 99.00% 98.89% -0.12%
==========================================
Files 258 258
Lines 8070 8204 +134
Branches 1360 1387 +27
==========================================
+ Hits 7990 8113 +123
- Misses 53 60 +7
- Partials 27 31 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
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
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/ 8000 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
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.
|
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
In another step, we could add special handling to 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. |
|
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. |
|
My pleasure! You are doing great work and it turns out sharing knowledge and ideas with you is always a good investment 😉 |
2f541ee to
eda5b5a
Compare
eda5b5a to
080b10f
Compare
40f1b43 to
c6c2366
Compare
c6c2366 to
adf93ab
Compare
65a67f3 to
4f938e3
Compare
|
This PR has been released as part of rollup@4.27.0-1. Note that this is a pre-release, so to test it, you need to install Rollup via |
|
Thanks! I tried it out and the tests in Vite repo now passes 👍 |
|
This PR has been released as part of rollup@4.27.0. You can test it via |
|
Would it be feasible to make this configurable? In my large Vite project (with big JSONs, which could be the culprit), it increases memory usage a lot, to the point of OOM (which doesn't happen with |
|
Unfortunately, that is not really possible as there is a big architecture change behind it that cannot just be toggled, so while we could easily turn off the tree-shaking effects, it is much harder to fix the memory consumption. to whatever is necessary to fix this. Can you give an estimate how much memory consumption changes? In our own benchmark, it only changed by a few percentage points #5420 (comment), so it would be good to know if there is something obvious we missed. |
|
Understood, I was suprised myself given your benchmarks. I tried to search for the lowest max-old-space-size (trial and error) that still allows Node to finish the build for my app (consisting of ~28000 modules). I actually wasn't able to get the build to finish at all with 4.27
Note: I'm building with sourcemaps for Sentry. The OOM happens before chunks start getting rendered. Also, from a certain point, the memory usage increases very slowly until reaching the limit. |
|
Ok, that actually looks like a bug. Especially if it does not complete. I assume this is a private project that cannot be shared, and it is not possible to provide some kind of reproduction? |
|
Yes, it cannot be shared. I can try to come up with some reproduction over the weekend, but unfortunately I cannot guarantee it. But in case somebody else wants to give it a shot, I'd try bundling a project that imports this package, but even that might not be enough, as it's smaller. |
|
Ok, I found one small thing that I overlooked that might cause this. Can you give rollup@4.27.1-0 a shot and see if it improves anything? |
|
Hm, now Maybe similar to this? |
|
But the error happens after the point where Rollup OOM'd previously, so it might be a step in the right direction. |
|
Nice, it definitely is! I wonder if there is any chance we can figure out what the duplicate declarations look like. We would need to see the code of an erroring chunk before it is processed by esbuild. export default defineConfig({
plugins: [
// should be the first plugin
{
name: 'debug-chunks',
enforce: 'pre',
renderChunk(code) {
console.log('\n\n---CHUNK START---\n', code);
}
},
// ... other plugins
]
//...
})Then have a look at the last chunk(s) printed before the error occurs. At the moment, I am still struggling to find any hint what might be overlooked here. It would be enough to just see the two lines where the same variable is declared twice. Then I hopefully can limit my search a little bit, e.g. if I know if one of the declarations is in a destructuring, or in a parameter. |
|
In any case, I will release the fixes I have so far. |
|
I can confirm that rollup 4.27.0 made it into my (small) nuxt project and prevents it from building due to a node memory-leak. |
|
Try 4.27.1 |
4.27.1 fixes the issue. Thank you. |
|
4.27.2 fixes some deconflicting issues |
|
@lukastaegert I will take a look as soon as I can, probably in a few hours. Btw I wasn't able to easily extract the problematic chunk from Vite yet, not even with your plugin |
|
Just came here to say - I spent a chunk of my late morning chasing this exact build failure and I'm ashamed I'm so late (3 hrs lol) to the party. Thanks for the quick fix! |
|
Past experience shows that for such a big feature you can only find so much with thorough testing and beta users. So best thing to release on a day where I know I will find time for quick bug fixes. Thank you all for your patience, hope we are good for the weekend now. |
|
4.27.2 fixes both OOM and variable conflicts! But I'll have to play around with it a bit more to see if the build time is unaffected. Either way thanks Lukas |
This reverts commit a9acb57.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
related #5410
Description
Please see the comments in this PR.