feat: improve tree-shaking by propagate const parameter

[liuly0322]

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:

Description

Hi, all! This Pull Request trys to collect all function calls of top-level functions, so that we can determine if a param of the function is a literal to improve tree shaking (or be used to remove unused default params, which is not included in this PR)

The idea comes from #5435 , and related PRs: #4498 , #4510 , which are finally reverted by #4520

This PR has changed the original code framework to some extent (to use a new approach to collect function use-def information), so I want to know whether the method of collecting all references in the PR and the placement position of the optimization pass are reasonable, thanks!

before:

function fun1(enable) {
	if (enable) {
		return 'fun1';
	}
}

function fun2(enable) {
	if (enable) {
		return fun1(enable);
	}
}

console.log(fun2(true));

after:

function fun1(enable) {
	{
		return 'fun1';
	}
}

function fun2(enable) {
	{
		return fun1();
	}
}

console.log(fun2());

[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	Apr 20, 2024 5:54pm

[liuly0322]

Update: I think it will be pretty useful for library usage, I have just update an optimization:

before:

function fun1(options) {
	if (options.enable) {
		return 'fun1';
	} else {
		console.log(222);
	}
}

function fun2(options) {
	if (options.enable) {
		return fun1(options);
	} else {
		console.log(111);
	}
}

console.log(
	fun2({
		enable: true
	})
);

output:

function fun1(options) {
	if (options.enable) {
		return 'fun1';
	}
}

function fun2(options) {
	if (options.enable) {
		return fun1(options);
	}
}

console.log(
	fun2({
		enable: true
	})
);

and providing an options parameter is a common pattern for many librarys.

[github-actions]

Thank you for your contribution! ❤️

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

npm install liuly0322/_rollup#master

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

[codecov]

Codecov Report

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

Project coverage is 98.83%. Comparing base (e6e05cd) to head (3f2f45e).

Files	Patch %	Lines
src/ast/nodes/shared/FunctionBase.ts	96.55%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5443      +/-   ##
==========================================
+ Coverage   98.81%   98.83%   +0.02%     
==========================================
  Files         238      238              
  Lines        9451     9554     +103     
  Branches     2408     2439      +31     
==========================================
+ Hits         9339     9443     +104     
+ Misses         47       46       -1     
  Partials       65       65              

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

[lukastaegert]

Thank you for your work! This is a really interesting approach. I will give you a more thorough review in the next days as I find time, maybe we can develop this in a slightly different direction.

[lukastaegert]

So, as promised, here is my review. This PR does indeed show quite some ingenuity. It is not free of bugs but that would be fixable. Performance is also ok as your additional pass only scans top-level variables.

What I do not like, though, is that it does not tie well into the existing tree-shaking logic but rather appears to build its separate logic next to it. That also means that it

• needs to make a distinction to work only for top-level variables (which avoids performance issues)
• does not take side effects into account, e.g. here, it could also remove the if(x.y), but it is retained because the property access x.y might have a side effect—which it obviously does not have.
• I am not sure how easy it is to build other features on top of this.

Also, there already IS a mechanism that connects call arguments and functions. Actually, two mechanisms. One is includeCallArguments. This one is responsible for removing unused arguments from function calls like here.
this might be one candidate where one could put additional logic for parameter tracking, i.e. the ParameterScope could tell the ParameterVariables which values were included. Note though that this might be a little late as until the point where the call is included, the function was evaluated without the parameter value, which means it can probably only assume the value to be "unknown", which might provide suboptimal results.

The other mechanism is deoptimizeArgumentsOnInteractionAtPath. This one is triggered very early, basically the first time a CallExpression is scanned for effects (which roughly corresponds to the first time Rollup determines this CallExpression to be in an included code path) or the first time it is included (for unconditionally included code), see CallExpression.applyDeoptimizations. This mechanism tracks mutations that happens in functions to mark call arguments as "deoptimized" if necessary, so that e.g. they do not report wrong literal values. You could use deoptimizeArgumentsOnInteractionAtPath to report possible values to the parameters.

You could then not limit the values to literal values alone, but if you do that, I would make sure not to track long lists of call parameters because then you might run into performance problems. Or again limit yourself to literal values for now. Rollup usually has the logic that if a function is reassigned, the value is assumed to be "unknown" because otherwise, we would often quickly run into exponential performance degradation issues.

There is another case to consider, though. What happens if a function is exported or assigned to a global variable? In that case, we must not make assumption about passed in parameters. Luckily, this case can be easily detected as well.

Would you consider re-implementing the feature in such a direction? One immediate benefit would be that it would not longer be limited to top-level functions and also work e.g. in IIFEs etc.

[liuly0322]

Thank you very much for your detailed explanation! I didn't know much about the internal details of rollup before, you help me a lot.

I have refactored my code. deoptimizeArgumentsOnInteractionAtPath is extremelly useful to collect function call information, and the evaluation step is done in include.

I have updated the logic to fit in the tree-shaking framework. Now the updated version works for old tests, and new tests:

• no removing function exports at top level.
• IIFE

Below is a new problem:

When hasEffects is called, if expression and other expression will cache the condition result. later when I changed the parameter's calculated value in include, the if expression result is still cached, so it won't update. currently I remove the if expression cache, but I think it may not be the best solution. (there are other expressions with cache too)

Also, I am not sure how to remove if(x.y) with no side-effect in your post

Thank you again!

[liuly0322]

I just saw you were considering to bind the parameters lazily. This can be done, but basically you have to do it not only on "include", but when the identifier becomes part of an "executed code path" (that may or may not be included later, depending on its side effects). To do that, you have to do it in two places: On "hasEffects" (which is a reliable indication that a code path is considered to be executed) AND on "include" (for cases where a code path is directly included without any previous checks). Incidentally, this is exactly when applyDeoptimizations is run

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

Line 325 in e64f3d8

protected applyDeoptimizations(): void {

And in a way, adding new call parameters is a potential deoptimization. So you can just extend applyDeoptimizations for Identifier and MemberExpression and place the logic there.

Ah, many thanks, that's exactly what I need! By lazy binding, the ExportDefaultVariable is now free of clearing cache. 😉

Still one problem to fix, the isReassigned update should also trigger cache update. like this example I would like to make isReassigned in base class a getter and setter, or any other ideas? thanks!

[lukastaegert]

Still one problem to fix, the isReassigned update should also trigger cache update. like this example I would like to make isReassigned a getter or setter, or any other ideas? thanks!

That is a very good example, if you did not do so already, maybe we can rework this to a function test, I.e. a test that runs itself to see if the correct logic is executed? Maybe change it like this.

About changing it to accessors, yes, we can do that. However, this affects all Variables and the field is read much more often than it is written, incurring a performance cost. So why not add a method, e.g. markReassigned() that sets the value and which can be overridden. To protect it from unintended writes you can mark it as readonly and use a type cast in the setter, e.g.

markReassigned() {
  (this as {isReassigned: boolean}).isReassigned = true;
}

[liuly0322]

Still one problem to fix, the isReassigned update should also trigger cache update. like this example I would like to make isReassigned a getter or setter, or any other ideas? thanks!

That is a very good example, if you did not do so already, maybe we can rework this to a function test, I.e. a test that runs itself to see if the correct logic is executed? Maybe change it like this.

About changing it to accessors, yes, we can do that. However, this affects all Variables and the field is read much more often than it is written, incurring a performance cost. So why not add a method, e.g. markReassigned() that sets the value and which can be overridden. To protect it from unintended writes you can mark it as readonly and use a type cast in the setter, e.g.

markReassigned() {
  (this as {isReassigned: boolean}).isReassigned = true;
}

As always I learned a lot, really a wonderful experience! The only other place need to be modified is in deoptimizePath if I understand correctly? The test is also added.

[lukastaegert]

This looks good from my side. Is it ok from your side if I release it?

[liuly0322]

This looks good from my side. Is it ok from your side if I release it?

It looks good to me too🎉. Thanks for your review.

[github-actions]

This PR has been released as part of rollup@4.16.0. You can test it via npm install rollup.

[cpojer]

I updated Athena Crisis to the latest version of Vite, and pnpm chose to upgrade Rollup from 4.14.3 to 4.16.1, which comes with this PR. It unfortunately broke my app in production, making it so that react-router could no longer match any routes:

Changing treeshake to false in my Vite config resolves the issue for me, so it is highly likely that this change introduces subtle issues to compiled code. This comment on the react-router repo highlights a concrete bug: remix-run/react-router#11480 (comment)

Let me know if there is anything I can do to help troubleshoot the issue. It might be best to revert this PR, make a new release, and then rework and fix any issues from this change.

[liuly0322]

I'm OK with either first revert or fix ASAP, sorry for the inconvenience.

Using examples from remix-run/react-router#11480 (comment)

By config:

build: {
    minify: false	
}

rollup 4.15 and 4.16.1 has 16 differences.

I'm trying to figure out the reason, but may not be as fast.

EDIT：fixed in #5482

[liuly0322]

REPL

[liaochente]

I updated Athena Crisis to the latest version of Vite, and pnpm chose to upgrade Rollup from 4.14.3 to 4.16.1, which comes with this PR. It unfortunately broke my app in production, making it so that react-router could no longer match any routes:

Changing treeshake to false in my Vite config resolves the issue for me, so it is highly likely that this change introduces subtle issues to compiled code. This comment on the react-router repo highlights a concrete bug: remix-run/react-router#11480 (comment)

Let me know if there is anything I can do to help troubleshoot the issue. It might be best to revert this PR, make a new release, and then rework and fix any issues from this change.

I'm using Vite5, and a temp fix for that issue is:

If you using npm, add to package.json file:

"overrides": {
    "rollup": "4.15.0"
}

If you using pnpm, add to package.json file:

"pnpm": {
    "overrides": {
        "rollup": "4.15.0"
    }
}

[brophdawg11]

👋 We just got a bug filed over in react-router for this too - here's some info I found while debugging if it's useful! It seems like a default param value is being incorrectly transpiled in flattenRoutes and end sup being used every time, and overriding the parameter value passed in: remix-run/remix#9271 (comment)

[liuly0322]

👋 We just got a bug filed over in react-router for this too - here's some info I found while debugging if it's useful! It seems like a default param value is being incorrectly transpiled in flattenRoutes and end sup being used every time, and overriding the parameter value passed in: remix-run/remix#9271 (comment)

Thank you for your work! Your conclusion is right, it should be fixed after #5482 merged.

branches = []; is a polyfill for default parameter syntax, and I missed this situation: once there is an assignment to branches, we shouldn't assume anything about branches.

However, if there isn't such a polyfill, 4.16.1 will behave correctly, also chances are there to optimize these default parameters away (though not in current version of rollup, maybe future work).

[brophdawg11]

Thank you for the quick turnaround!!

[cpojer]

Thank you for the fix. I updated to the latest version of Rollup today, and unfortunately this feature is now causing other production issues in a relatively large app. Because Rollup is only used in production, and these errors appear to be subtle (in my case it isn't even throwing errors, random stuff just silently fails like when clicking buttons), it is extremely hard to debug and I have no choice but to revert and lock rollup to 4.15.0.

Would you be able to add an option separate from treeshake for this feature (like experimental_treeshake?), which is disabled by default, so that we can all move forward with the latest version of Rollup, and in addition help you debug all the issues with this optimization, instead of causing hard to debug production issues for tons of apps?

Here is a user reported issue with a video of the issue: https://discord.com/channels/1070226067845042206/1176528552351502417/1232298199767847003 – the button just does nothing in prod, but works fine in dev.

[lukastaegert]

A revert will be published in the next minutes and we will look at the remaining issues more thoroughly before releasing this feature again.

[Shakeskeyboarde]

A revert will be published in the next minutes and we will look at the remaining issues more thoroughly before releasing this feature again.

Love the feature :). Hope you get it sorted and re-released!