From d376cf1e378bfab8dc2ec11dac992863eb79eaa0 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Thu, 9 Mar 2023 12:44:30 -0800 Subject: [PATCH] Memoize arrays/objects created with destructuring spread We were treating Destructuring as if it could never allocate and therefore didn't have to be memoized. That's only true if there are no rest spreads though. This PR teaches the compiler to treat rest spreads differently for scoping and memoization purposes, fixing the newly added test case and some existing bugs. --- compiler/forget/src/HIR/visitors.ts | 28 ++++ .../InferReactiveScopeVariables.ts | 10 +- .../ReactiveScopes/PruneNonEscapingScopes.ts | 149 +++++++++++++----- .../fixtures/hir/destructuring.expect.md | 92 +++++++---- ...alysis-destructured-rest-element.expect.md | 22 ++- .../unused-object-element-with-rest.expect.md | 11 +- 6 files changed, 235 insertions(+), 77 deletions(-) diff --git a/compiler/forget/src/HIR/visitors.ts b/compiler/forget/src/HIR/visitors.ts index 02bb3fd52a..58397223fe 100644 --- a/compiler/forget/src/HIR/visitors.ts +++ b/compiler/forget/src/HIR/visitors.ts @@ -185,6 +185,34 @@ export function* eachInstructionValueOperand( } } +export function doesPatternContainSpreadElement(pattern: Pattern): boolean { + switch (pattern.kind) { + case "ArrayPattern": { + for (const item of pattern.items) { + if (item.kind === "Spread") { + return true; + } + } + break; + } + case "ObjectPattern": { + for (const property of pattern.properties) { + if (property.kind === "Spread") { + return true; + } + } + break; + } + default: { + assertExhaustive( + pattern, + `Unexpected pattern kind '${(pattern as any).kind}'` + ); + } + } + return false; +} + export function* eachPatternOperand(pattern: Pattern): Iterable { switch (pattern.kind) { case "ArrayPattern": { diff --git a/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts index 581c88aedd..b00169e4c1 100644 --- a/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -15,7 +15,11 @@ import { Place, ReactiveScope, } from "../HIR/HIR"; -import { eachInstructionOperand, eachPatternOperand } from "../HIR/visitors"; +import { + doesPatternContainSpreadElement, + eachInstructionOperand, + eachPatternOperand, +} from "../HIR/visitors"; import DisjointSet from "../Utils/DisjointSet"; import { assertExhaustive } from "../Utils/utils"; @@ -198,7 +202,9 @@ function isMutable({ id }: Instruction, place: Place): boolean { function mayAllocate(value: InstructionValue): boolean { switch (value.kind) { - case "Destructure": + case "Destructure": { + return doesPatternContainSpreadElement(value.lvalue.pattern); + } case "StoreLocal": case "LoadGlobal": case "TypeCastExpression": diff --git a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts index 5c222aa25f..7f8a2de5ac 100644 --- a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -12,6 +12,7 @@ import { Effect, IdentifierId, InstructionId, + Pattern, Place, ReactiveFunction, ReactiveInstruction, @@ -22,7 +23,6 @@ import { ReactiveValue, ScopeId, } from "../HIR"; -import { eachPatternOperand } from "../HIR/visitors"; import { log } from "../Utils/logger"; import { assertExhaustive } from "../Utils/utils"; import { getPlaceScope } from "./BuildReactiveBlocks"; @@ -319,43 +319,48 @@ function computeMemoizationInputs( lvalue: Place | null ): { // can optionally return a custom set of lvalues per instruction - lvalues: Array; + lvalues: Array; rvalues: Array; - level: MemoizationLevel; } { switch (value.kind) { case "ConditionalExpression": { return { - lvalues: lvalue !== null ? [lvalue] : [], + // Only need to memoize if the rvalues are memoized + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Conditional }] + : [], rvalues: [ // Conditionals do not alias their test value. ...computeMemoizationInputs(value.consequent, null).rvalues, ...computeMemoizationInputs(value.alternate, null).rvalues, ], - // Only need to memoize if the rvalues are memoized - level: MemoizationLevel.Conditional, }; } case "LogicalExpression": { return { - lvalues: lvalue !== null ? [lvalue] : [], + // Only need to memoize if the rvalues are memoized + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Conditional }] + : [], rvalues: [ ...computeMemoizationInputs(value.left, null).rvalues, ...computeMemoizationInputs(value.right, null).rvalues, ], - // Only need to memoize if the rvalues are memoized - level: MemoizationLevel.Conditional, }; } case "SequenceExpression": { return { - lvalues: lvalue !== null ? [lvalue] : [], + // Only need to memoize if the rvalues are memoized + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Conditional }] + : [], // Only the final value of the sequence is a true rvalue: // values from the sequence's instructions are evaluated // as separate nodes rvalues: computeMemoizationInputs(value.value, null).rvalues, - // Only memoize if the final value was memoized - level: MemoizationLevel.Conditional, }; } case "JsxExpression": { @@ -374,20 +379,24 @@ function computeMemoizationInputs( } } return { - lvalues: lvalue !== null ? [lvalue] : [], - rvalues: operands, // JSX elements themselves are not memoized unless forced to // avoid breaking downstream memoization - level: MemoizationLevel.Unmemoized, + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Unmemoized }] + : [], + rvalues: operands, }; } case "JsxFragment": { return { - lvalues: lvalue !== null ? [lvalue] : [], - rvalues: value.children, // JSX elements themselves are not memoized unless forced to // avoid breaking downstream memoization - level: MemoizationLevel.Unmemoized, + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Unmemoized }] + : [], + rvalues: value.children, }; } case "ComputedDelete": @@ -399,68 +408,84 @@ function computeMemoizationInputs( case "BinaryExpression": case "UnaryExpression": { return { - lvalues: lvalue !== null ? [lvalue] : [], - rvalues: [], // All of these instructions return a primitive value and never need to be memoized - level: MemoizationLevel.Never, + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Never }] + : [], + rvalues: [], }; } case "TypeCastExpression": { return { - lvalues: lvalue !== null ? [lvalue] : [], // Indirection for the inner value, memoized if the value is + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Conditional }] + : [], rvalues: [value.value], - level: MemoizationLevel.Conditional, }; } case "LoadLocal": { return { - lvalues: lvalue !== null ? [lvalue] : [], // Indirection for the inner value, memoized if the value is + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Conditional }] + : [], rvalues: [value.place], - level: MemoizationLevel.Conditional, }; } case "StoreLocal": { + const lvalues = [ + { place: value.lvalue.place, level: MemoizationLevel.Conditional }, + ]; + if (lvalue !== null) { + lvalues.push({ place: lvalue, level: MemoizationLevel.Conditional }); + } return { - lvalues: - lvalue !== null ? [lvalue, value.lvalue.place] : [value.lvalue.place], // Indirection for the inner value, memoized if the value is + lvalues, rvalues: [value.value], - level: MemoizationLevel.Conditional, }; } case "Destructure": { + // Indirection for the inner value, memoized if the value is const lvalues = []; if (lvalue !== null) { - lvalues.push(lvalue); + lvalues.push({ place: lvalue, level: MemoizationLevel.Conditional }); } - lvalues.push(...eachPatternOperand(value.lvalue.pattern)); + lvalues.push(...computePatternLValues(value.lvalue.pattern)); return { lvalues: lvalues, - // Indirection for the inner value, memoized if the value is rvalues: [value.value], - level: MemoizationLevel.Conditional, }; } case "ComputedLoad": case "PropertyLoad": { return { - lvalues: lvalue !== null ? [lvalue] : [], // Indirection for the inner value, memoized if the value is + lvalues: + lvalue !== null + ? [{ place: lvalue, level: MemoizationLevel.Conditional }] + : [], // Only the object is aliased to the result, and the result only needs to be // memoized if the object is rvalues: [value.object], - level: MemoizationLevel.Conditional, }; } case "ComputedStore": { // The object being stored to acts as an lvalue (it aliases the value), but // the computed key is not aliased + const lvalues = [ + { place: value.object, level: MemoizationLevel.Conditional }, + ]; + if (lvalue !== null) { + lvalues.push({ place: lvalue, level: MemoizationLevel.Conditional }); + } return { - lvalues: lvalue !== null ? [lvalue, value.object] : [value.object], + lvalues, rvalues: [value.value], - level: MemoizationLevel.Conditional, }; } case "FunctionExpression": @@ -475,16 +500,15 @@ function computeMemoizationInputs( // All of these instructions may produce new values which must be memoized if // reachable from a return value. Any mutable rvalue may alias any other rvalue const operands = [...eachReactiveValueOperand(value)]; - const lvalues = operands.filter((operand) => - isMutableEffect(operand.effect) - ); + const lvalues = operands + .filter((operand) => isMutableEffect(operand.effect)) + .map((place) => ({ place, level: MemoizationLevel.Memoized })); if (lvalue !== null) { - lvalues.push(lvalue); + lvalues.push({ place: lvalue, level: MemoizationLevel.Memoized }); } return { lvalues, rvalues: operands, - level: MemoizationLevel.Memoized, }; } case "UnsupportedNode": { @@ -496,6 +520,45 @@ function computeMemoizationInputs( } } +function computePatternLValues(pattern: Pattern): Array { + const lvalues: Array = []; + switch (pattern.kind) { + case "ArrayPattern": { + for (const item of pattern.items) { + if (item.kind === "Identifier") { + lvalues.push({ place: item, level: MemoizationLevel.Conditional }); + } else { + lvalues.push({ place: item.place, level: MemoizationLevel.Memoized }); + } + } + break; + } + case "ObjectPattern": { + for (const property of pattern.properties) { + if (property.kind === "ObjectProperty") { + lvalues.push({ + place: property.place, + level: MemoizationLevel.Conditional, + }); + } else { + lvalues.push({ + place: property.place, + level: MemoizationLevel.Memoized, + }); + } + } + break; + } + default: { + assertExhaustive( + pattern, + `Unexpected pattern kind '${(pattern as any).kind}'` + ); + } + } + return lvalues; +} + /** * Populates the input state with the set of returned identifiers and information about each * identifier's and scope's dependencies. @@ -521,7 +584,7 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { } // Add the operands as dependencies of all lvalues. - for (const lvalue of aliasing.lvalues) { + for (const { place: lvalue, level } of aliasing.lvalues) { const lvalueId = state.definitions.get(lvalue.identifier.id) ?? lvalue.identifier.id; let node = state.identifiers.get(lvalueId); @@ -535,7 +598,7 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { }; state.identifiers.set(lvalueId, node); } - node.level = joinAliases(node.level, aliasing.level); + node.level = joinAliases(node.level, level); // This looks like NxM iterations but in practice all instructions with multiple // lvalues have only a single rvalue for (const operand of aliasing.rvalues) { diff --git a/compiler/forget/src/__tests__/fixtures/hir/destructuring.expect.md b/compiler/forget/src/__tests__/fixtures/hir/destructuring.expect.md index f40bb9601b..4a12429c04 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/destructuring.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/destructuring.expect.md @@ -28,39 +28,75 @@ function foo(a, b, c) { ```javascript function foo(a, b, c) { - const $ = React.unstable_useMemoCache(8); - const [d, t40, ...h] = a; + const $ = React.unstable_useMemoCache(18); + const c_0 = $[0] !== a; + let t0; + let d; + let h; + if (c_0) { + [d, t0, ...h] = a; + $[0] = a; + $[1] = t0; + $[2] = d; + $[3] = h; + } else { + t0 = $[1]; + d = $[2]; + h = $[3]; + } - const [t43] = t40; - const { e: t45, ...g } = t43; - const { f } = t45; + const [t1] = t0; + const c_4 = $[4] !== t1; + let t2; + let g; + if (c_4) { + ({ e: t2, ...g } = t1); + $[4] = t1; + $[5] = t2; + $[6] = g; + } else { + t2 = $[5]; + g = $[6]; + } + const { f } = t2; const { l: t51, p } = b; - const { m: t54 } = t51; - const [t56, ...o] = t54; - const [n] = t56; - const c_0 = $[0] !== d; - const c_1 = $[1] !== f; - const c_2 = $[2] !== g; - const c_3 = $[3] !== h; - const c_4 = $[4] !== n; - const c_5 = $[5] !== o; - const c_6 = $[6] !== p; - let t0; - if (c_0 || c_1 || c_2 || c_3 || c_4 || c_5 || c_6) { - t0 = [d, f, g, h, n, o, p]; - $[0] = d; - $[1] = f; - $[2] = g; - $[3] = h; - $[4] = n; - $[5] = o; - $[6] = p; - $[7] = t0; + const { m: t3 } = t51; + const c_7 = $[7] !== t3; + let t4; + let o; + if (c_7) { + [t4, ...o] = t3; + $[7] = t3; + $[8] = t4; + $[9] = o; } else { - t0 = $[7]; + t4 = $[8]; + o = $[9]; } - return t0; + const [n] = t4; + const c_10 = $[10] !== d; + const c_11 = $[11] !== f; + const c_12 = $[12] !== g; + const c_13 = $[13] !== h; + const c_14 = $[14] !== n; + const c_15 = $[15] !== o; + const c_16 = $[16] !== p; + let t5; + if (c_10 || c_11 || c_12 || c_13 || c_14 || c_15 || c_16) { + t5 = [d, f, g, h, n, o, p]; + $[10] = d; + $[11] = f; + $[12] = g; + $[13] = h; + $[14] = n; + $[15] = o; + $[16] = p; + $[17] = t5; + } else { + t5 = $[17]; + } + return t5; } ``` diff --git a/compiler/forget/src/__tests__/fixtures/hir/escape-analysis-destructured-rest-element.expect.md b/compiler/forget/src/__tests__/fixtures/hir/escape-analysis-destructured-rest-element.expect.md index 310b7ada9b..d9fde0f28d 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/escape-analysis-destructured-rest-element.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/escape-analysis-destructured-rest-element.expect.md @@ -16,9 +16,25 @@ function Component(props) { ```javascript function Component(props) { - const { a, ...b } = props.a; - - const [c, ...d] = props.c; + const $ = React.unstable_useMemoCache(4); + const c_0 = $[0] !== props.a; + let b; + if (c_0) { + ({ a, ...b } = props.a); + $[0] = props.a; + $[1] = b; + } else { + b = $[1]; + } + const c_2 = $[2] !== props.c; + let d; + if (c_2) { + [c, ...d] = props.c; + $[2] = props.c; + $[3] = d; + } else { + d = $[3]; + } return
; } diff --git a/compiler/forget/src/__tests__/fixtures/hir/unused-object-element-with-rest.expect.md b/compiler/forget/src/__tests__/fixtures/hir/unused-object-element-with-rest.expect.md index d5e0a68550..8a7576905a 100644 --- a/compiler/forget/src/__tests__/fixtures/hir/unused-object-element-with-rest.expect.md +++ b/compiler/forget/src/__tests__/fixtures/hir/unused-object-element-with-rest.expect.md @@ -14,7 +14,16 @@ function Foo(props) { ```javascript function Foo(props) { - const { unused, ...rest } = props.a; + const $ = React.unstable_useMemoCache(2); + const c_0 = $[0] !== props.a; + let rest; + if (c_0) { + ({ unused, ...rest } = props.a); + $[0] = props.a; + $[1] = rest; + } else { + rest = $[1]; + } return rest; }