feat: improve function return value tracking - #6065
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6065 +/- ##
==========================================
- Coverage 98.78% 98.76% -0.02%
==========================================
Files 275 276 +1
Lines 10823 10885 +62
Branches 2887 2908 +21
==========================================
+ Hits 10691 10751 +60
- Misses 89 91 +2
Partials 43 43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install cyyynthia/rollup#function-return-valuesNotice: Ensure you have installed the latest nightly 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
|
lukastaegert
left a comment
There was a problem hiding this comment.
This PR does a lot of things, and the concept of locallyReachable makes me a little uneasy. Also, I do not fully understand this concept, is this meant as a "there is a chance this node might be included at some point"?
If, for instance, we would just take the literal value of all included return statements into account and then deoptimize if another return statement is included, would this deoptimize too often because the value is queried too early before any return statement is included?
Another point is that so far, I have avoided to track multiple return expressions out of fear of a performance regression. Below is a (completely artificial) example that demonstrates this. At least for me, this will basically make the browser hang for a long time. Removing the pr=6065& from the URL, i.e. using production Rollup, will make it load quickly:
As this is an artificial example, many code-bases will not have this issue. But some will, or at least they will be noticeably slowed, especially if they have some functions with a lot of return statements. To that end, I would make this feature opt-in, e.g. as treeshake.trackAllReturnValues, but add this to the treeshake.preset: "smallest" preset.
So in general, this is how I would propose to proceed
- Look into aborting binding in a separate PR.
- Tracking multiple return values shop be opt-in via parameter
- Can we implement tracking multiple return statements without the additional "isLocallyReachable" static analysis? While the logic is not too complicated, I am worried that it is another separate layer of executed code path analysis that is only used for this one feature, where bugs may be noticed very late, and which will not benefit from improvements in other areas. Another way to do this could be to start tracking return statements the first time either
hasEffects,includeorincludePathare called on a return statement, as that is equivalent to "we know this return statement is in an executed code path". This is also the semantic we use forapplyDeoptimizationsand actually, this could just become another "deoptimization" to perform on return statements I theirapplyDeoptimizationsmethod, i.e. add a return value. Now the important thing would be to make sure that when a return statement is added and a consumer already queried the literal return value of the function (tracked via theoriginparameter), then that consumer is deoptimized. However, I hope this is not a wild goose chase.
What do you think?
There was a problem hiding this comment.
Hm, you're definitely right that this kind of performance regression is unacceptable (regardless of the exotic/synthetic nature of the scenario). I convinced myself along the way that it shouldn't be too much of a performance issue but I overlooked the exponential growth of return values in deep recursive function call chains that can definitely show up in the real world.
Having full-depth search opt-in seems a good idea. I'm wondering if depth control would allow for a good balance between performance and output quality (by default have a depth of only 1 or 2, and in the smallest preset have a depth of Infinity; i.e. exhaustive return value search.
It's very likely that performance can be improved for some scenarios through caching and smarter exploration, perhaps beyond just this use-case if getLiteralValue* gets smarter. I'll explore this further :)
5de9123 to
5d16167
Compare
5d16167 to
cf8e36c
Compare
5369863 to
96b5453
Compare
|
Hi, I am somewhat I slightly lost track of this. What is the current state here, is this currently in a releasable state from your side? Then I would focus on finishing the review next. |
|
Hi! The state of things hasn't moved much since your last review and the concerns you raised still are valid. I tried a bunch of things locally, with more often than not more issues created than solved... And then I ran out of free time to mess with this PR :( The main issue remains that the information I need for the reachability check is not known prior to the inclusion process being in progress, at which point it is too late to react (since Rollup works via an additive only strategy). This is the key to solve #5063 robustly, and is quite important to make deep function return evaluation worth the effort. The PR can likely be split in 2 separate ones; one adding the deep function return value evaluation capabilities, the other making reachability analysis smarter such that treeshaking can have a "recursive" effect without resorting to treeshaking over and over until we have a stable result which would be prohibitively expensive >:) There are some low hanging fruits that can be cherry picked from the PR and improves some situations too, such as making blocks return undefined (instead of UnknownValue) through their implicit return statement. |
|
Thanks for the quick response! I think I will leave it like this for a little longer from my side and focus on getting Rollup 5 ready, there is a lot still to do. After happy I am happy to invest more time here. But feel free to drive this forward in the meantime yourself. |
e0d5ce0 to
622020f
Compare
|
@cyyynthia is attempting to deploy a commit to the rollup-js Team on Vercel. A member of the Team first needs to authorize it. |
|
I'm back at this! 😼 I've removed the code that was re-implementing treeshaking to mitigate dead code elimination not affecting the outcome of const folding, which was one of the major blockers for this PR to go through. The second blocker was performance of exhaustively checking return values. I don't fully remember the specifics on the performance impact this PR has. I improved it a bit since but it'd definitely need to be checked again, to know if this needs to be made opt-in or if having it by default is viable. |
88c072b to
3bc835c
Compare
ab5f6a6 to
d49ca8e
Compare
d49ca8e to
e2b60b3
Compare
|
I had an "eureka" moment; as I'm a lot more familiar with the inner workings of Rollup and its internal dance, I've successfully implemented the fix for #5063 without introducing a weird secondary path for detecting dead code. I'm a lot more confident this approach is the right one and can be merged!! :D |
e2b60b3 to
f89042b
Compare
|
Hi! Great seeing you back, and sorry for my lack of response so far. I had very limited time in the last months. Also, I will be off keyboard for another 2-3 weeks, then reviewing your PRs has high priority! |
lukastaegert
left a comment
There was a problem hiding this comment.
Thank you! I no longer found any obvious flaws here, and would merge this now. Thank you for coming back here! Let's keep an eye on new issues after this is released.
|
This PR has been released as part of rollup@4.63.0. You can test it via |
|
Just a note, we have one report of an OOM after the release. It might be benign, and there is no reproduction yet, but just a heads-up here: #6487 |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
This PR improves the way return values are resolved inside function calls. It does so via 4 changes:
MultiExpressionnode. It has been expanded to allow resolving literal values if all paths agree on a specific value, trying to resolve its truthiness if there are different values being returned.MultiExpressionfactors whether a return statement has been reached, which solves tree-shaking will not execute recursively. #5063 since the resolved literal now depends on the treeshaking status.deoptimizeCacheonly clears the cached value, allowing it to be re-computed. Not all nodes have been tuned to this less pessimistic approach; only the low hanging fruits relevant for this PR.