Reapply object tree-shaking #5737
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install rollup/rollup#object-property-treeshakingNotice: 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: |
Performance report
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5737 +/- ##
==========================================
- Coverage 98.79% 98.54% -0.25%
==========================================
Files 260 269 +9
Lines 8123 8527 +404
Branches 1373 1460 +87
==========================================
+ Hits 8025 8403 +378
- Misses 72 92 +20
- Partials 26 32 +6 ☔ View full report in Codecov by Sentry. |
1331697 to
6ff5dee
Compare
|
I added a fix for #5735. What I found is that for MemberExpressions, we dynamically determine the value of computed properties not only when determining their "literal value" or their "return value", but also when deciding what to include or deoptimize. For the latter two cases, we do not have a mechanism to handle the case where we discover that the property value is reassigned later, which is also the case when a function is called more than once with different values. |
131b6b2 to
3e21bd8
Compare
|
I added another fix for #5739. There was indeed a severe issue here: If the first call to a function with rest arguments did not supply any parameter value that would make it into the rest argument, then the logic would wrongly assume the value to be |
cc5d380 to
a03960c
Compare
47d3e40 to
ecffc17
Compare
|
👍 Awesome! The "Maximum call stack size exceeded" error is fixed in commit 40e2447! |
|
But it is noticeably slower now. So this is what the problem was: Because we replaced |
8deec2b to
c10d65d
Compare
|
This PR has been released as part of rollup@4.31.0-0. Note that this is a pre-release, so to test it, you need to install Rollup via |
# Conflicts: # browser/package.json # package-lock.json # package.json
This is a massive performance improvement especially in scenarios with recursive calls where different properties of a function argument are passed to the same function.
4f225f0 to
efe7b2f
Compare
This should save some memory which is rarely needed.
efe7b2f to
e9f1e3a
Compare
|
This PR has been released as part of rollup@4.33.0-0. Note that this is a pre-release, so to test it, you need to install Rollup via |
# Conflicts: # browser/package.json # package-lock.json # package.json # test/form/samples/pure-comment-scenarios-complex/_expected.js
|
This PR has been released as part of rollup@4.34.0. You can test it via |
|
I believe this has just broken my build, I'm getting errors with rollup 4.34. My application uses webassembly (https://github.com/bytecodealliance/jco and the wasi-p2 shim), triggering an exception "TypeError: invalid variant tag value I can try and make a minimal reproduction and open an issue for this later. |
|
Thank you to the quick reproduction. Let me know if |
|
I'm not sure if I should report this as a bug, but, it seems function properties are treated specially, and using any function property retains all of them? Even if they don't use const obj = {
x: 1,
y: () => {},
z: () => {}
};
console.log(obj.x); // only retains x
console.log(obj.y()); // retains both y and z |
|
And I would love to see this extended to assigned properties, as in: function foo() {}
foo.bar = function() {}
foo.baz = function() {}
console.log(foo.bar()); // only retains bar |
|
At the moment, the property tree-shaking logic is quite conservative and, as you see, does not include properties that are added later. One big complexity comes from the fact that any assignment could trigger a setter that again modifies the entire object (admittedly, this also applies to getters...). Also, a method call can modify the object via this. I assume the issues come from some deoptimizations in this area. We plan on adding further optimizations, but at the moment we focus on making sure that no bugs crept in. |
This reverts commit 10bc150.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
This re-applies #5420 when we have fixes for #5733, #5734, #5735, #5480 (comment) and some improvement for #5729