From 42caeeef219214a6af7c9ed91b4ff874b7c2cccd Mon Sep 17 00:00:00 2001 From: Miles Malerba Date: Sun, 16 Jul 2023 09:40:30 -0700 Subject: [PATCH] refactor(compiler): reuse temp vars when possible (#51100) Updates the template pipeline's temporary variables phase to reuse temporary variables within an expression. The algorithm implemented here reuses variables more aggressively than TemplateDefinitionBuilder. This change in behavior is acceptable, as it is unlikely to cause any failures, and implementing the exact behavior observed in TemplateDefinitionBuilder would be difficult. PR Close #51100 --- .../nullish_coalescing/TEST_CASES.json | 18 ++---- .../safe_access/TEST_CASES.json | 33 +++------- ...fe_access_temporaries_template.pipeline.js | 18 ++++++ packages/compiler/src/output/output_ast.ts | 2 +- .../template/pipeline/ir/src/expression.ts | 2 +- .../src/phases/temporary_variables.ts | 61 ++++++++++++------- 6 files changed, 75 insertions(+), 59 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/safe_access_temporaries_template.pipeline.js diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json index 23a03209de6..3bfadccf874 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/nullish_coalescing/TEST_CASES.json @@ -3,9 +3,7 @@ "cases": [ { "description": "should handle nullish coalescing inside interpolations", - "inputFiles": [ - "nullish_coalescing_interpolation.ts" - ], + "inputFiles": ["nullish_coalescing_interpolation.ts"], "expectations": [ { "files": [ @@ -16,14 +14,11 @@ ], "failureMessage": "Incorrect template" } - ], - "skipForTemplatePipeline": true + ] }, { "description": "should handle nullish coalescing inside property bindings", - "inputFiles": [ - "nullish_coalescing_property.ts" - ], + "inputFiles": ["nullish_coalescing_property.ts"], "expectations": [ { "files": [ @@ -34,14 +29,11 @@ ], "failureMessage": "Incorrect template" } - ], - "skipForTemplatePipeline": true + ] }, { "description": "should handle nullish coalescing inside host bindings", - "inputFiles": [ - "nullish_coalescing_host.ts" - ], + "inputFiles": ["nullish_coalescing_host.ts"], "expectations": [ { "files": [ diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/TEST_CASES.json index 9b37a8e75cd..9d89d267308 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/TEST_CASES.json @@ -3,9 +3,7 @@ "cases": [ { "description": "should handle safe keyed reads inside templates", - "inputFiles": [ - "safe_keyed_read.ts" - ], + "inputFiles": ["safe_keyed_read.ts"], "expectations": [ { "files": [ @@ -20,9 +18,7 @@ }, { "description": "should handle deep safe property reads inside templates", - "inputFiles": [ - "safe_access_deep.ts" - ], + "inputFiles": ["safe_access_deep.ts"], "expectations": [ { "files": [ @@ -30,21 +26,19 @@ "expected": "safe_access_deep_template.js", "generated": "safe_access_deep.js" } - ], - "failureMessage": "Incorrect template" + ] } ] }, { "description": "should handle property reads requiring temporary variables", - "inputFiles": [ - "safe_access_temporaries.ts" - ], + "inputFiles": ["safe_access_temporaries.ts"], "expectations": [ { "files": [ { "expected": "safe_access_temporaries_template.js", + "templatePipelineExpected": "safe_access_temporaries_template.pipeline.js", "generated": "safe_access_temporaries.js" } ], @@ -54,9 +48,7 @@ }, { "description": "should handle safe method calls inside templates", - "inputFiles": [ - "safe_method_call.ts" - ], + "inputFiles": ["safe_method_call.ts"], "expectations": [ { "files": [ @@ -67,14 +59,11 @@ ], "failureMessage": "Incorrect template" } - ], - "skipForTemplatePipeline": true + ] }, { "description": "should handle safe calls inside templates", - "inputFiles": [ - "safe_call.ts" - ], + "inputFiles": ["safe_call.ts"], "expectations": [ { "files": [ @@ -90,9 +79,7 @@ }, { "description": "should handle non-null assertions after a safe access", - "inputFiles": [ - "safe_access_non_null.ts" - ], + "inputFiles": ["safe_access_non_null.ts"], "expectations": [ { "files": [ @@ -107,4 +94,4 @@ "skipForTemplatePipeline": true } ] -} \ No newline at end of file +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/safe_access_temporaries_template.pipeline.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/safe_access_temporaries_template.pipeline.js new file mode 100644 index 00000000000..bc0509c3737 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/safe_access_temporaries_template.pipeline.js @@ -0,0 +1,18 @@ +} if (rf & 2) { + let $tmp_0_0$; + let $tmp_1_0$; + let $tmp_2_0$; + let $tmp_2_1$; + let $tmp_3_0$; + let $tmp_3_1$; + let $tmp_3_2$; + let $tmp_3_3$; + i0.ɵɵadvance(1); + i0.ɵɵtextInterpolate1("Safe Propery with Calls: ", ($tmp_0_0$ = ctx.p()) == null ? null : ($tmp_0_0$ = $tmp_0_0$.a()) == null ? null : ($tmp_0_0$ = $tmp_0_0$.b()) == null ? null : ($tmp_0_0$ = $tmp_0_0$.c()) == null ? null : $tmp_0_0$.d(), ""); + i0.ɵɵadvance(2); + i0.ɵɵtextInterpolate1("Safe and Unsafe Property with Calls: ", ctx.p == null ? null : ($tmp_1_0$ = ctx.p.a()) == null ? null : ($tmp_1_0$ = $tmp_1_0$.b().c().d()) == null ? null : ($tmp_1_0$ = $tmp_1_0$.e()) == null ? null : $tmp_1_0$.f == null ? null : $tmp_1_0$.f.g.h == null ? null : ($tmp_1_0$ = $tmp_1_0$.f.g.h.i()) == null ? null : ($tmp_1_0$ = $tmp_1_0$.j()) == null ? null : $tmp_1_0$.k().l, ""); + i0.ɵɵadvance(2); + i0.ɵɵtextInterpolate1("Nested Safe with Calls: ", ($tmp_2_0$ = ctx.f1()) == null ? null : $tmp_2_0$[($tmp_2_1$ = ctx.f2()) == null ? null : $tmp_2_1$.a] == null ? null : $tmp_2_0$[($tmp_2_1$ = $tmp_2_1$) == null ? null : $tmp_2_1$.a].b, ""); + i0.ɵɵadvance(2); + i0.ɵɵtextInterpolate1("Deep Nested Safe with Calls: ", ($tmp_3_0$ = ctx.f1()) == null ? null : $tmp_3_0$[($tmp_3_1$ = ctx.f2()) == null ? null : ($tmp_3_2$ = $tmp_3_1$.f3()) == null ? null : $tmp_3_2$[($tmp_3_3$ = ctx.f4()) == null ? null : $tmp_3_3$.f5()]] == null ? null : $tmp_3_0$[($tmp_3_1$ = $tmp_3_1$) == null ? null : ($tmp_3_2$ = $tmp_3_2$) == null ? null : $tmp_3_2$[($tmp_3_3$ = $tmp_3_3$) == null ? null : $tmp_3_3$.f5()]].f6(), ""); +} diff --git a/packages/compiler/src/output/output_ast.ts b/packages/compiler/src/output/output_ast.ts index 92df21bc619..a8c7b4b578b 100644 --- a/packages/compiler/src/output/output_ast.ts +++ b/packages/compiler/src/output/output_ast.ts @@ -996,7 +996,7 @@ export class ReadKeyExpr extends Expression { } override clone(): ReadKeyExpr { - return new ReadKeyExpr(this.receiver, this.index.clone(), this.type, this.sourceSpan); + return new ReadKeyExpr(this.receiver.clone(), this.index.clone(), this.type, this.sourceSpan); } } diff --git a/packages/compiler/src/template/pipeline/ir/src/expression.ts b/packages/compiler/src/template/pipeline/ir/src/expression.ts index 025c6102a1e..019ceec0bec 100644 --- a/packages/compiler/src/template/pipeline/ir/src/expression.ts +++ b/packages/compiler/src/template/pipeline/ir/src/expression.ts @@ -675,7 +675,7 @@ export class AssignTemporaryExpr extends ExpressionBase { } override clone(): AssignTemporaryExpr { - const a = new AssignTemporaryExpr(this.expr, this.xref); + const a = new AssignTemporaryExpr(this.expr.clone(), this.xref); a.name = this.name; return a; } diff --git a/packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts b/packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts index c95a58a04a8..5fcef42358c 100644 --- a/packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts +++ b/packages/compiler/src/template/pipeline/src/phases/temporary_variables.ts @@ -24,37 +24,56 @@ export function phaseTemporaryVariables(cpl: CompilationJob): void { let opCount = 0; let generatedStatements: Array> = []; for (const op of unit.ops()) { + // Identify the final time each temp var is read. + const finalReads = new Map(); + ir.visitExpressionsInOp(op, expr => { + if (expr instanceof ir.ReadTemporaryExpr) { + finalReads.set(expr.xref, expr); + } + }); + + // Name the temp vars, accounting for the fact that a name can be reused after it has been + // read for the final time. let count = 0; - let xrefs = new Set(); - let defs = new Map(); - + const assigned = new Set(); + const released = new Set(); + const defs = new Map(); ir.visitExpressionsInOp(op, expr => { - if (expr instanceof ir.ReadTemporaryExpr || expr instanceof ir.AssignTemporaryExpr) { - xrefs.add(expr.xref); - } - }); - - for (const xref of xrefs) { - // TODO: Exactly replicate the naming scheme used by `TemplateDefinitionBuilder`. It seems - // to rely on an expression index instead of an op index. - defs.set(xref, `tmp_${opCount}_${count++}`); - } - - ir.visitExpressionsInOp(op, expr => { - if (expr instanceof ir.ReadTemporaryExpr || expr instanceof ir.AssignTemporaryExpr) { - const name = defs.get(expr.xref); - if (name === undefined) { - throw new Error('Found xref with unassigned name'); + if (expr instanceof ir.AssignTemporaryExpr) { + if (!assigned.has(expr.xref)) { + assigned.add(expr.xref); + // TODO: Exactly replicate the naming scheme used by `TemplateDefinitionBuilder`. + // It seems to rely on an expression index instead of an op index. + defs.set(expr.xref, `tmp_${opCount}_${count++}`); } - expr.name = name; + assignName(defs, expr); + } else if (expr instanceof ir.ReadTemporaryExpr) { + if (finalReads.get(expr.xref) === expr) { + released.add(expr.xref); + count--; + } + assignName(defs, expr); } }); + // Add declarations for the temp vars. generatedStatements.push( - ...Array.from(defs.values()) + ...Array.from(new Set(defs.values())) .map(name => ir.createStatementOp(new o.DeclareVarStmt(name)))); opCount++; } unit.update.prepend(generatedStatements); } } + +/** + * Assigns a name to the temporary variable in the given temporary variable expression. + */ +function assignName( + names: Map, expr: ir.AssignTemporaryExpr|ir.ReadTemporaryExpr) { + const name = names.get(expr.xref); + if (name === undefined) { + throw new Error(`Found xref with unassigned name: ${expr.xref}`); + } + expr.name = name; +}