[WIP] handle assignments in dead code #3212

tjenkinson · 2019-11-02T15:10:06Z

ref #3200

This PR contains:

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

lukastaegert

Just some comments. I know the deoptimization logic is slightly complicated, but it is actually quite efficient. I would not change IfStatement. The initial testValue retrieved during bind will depend on the initial values of the declared variables, but will then be deoptimized automatically later if assignments are added.

lukastaegert · 2019-11-04T07:23:20Z

src/ast/nodes/AssignmentExpression.ts

@@ -46,6 +41,11 @@ export default class AssignmentExpression extends NodeBase {
 		return path.length > 0 && this.right.hasEffectsWhenAccessedAtPath(path, context);
 	}

+	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
+		this.deoptimize();
+		return super.include(context, includeChildrenRecursively);


This could actually be inlined to become this.included = true; this.left.include(context, includeChildrenRecursively); this.right.include(context, includeChildrenRecursively); to save one extra function call.

lukastaegert · 2019-11-04T07:26:22Z

src/ast/nodes/AssignmentExpression.ts

@@ -69,4 +69,17 @@ export default class AssignmentExpression extends NodeBase {
 			}
 		}
 	}
+
+	shouldBeIncluded(context: InclusionContext): boolean {
+		this.deoptimize();


Actually on second thought I think there is no reason to do this here: This will only be called for statements (which this one is not) and in some edge cases. Unless we are in a try statement or for declarations, hasEffects will be called at least once if the expression is included. So shouldBeIncluded can be removed.

lukastaegert · 2019-11-04T07:31:31Z

src/ast/nodes/AssignmentExpression.ts

@@ -46,6 +41,11 @@ export default class AssignmentExpression extends NodeBase {
 		return path.length > 0 && this.right.hasEffectsWhenAccessedAtPath(path, context);
 	}

+	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
+		this.deoptimize();


By now, everything I told you about this being necessary was guess work. Would be nice to first have a test for the situation where an assignment is included without a hasEffects check first and only THEN I would add this here. This is all really performance critical code so there should be a tested justification for adding additional checks.

Maybe something like this (but please check if the test is failing without the above deoptimization):

let x = false; let reassigned = false; try { x = true; } catch (err) {} if (x) { reassigned = true; } assert.ok(reassigned);

lukastaegert · 2019-11-04T08:01:02Z

src/ast/nodes/IfStatement.ts

@@ -90,14 +88,22 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
 		}
 	}

+	private getTestValue(): LiteralValueOrUnknown {


The changes in this file make sense but I do not think they actually solve any issue; on the other hand, they add a lot of additional function calls in various places. Usually, the deoptimization logic should fix it if the test value is later reassigned. The reason why it was not working is because there is a bug in MemberExpression:
deoptimizeCache

rollup/src/ast/nodes/MemberExpression.ts

Lines 114 to 118 in f9b9a7c

deoptimizeCache() {

for (const expression of this.expressionsToBeDeoptimized) {

expression.deoptimizeCache();

}

}

should not only deoptimize all dependent expressionsToBeDeoptimized but also set this.propertyKey = UnknownKey. With that change, I can revert all changes in IfStatement and still have all tests green.

So how do DeoptimizeableEntitys work? This is a performance optimization that allows nodes to cache values (such as this.testValue) reliably while still making sure the value is reset if one of the entities needed to get the cached value is reassigned. To do that, getLiteralValueAtPath receives the calling Node as parameter. Then when the value that was initially returned is deoptimized, deoptimizeCache is called on the caller to make it "forget" the cached value. This was missing for MemberExpression. A similar logic is in place for caching function return values.

MemberExpression is fixed now and I think the changes are ok as I think moving logic away from bind is a good thing.

lukastaegert · 2019-12-16T05:12:05Z

As I really want this merged because I would love to do some larger refactorings that would include changing some names, I added some tests and slightly adapted the logic. Please have a look!

codecov · 2019-12-16T05:13:30Z

Codecov Report

Merging #3212 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3212      +/-   ##
==========================================
- Coverage   93.17%   93.13%   -0.04%     
==========================================
  Files         172      172              
  Lines        6049     6062      +13     
  Branches     1806     1809       +3     
==========================================
+ Hits         5636     5646      +10     
- Misses        219      221       +2     
- Partials      194      195       +1

Impacted Files	Coverage Δ
src/ast/nodes/IfStatement.ts	`100% <100%> (ø)`	⬆️
src/ast/nodes/AssignmentExpression.ts	`96.29% <100%> (+1.05%)`	⬆️
src/ast/nodes/CallExpression.ts	`95.41% <0%> (-2.76%)`	⬇️

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 61b3129...90b242d. Read the comment docs.

lukastaegert · 2019-12-16T05:13:37Z

src/ast/nodes/AssignmentExpression.ts


 	hasEffects(context: HasEffectsContext): boolean {
+		if (!this.deoptimized) this.applyDeoptimizations();


I extracted the check because hasEffects and include are called quite often and the deoptimizations will only be necessary once. This would save the cost of one function call, but the wins are probably small so we could also move the check back if you prefer.

lukastaegert · 2019-12-16T05:14:30Z

src/ast/nodes/AssignmentExpression.ts

+		this.included = true;
+		this.left.include(context, includeChildrenRecursively);
+		this.right.include(context, includeChildrenRecursively);
+	}


Similar reasons, we could also call super.include, but this is all it would do, though Node.include is slightly more complicated and generic, so this is more efficient.

lukastaegert · 2019-12-16T05:28:24Z

src/ast/nodes/IfStatement.ts

@@ -90,14 +88,22 @@ export default class IfStatement extends StatementBase implements DeoptimizableE
 		}
 	}

+	private getTestValue(): LiteralValueOrUnknown {


MemberExpression is fixed now and I think the changes are ok as I think moving logic away from bind is a good thing.

tjenkinson · 2020-01-08T07:36:05Z

@lukastaegert thanks for finishing this off! I hadn’t realised you pushed some commits a while back

lukastaegert · 2020-01-08T09:23:24Z

Sure, I hope it was ok. At the time, I had high hopes of pushing some more stuff based on these refactorings but Christmas and Life got in the way 😜

tjenkinson mentioned this pull request Nov 2, 2019

Multiple passes on tree shaking #3200

Closed

lukastaegert reviewed Nov 4, 2019

View reviewed changes

lukastaegert reviewed Dec 16, 2019

View reviewed changes

lukastaegert marked this pull request as ready for review January 4, 2020 19:38

tjenkinson and others added 4 commits January 4, 2020 20:47

wip handle assignments in dead code

30b6e56

Test skipped code does not cause deoptimizations

0725fbe

Add tests and slightly adapt logic

f6ea740

Some mini-optimizations

90b242d

lukastaegert force-pushed the tree-shake-handle-assignments-in-dead-code branch from 13229e0 to 90b242d Compare January 4, 2020 19:48

lukastaegert merged commit 6106af1 into rollup:master Jan 4, 2020

snyk-bot mentioned this pull request Jan 17, 2020

[Snyk] Upgrade rollup from 1.28.0 to 1.29.0 Tootoot222/react-typescript-hook-mouse#8

Merged

This was referenced Feb 8, 2020

[Snyk] Upgrade rollup from 1.27.14 to 1.29.0 jamesleesaunders/d3-x3d#142

Merged

[Snyk] Upgrade rollup from 1.27.14 to 1.29.0 shlomif/fc-solve#28

Closed

This was referenced Mar 7, 2020

[Snyk] Upgrade rollup from 0.63.5 to 1.32.0 tjenkinson/DefinitelyTyped#4

Open

[Snyk] Upgrade rollup from 1.27.2 to 1.32.0 tjenkinson/rollup-bug-empty-dymanic-import#2

Open

[Snyk] Upgrade rollup from 0.57.1 to 1.32.0 tjenkinson/www.jsdelivr.com#3

Open

snyk-bot mentioned this pull request Mar 16, 2020

[Snyk] Upgrade rollup from 0.62.0 to 1.31.1 luke7oaks/node-1#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] handle assignments in dead code #3212

[WIP] handle assignments in dead code #3212

tjenkinson commented Nov 2, 2019 •

edited

Loading

lukastaegert left a comment

lukastaegert Nov 4, 2019

lukastaegert Nov 4, 2019

lukastaegert Nov 4, 2019

lukastaegert Nov 4, 2019

lukastaegert Dec 16, 2019

lukastaegert commented Dec 16, 2019

codecov bot commented Dec 16, 2019 •

edited

Loading

lukastaegert Dec 16, 2019

lukastaegert Dec 16, 2019

lukastaegert Dec 16, 2019

tjenkinson commented Jan 8, 2020

lukastaegert commented Jan 8, 2020

	deoptimizeCache() {
	for (const expression of this.expressionsToBeDeoptimized) {
	expression.deoptimizeCache();
	}
	}


		hasEffects(context: HasEffectsContext): boolean {
		if (!this.deoptimized) this.applyDeoptimizations();

[WIP] handle assignments in dead code #3212

[WIP] handle assignments in dead code #3212

Conversation

tjenkinson commented Nov 2, 2019 • edited Loading

Description

lukastaegert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert commented Dec 16, 2019

codecov bot commented Dec 16, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjenkinson commented Jan 8, 2020

lukastaegert commented Jan 8, 2020

tjenkinson commented Nov 2, 2019 •

edited

Loading

codecov bot commented Dec 16, 2019 •

edited

Loading