Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] handle assignments in dead code #3212

Conversation

tjenkinson
Copy link
Member

@tjenkinson tjenkinson commented Nov 2, 2019

ref #3200

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

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -69,4 +69,17 @@ export default class AssignmentExpression extends NodeBase {
}
}
}

shouldBeIncluded(context: InclusionContext): boolean {
this.deoptimize();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

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

private getTestValue(): LiteralValueOrUnknown {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lukastaegert
Copy link
Member

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
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

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

Impacted file tree graph

@@            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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.included = true;
this.left.include(context, includeChildrenRecursively);
this.right.include(context, includeChildrenRecursively);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

private getTestValue(): LiteralValueOrUnknown {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lukastaegert lukastaegert marked this pull request as ready for review January 4, 2020 19:38
@lukastaegert lukastaegert force-pushed the tree-shake-handle-assignments-in-dead-code branch from 13229e0 to 90b242d Compare January 4, 2020 19:48
@lukastaegert lukastaegert merged commit 6106af1 into rollup:master Jan 4, 2020
@tjenkinson
Copy link
Member Author

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

@lukastaegert
Copy link
Member

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 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants