From 30b6e568448b9894584c893c4025522100afecfe Mon Sep 17 00:00:00 2001 From: Tom Jenkinson Date: Sat, 2 Nov 2019 15:08:39 +0000 Subject: [PATCH 1/4] wip handle assignments in dead code --- src/ast/nodes/AssignmentExpression.ts | 31 +++++++++++++------ src/ast/nodes/IfStatement.ts | 43 ++++++++++++++++----------- 2 files changed, 47 insertions(+), 27 deletions(-) diff --git a/src/ast/nodes/AssignmentExpression.ts b/src/ast/nodes/AssignmentExpression.ts index 9daabe0bd36..5303dd90e85 100644 --- a/src/ast/nodes/AssignmentExpression.ts +++ b/src/ast/nodes/AssignmentExpression.ts @@ -1,11 +1,11 @@ import MagicString from 'magic-string'; import { findFirstOccurrenceOutsideComment, RenderOptions } from '../../utils/renderHelpers'; import { getSystemExportStatement } from '../../utils/systemJsRendering'; -import { HasEffectsContext } from '../ExecutionContext'; +import { HasEffectsContext, InclusionContext } from '../ExecutionContext'; import { EMPTY_PATH, ObjectPath, UNKNOWN_PATH } from '../utils/PathTracker'; import Variable from '../variables/Variable'; import * as NodeType from './NodeType'; -import { ExpressionNode, NodeBase } from './shared/Node'; +import { ExpressionNode, IncludeChildren, NodeBase } from './shared/Node'; import { PatternNode } from './shared/Pattern'; export default class AssignmentExpression extends NodeBase { @@ -26,15 +26,10 @@ export default class AssignmentExpression extends NodeBase { | '**='; right!: ExpressionNode; type!: NodeType.tAssignmentExpression; - - bind() { - super.bind(); - this.left.deoptimizePath(EMPTY_PATH); - // We cannot propagate mutations of the new binding to the old binding with certainty - this.right.deoptimizePath(UNKNOWN_PATH); - } + private deoptimized = false; hasEffects(context: HasEffectsContext): boolean { + this.deoptimize(); return ( this.right.hasEffects(context) || this.left.hasEffects(context) || @@ -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); + } + render(code: MagicString, options: RenderOptions) { this.left.render(code, options); this.right.render(code, options); @@ -79,4 +79,17 @@ export default class AssignmentExpression extends NodeBase { } } } + + shouldBeIncluded(context: InclusionContext): boolean { + this.deoptimize(); + return super.shouldBeIncluded(context); + } + + private deoptimize() { + if (!this.deoptimized) { + this.deoptimized = true; + this.left.deoptimizePath(EMPTY_PATH); + this.right.deoptimizePath(UNKNOWN_PATH); + } + } } diff --git a/src/ast/nodes/IfStatement.ts b/src/ast/nodes/IfStatement.ts index 615fb1c3951..a114e538bd7 100644 --- a/src/ast/nodes/IfStatement.ts +++ b/src/ast/nodes/IfStatement.ts @@ -8,27 +8,26 @@ import { LiteralValueOrUnknown, UnknownValue } from '../values'; import * as NodeType from './NodeType'; import { ExpressionNode, IncludeChildren, StatementBase, StatementNode } from './shared/Node'; +const unset = Symbol('unset'); + export default class IfStatement extends StatementBase implements DeoptimizableEntity { alternate!: StatementNode | null; consequent!: StatementNode; test!: ExpressionNode; type!: NodeType.tIfStatement; - private testValue: LiteralValueOrUnknown; - - bind() { - super.bind(); - // ensure the testValue is set for the tree-shaking passes - this.testValue = this.test.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); - } + private testValue: LiteralValueOrUnknown | typeof unset = unset; deoptimizeCache() { this.testValue = UnknownValue; } hasEffects(context: HasEffectsContext): boolean { - if (this.test.hasEffects(context)) return true; - if (this.testValue === UnknownValue) { + if (this.test.hasEffects(context)) { + return true; + } + const testValue = this.getTestValue(); + if (testValue === UnknownValue) { const { brokenFlow } = context; if (this.consequent.hasEffects(context)) return true; const consequentBrokenFlow = context.brokenFlow; @@ -39,16 +38,17 @@ export default class IfStatement extends StatementBase implements DeoptimizableE context.brokenFlow < consequentBrokenFlow ? context.brokenFlow : consequentBrokenFlow; return false; } - return this.testValue + return testValue ? this.consequent.hasEffects(context) : this.alternate !== null && this.alternate.hasEffects(context); } include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) { this.included = true; + const testValue = this.getTestValue(); if (includeChildrenRecursively) { this.includeRecursively(includeChildrenRecursively, context); - } else if (this.testValue === UnknownValue) { + } else if (testValue === UnknownValue) { this.includeUnknownTest(context); } else { this.includeKnownTest(context); @@ -57,13 +57,12 @@ export default class IfStatement extends StatementBase implements DeoptimizableE render(code: MagicString, options: RenderOptions) { // Note that unknown test values are always included + const testValue = this.getTestValue(); if ( !this.test.included && - (this.testValue - ? this.alternate === null || !this.alternate.included - : !this.consequent.included) + (testValue ? this.alternate === null || !this.alternate.included : !this.consequent.included) ) { - const singleRetainedBranch = (this.testValue ? this.consequent : this.alternate)!; + const singleRetainedBranch = (testValue ? this.consequent : this.alternate)!; code.remove(this.start, singleRetainedBranch.start); code.remove(singleRetainedBranch.end, this.end); removeAnnotations(this, code); @@ -72,7 +71,7 @@ export default class IfStatement extends StatementBase implements DeoptimizableE if (this.test.included) { this.test.render(code, options); } else { - code.overwrite(this.test.start, this.test.end, this.testValue ? 'true' : 'false'); + code.overwrite(this.test.start, this.test.end, testValue ? 'true' : 'false'); } if (this.consequent.included) { this.consequent.render(code, options); @@ -89,14 +88,22 @@ export default class IfStatement extends StatementBase implements DeoptimizableE } } + private getTestValue(): LiteralValueOrUnknown { + if (this.testValue === unset) { + this.testValue = this.test.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); + } + return this.testValue; + } + private includeKnownTest(context: InclusionContext) { if (this.test.shouldBeIncluded(context)) { this.test.include(context, false); } - if (this.testValue && this.consequent.shouldBeIncluded(context)) { + const testValue = this.getTestValue(); + if (testValue && this.consequent.shouldBeIncluded(context)) { this.consequent.include(context, false); } - if (this.alternate !== null && !this.testValue && this.alternate.shouldBeIncluded(context)) { + if (this.alternate !== null && !testValue && this.alternate.shouldBeIncluded(context)) { this.alternate.include(context, false); } } From 0725fbe61f44e5a4df286e10ad5c79632e88fadd Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Sun, 15 Dec 2019 07:26:05 +0100 Subject: [PATCH 2/4] Test skipped code does not cause deoptimizations --- .../lazy-assignment-deoptimization/_config.js | 3 +++ .../lazy-assignment-deoptimization/_expected.js | 3 +++ .../samples/lazy-assignment-deoptimization/main.js | 12 ++++++++++++ 3 files changed, 18 insertions(+) create mode 100644 test/form/samples/lazy-assignment-deoptimization/_config.js create mode 100644 test/form/samples/lazy-assignment-deoptimization/_expected.js create mode 100644 test/form/samples/lazy-assignment-deoptimization/main.js diff --git a/test/form/samples/lazy-assignment-deoptimization/_config.js b/test/form/samples/lazy-assignment-deoptimization/_config.js new file mode 100644 index 00000000000..3d2fbfd1923 --- /dev/null +++ b/test/form/samples/lazy-assignment-deoptimization/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'only deoptimizes assigned variables when the assignment is included' +}; diff --git a/test/form/samples/lazy-assignment-deoptimization/_expected.js b/test/form/samples/lazy-assignment-deoptimization/_expected.js new file mode 100644 index 00000000000..724e99a15d2 --- /dev/null +++ b/test/form/samples/lazy-assignment-deoptimization/_expected.js @@ -0,0 +1,3 @@ +console.log('retained'); + +console.log('retained'); diff --git a/test/form/samples/lazy-assignment-deoptimization/main.js b/test/form/samples/lazy-assignment-deoptimization/main.js new file mode 100644 index 00000000000..43da5d447fd --- /dev/null +++ b/test/form/samples/lazy-assignment-deoptimization/main.js @@ -0,0 +1,12 @@ +const foo = { toggled: false }; +const bar = { toggled: false }; + +if (foo.toggled) { + foo.toggled = bar; +} + +if (foo.toggled) console.log('this should be removed'); +else console.log('retained'); + +if (bar.toggled) console.log('this should be removed'); +else console.log('retained'); From f6ea7408040bed478a00bdc902daccf82ead82da Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Mon, 16 Dec 2019 06:09:11 +0100 Subject: [PATCH 3/4] Add tests and slightly adapt logic --- src/ast/nodes/AssignmentExpression.ts | 25 ++++++++----------- .../assignee-is-deoptimized/_config.js | 3 +++ .../assignee-is-deoptimized/main.js | 5 ++++ .../_config.js | 3 +++ .../assignment-target-is-deoptimized/main.js | 3 +++ .../nested-assignment/_config.js | 3 +++ .../nested-assignment/main.js | 3 +++ .../try-catch-inclusion/_config.js | 3 +++ .../try-catch-inclusion/main.js | 6 +++++ 9 files changed, 39 insertions(+), 15 deletions(-) create mode 100644 test/function/samples/assignment-deoptimization/assignee-is-deoptimized/_config.js create mode 100644 test/function/samples/assignment-deoptimization/assignee-is-deoptimized/main.js create mode 100644 test/function/samples/assignment-deoptimization/assignment-target-is-deoptimized/_config.js create mode 100644 test/function/samples/assignment-deoptimization/assignment-target-is-deoptimized/main.js create mode 100644 test/function/samples/assignment-deoptimization/nested-assignment/_config.js create mode 100644 test/function/samples/assignment-deoptimization/nested-assignment/main.js create mode 100644 test/function/samples/assignment-deoptimization/try-catch-inclusion/_config.js create mode 100644 test/function/samples/assignment-deoptimization/try-catch-inclusion/main.js diff --git a/src/ast/nodes/AssignmentExpression.ts b/src/ast/nodes/AssignmentExpression.ts index 5303dd90e85..8dd9796cab8 100644 --- a/src/ast/nodes/AssignmentExpression.ts +++ b/src/ast/nodes/AssignmentExpression.ts @@ -29,7 +29,7 @@ export default class AssignmentExpression extends NodeBase { private deoptimized = false; hasEffects(context: HasEffectsContext): boolean { - this.deoptimize(); + if (!this.deoptimized) this.applyDeoptimizations(); return ( this.right.hasEffects(context) || this.left.hasEffects(context) || @@ -41,9 +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); + include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { + if (!this.deoptimized) this.applyDeoptimizations(); + this.included = true; + this.left.include(context, includeChildrenRecursively); + this.right.include(context, includeChildrenRecursively); } render(code: MagicString, options: RenderOptions) { @@ -80,16 +82,9 @@ export default class AssignmentExpression extends NodeBase { } } - shouldBeIncluded(context: InclusionContext): boolean { - this.deoptimize(); - return super.shouldBeIncluded(context); - } - - private deoptimize() { - if (!this.deoptimized) { - this.deoptimized = true; - this.left.deoptimizePath(EMPTY_PATH); - this.right.deoptimizePath(UNKNOWN_PATH); - } + private applyDeoptimizations() { + this.deoptimized = true; + this.left.deoptimizePath(EMPTY_PATH); + this.right.deoptimizePath(UNKNOWN_PATH); } } diff --git a/test/function/samples/assignment-deoptimization/assignee-is-deoptimized/_config.js b/test/function/samples/assignment-deoptimization/assignee-is-deoptimized/_config.js new file mode 100644 index 00000000000..25bb08a0f03 --- /dev/null +++ b/test/function/samples/assignment-deoptimization/assignee-is-deoptimized/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'makes sure the assignee is deoptimized' +}; diff --git a/test/function/samples/assignment-deoptimization/assignee-is-deoptimized/main.js b/test/function/samples/assignment-deoptimization/assignee-is-deoptimized/main.js new file mode 100644 index 00000000000..fc5921b2db5 --- /dev/null +++ b/test/function/samples/assignment-deoptimization/assignee-is-deoptimized/main.js @@ -0,0 +1,5 @@ +const flags = { updated: false }; +let toBeUpdated = {}; +toBeUpdated = flags; +toBeUpdated.updated = true; +if (!flags.updated) throw new Error('Update was not tracked'); diff --git a/test/function/samples/assignment-deoptimization/assignment-target-is-deoptimized/_config.js b/test/function/samples/assignment-deoptimization/assignment-target-is-deoptimized/_config.js new file mode 100644 index 00000000000..cd94f45ae26 --- /dev/null +++ b/test/function/samples/assignment-deoptimization/assignment-target-is-deoptimized/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'makes sure the assignment target is deoptimized' +}; diff --git a/test/function/samples/assignment-deoptimization/assignment-target-is-deoptimized/main.js b/test/function/samples/assignment-deoptimization/assignment-target-is-deoptimized/main.js new file mode 100644 index 00000000000..f7112f60773 --- /dev/null +++ b/test/function/samples/assignment-deoptimization/assignment-target-is-deoptimized/main.js @@ -0,0 +1,3 @@ +let updated = false; +updated = true; +if (!updated) throw new Error('Update was not tracked'); diff --git a/test/function/samples/assignment-deoptimization/nested-assignment/_config.js b/test/function/samples/assignment-deoptimization/nested-assignment/_config.js new file mode 100644 index 00000000000..7c3114ddadc --- /dev/null +++ b/test/function/samples/assignment-deoptimization/nested-assignment/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'tracks assigments nested in expressions that are included for other reasons' +}; diff --git a/test/function/samples/assignment-deoptimization/nested-assignment/main.js b/test/function/samples/assignment-deoptimization/nested-assignment/main.js new file mode 100644 index 00000000000..57e5d4fc85c --- /dev/null +++ b/test/function/samples/assignment-deoptimization/nested-assignment/main.js @@ -0,0 +1,3 @@ +let updated = false; +assert.ok(!updated) || (updated = true); +if (!updated) throw new Error('Update was not tracked'); diff --git a/test/function/samples/assignment-deoptimization/try-catch-inclusion/_config.js b/test/function/samples/assignment-deoptimization/try-catch-inclusion/_config.js new file mode 100644 index 00000000000..4a7b55c699a --- /dev/null +++ b/test/function/samples/assignment-deoptimization/try-catch-inclusion/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'tracks assigments included via try-catch-deoptimization' +}; diff --git a/test/function/samples/assignment-deoptimization/try-catch-inclusion/main.js b/test/function/samples/assignment-deoptimization/try-catch-inclusion/main.js new file mode 100644 index 00000000000..1a34c1d59b0 --- /dev/null +++ b/test/function/samples/assignment-deoptimization/try-catch-inclusion/main.js @@ -0,0 +1,6 @@ +let updated = false; +try { + updated = true; +} catch (err) {} + +if (!updated) throw new Error('Update was not tracked'); From 90b242d1df56f5bf1297a155cbc15a34f3c383d8 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Mon, 16 Dec 2019 06:24:28 +0100 Subject: [PATCH 4/4] Some mini-optimizations --- src/ast/nodes/IfStatement.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/ast/nodes/IfStatement.ts b/src/ast/nodes/IfStatement.ts index a114e538bd7..ad629bf667b 100644 --- a/src/ast/nodes/IfStatement.ts +++ b/src/ast/nodes/IfStatement.ts @@ -45,13 +45,15 @@ export default class IfStatement extends StatementBase implements DeoptimizableE include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) { this.included = true; - const testValue = this.getTestValue(); if (includeChildrenRecursively) { this.includeRecursively(includeChildrenRecursively, context); - } else if (testValue === UnknownValue) { - this.includeUnknownTest(context); } else { - this.includeKnownTest(context); + const testValue = this.getTestValue(); + if (testValue === UnknownValue) { + this.includeUnknownTest(context); + } else { + this.includeKnownTest(context, testValue); + } } } @@ -90,16 +92,19 @@ export default class IfStatement extends StatementBase implements DeoptimizableE private getTestValue(): LiteralValueOrUnknown { if (this.testValue === unset) { - this.testValue = this.test.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); + return (this.testValue = this.test.getLiteralValueAtPath( + EMPTY_PATH, + SHARED_RECURSION_TRACKER, + this + )); } return this.testValue; } - private includeKnownTest(context: InclusionContext) { + private includeKnownTest(context: InclusionContext, testValue: LiteralValueOrUnknown) { if (this.test.shouldBeIncluded(context)) { this.test.include(context, false); } - const testValue = this.getTestValue(); if (testValue && this.consequent.shouldBeIncluded(context)) { this.consequent.include(context, false); }