Disable pessimistic object deoptimization for calls when the called function doesn't ref this #4011

marijnh · 2021-03-24T16:33:12Z

Adds a referencesThis flag to FunctionNode and uses it to avoid the deoptimization around method calls, as discussed in #3989

Instead of new tests, this reverts some of the test changes made by 3e0aa46.

Let me know if the FunctionNode.createScope kludge is within the limits of kludginess accepted in this project. We could also do a conditional initialization of the property in initialize, but that'll lead to different object shapes for no real good reason.

This PR contains:

Are tests included?

kinda
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

List any relevant issue numbers: #3989

…unction doesn't ref this Issue rollup#3989

codecov · 2021-03-24T16:39:08Z

Codecov Report

Merging #4011 (8ad9bd7) into master (2368d9c) will increase coverage by 0.22%.
The diff coverage is 80.43%.

@@            Coverage Diff             @@
##           master    #4011      +/-   ##
==========================================
+ Coverage   97.21%   97.44%   +0.22%     
==========================================
  Files         192      192              
  Lines        6762     6762              
  Branches     1973     1981       +8     
==========================================
+ Hits         6574     6589      +15     
+ Misses        101       85      -16     
- Partials       87       88       +1

Impacted Files	Coverage Δ
src/ast/nodes/Identifier.ts	`98.11% <0.00%> (-1.89%)`	⬇️
src/ast/nodes/shared/MultiExpression.ts	`95.00% <0.00%> (-5.00%)`	⬇️
src/ast/variables/LocalVariable.ts	`88.29% <33.33%> (-1.82%)`	⬇️
src/ast/values.ts	`71.84% <79.16%> (+8.94%)`	⬆️
src/ast/nodes/ArrowFunctionExpression.ts	`100.00% <100.00%> (ø)`
src/ast/nodes/CallExpression.ts	`95.45% <100.00%> (ø)`
src/ast/nodes/MemberExpression.ts	`99.18% <100.00%> (+0.02%)`	⬆️
src/ast/nodes/ObjectExpression.ts	`94.44% <100.00%> (+1.58%)`	⬆️
src/ast/nodes/ThisExpression.ts	`100.00% <100.00%> (ø)`
src/ast/nodes/shared/FunctionNode.ts	`100.00% <100.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2368d9c...8ad9bd7. Read the comment docs.

marijnh · 2021-03-24T16:42:57Z

I'm frankly not terribly interested in writing a bunch of extra tests that make sure that all the () => true functions added in src/ast/values.ts do indeed return true, but if the process is so rigid that that's the only way to move this forward, please point me towards what such tests should look like (i.e. how does one produce these special values?)

lukastaegert · 2021-03-25T06:17:41Z

I would not focus on the values first but rather the loss in coverage for Identifier.ts, LocalVariable.ts and MultiExpression.ts. But I know some of those are tricky to test. I will have a look later.

marijnh · 2021-03-25T08:40:23Z

For me the details of the code coverage report look like those changes are covered (and they should be, since they will be exercised by just about every method call). I was thinking a refactor of values.ts might be a good idea anyway, to avoid duplication (I had to add identical copies of the new method in eight different places), and that might magically solve the code coverage thing. I'll give that a shot later.

lukastaegert · 2021-03-25T09:47:16Z

You are right, it is just some sub-cases which are not covered and it might be ok to skip them (though the ones in LocalVariable DO cover relevant situations, at least the "reassigned" and "no init" cases. The one in Identifier is likely impossible to trigger and one could actually use a TypeScript ! and get rid of it. But MultiExpression is just not covered, but I know it is a tricky one. It is used for the return value when calling a conditional. From my point of view, it would also be ok to use the default implementation for those.)

A refactoring of values.ts is definitely a good thing. Those are mainly used for return values of builtin prototype functions, e.g. .toString().

lukastaegert · 2021-03-25T09:51:53Z

And thank you for bearing with me a little here. I know the tests are usually a real pain, but it gives us the ability to do fundamental refactorings of the code base with a high level of safety, and this has happened before and will happen again.

To avoid duplicating all ExpressionEntity methods for every object.

marijnh · 2021-03-25T11:04:11Z

Attached patch does the refactor, but code coverage bot still isn't quite happy. Let me know if you want me to test those toString methods or create a weird test that tries to call a literal.

lukastaegert

Looks good! I will see if I can add a test for multi-expression. I like what you did with the values. Two very minor suggestions remaining, but in general this looks sound to me.

lukastaegert · 2021-03-26T05:17:59Z

src/ast/nodes/ThisExpression.ts

@@ -38,6 +39,12 @@ export default class ThisExpression extends NodeBase {
 				this.start
 			);
 		}
+		for (let parent = this.parent; parent instanceof NodeBase; parent = parent.parent) {


Nice, I like this use of a for loop.

lukastaegert · 2021-03-26T05:31:44Z

src/ast/nodes/shared/FunctionNode.ts

 	scope!: FunctionScope;

 	private isPrototypeDeoptimized = false;

 	createScope(parentScope: FunctionScope) {
 		this.scope = new FunctionScope(parentScope, this.context);
+		// Initialized here because child nodes will update it before
+		// `this.initialize` even runs.
+		this.referencesThis = false;


Ah I see. Maybe move it rather into parseNode, which is where "initialisation before initialise" usually happens. I.e. parseNode is guaranteed to run before child nodes are even created.

lukastaegert · 2021-03-26T05:41:25Z

src/ast/values.ts

-	toString: () => '[[UNKNOWN STRING]]'
+		includeAll(context, args);
+	}
+	toString() { return '[[UNKNOWN STRING]]' }


Actually these "toString" are only here for debugging and I do not think they turned out to be any useful. We should just remove them all.

lukastaegert

We should definitely include arrow functions as well! Those do not derive from FunctionNode.

lukastaegert · 2021-03-26T05:59:47Z

As I am on the code if you do not mind I would push the changes myself.

lukastaegert

If CI passes, this looks good to be merged from my side

lukastaegert · 2021-03-26T06:22:22Z

Fun fact: Looking at the tests there are a lot of variables that are called removedX. Which are now removed again!

marijnh · 2021-03-26T06:22:23Z

We should definitely include arrow functions as well! Those do not derive from FunctionNode.

Good point, thanks for catching that.

marijnh · 2021-03-26T08:27:36Z

Coverage is unhappy again. How do we proceed here? Should I add a test with some expressions like 1(); (x() ? "hi" : true)(); to get around that? If so, what should I call such a test?

lukastaegert · 2021-03-26T09:18:21Z

Na, let's leave it.

…unction doesn't ref this (#4011) * Disable pessimistic object deoptimization for calls when the called function doesn't ref this Issue #3989 * Use a base class for special-purpose value objects To avoid duplicating all ExpressionEntity methods for every object. * Remove toString for values * Move function Node initialization to parseNode * Also handle arrow functions Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com> Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>

Disable pessimistic object deoptimization for calls when the called f…

b0eb271

…unction doesn't ref this Issue rollup#3989

Use a base class for special-purpose value objects

41b3453

To avoid duplicating all ExpressionEntity methods for every object.

marijnh mentioned this pull request Mar 25, 2021

Assignments to side-effecting getters in class prototypes can be incorrectly tree-shaken #4016

Closed

lukastaegert reviewed Mar 26, 2021

View reviewed changes

lukastaegert requested changes Mar 26, 2021

View reviewed changes

lukastaegert and others added 4 commits March 26, 2021 07:01

Remove toString for values

e9cb664

Move function Node initialization to parseNode

12792eb

Also handle arrow functions

4214087

Merge branch 'master' into master

8ad9bd7

lukastaegert approved these changes Mar 26, 2021

View reviewed changes

marijnh mentioned this pull request Mar 26, 2021

Class method effects #4018

Merged

9 tasks

lukastaegert merged commit d8c6839 into rollup:master Mar 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable pessimistic object deoptimization for calls when the called function doesn't ref this #4011

Disable pessimistic object deoptimization for calls when the called function doesn't ref this #4011

marijnh commented Mar 24, 2021

codecov bot commented Mar 24, 2021 •

edited

marijnh commented Mar 24, 2021

lukastaegert commented Mar 25, 2021

marijnh commented Mar 25, 2021

lukastaegert commented Mar 25, 2021 •

edited

lukastaegert commented Mar 25, 2021 •

edited

marijnh commented Mar 25, 2021 •

edited

lukastaegert left a comment

lukastaegert Mar 26, 2021

lukastaegert Mar 26, 2021

lukastaegert Mar 26, 2021

lukastaegert left a comment

lukastaegert commented Mar 26, 2021 •

edited

lukastaegert left a comment

lukastaegert commented Mar 26, 2021

marijnh commented Mar 26, 2021

marijnh commented Mar 26, 2021

lukastaegert commented Mar 26, 2021

Disable pessimistic object deoptimization for calls when the called function doesn't ref this #4011

Disable pessimistic object deoptimization for calls when the called function doesn't ref this #4011

Conversation

marijnh commented Mar 24, 2021

codecov bot commented Mar 24, 2021 • edited

Codecov Report

marijnh commented Mar 24, 2021

lukastaegert commented Mar 25, 2021

marijnh commented Mar 25, 2021

lukastaegert commented Mar 25, 2021 • edited

lukastaegert commented Mar 25, 2021 • edited

marijnh commented Mar 25, 2021 • edited

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert Mar 26, 2021

Choose a reason for hiding this comment

lukastaegert Mar 26, 2021

Choose a reason for hiding this comment

lukastaegert Mar 26, 2021

Choose a reason for hiding this comment

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert commented Mar 26, 2021 • edited

lukastaegert left a comment

Choose a reason for hiding this comment

lukastaegert commented Mar 26, 2021

marijnh commented Mar 26, 2021

marijnh commented Mar 26, 2021

lukastaegert commented Mar 26, 2021

codecov bot commented Mar 24, 2021 •

edited

lukastaegert commented Mar 25, 2021 •

edited

lukastaegert commented Mar 25, 2021 •

edited

marijnh commented Mar 25, 2021 •

edited

lukastaegert commented Mar 26, 2021 •

edited