From 6d62d9f5057f4de89fe06e71aca01974b5f8ae7c Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 4 Apr 2023 12:30:12 -0700 Subject: [PATCH] Infer closures as frozen if they dont capture mutable values Fix for the previous issue, suggested by @gsathya: when we run InferReferenceEffects on the outer function we check each closure to see if it actually captured any mutable values. If it didn't, we can mark the closure as readonly and memoize it independently. --- compiler/forget/src/HIR/HIR.ts | 13 ++++++++ .../src/Inference/InferReferenceEffects.ts | 10 +++++- .../ReactiveScopes/PruneNonEscapingScopes.ts | 15 +-------- ...g.computed-call-evaluation-order.expect.md | 17 +++++++--- ...g.property-call-evaluation-order.expect.md | 17 +++++++--- .../compiler/array-at-closure.expect.md | 17 +++++++--- ...apturing-function-runs-inference.expect.md | 17 +++++++--- .../function-declaration-simple.expect.md | 15 ++++++--- ...rtent-mutability-readonly-lambda.expect.md | 26 ++++++++++++--- .../useEffect-nested-lambdas.expect.md | 32 ++++++++++++------- .../compiler/useEffect-nested-lambdas.js | 1 + .../compiler/useMemo-simple.expect.md | 23 +++++++++---- 12 files changed, 143 insertions(+), 60 deletions(-) diff --git a/compiler/forget/src/HIR/HIR.ts b/compiler/forget/src/HIR/HIR.ts index 5e70158bac..e0ddd97941 100644 --- a/compiler/forget/src/HIR/HIR.ts +++ b/compiler/forget/src/HIR/HIR.ts @@ -762,6 +762,19 @@ export enum Effect { Store = "store", } +export function isMutableEffect(effect: Effect): boolean { + switch (effect) { + case Effect.Capture: + case Effect.Mutate: + case Effect.Store: { + return true; + } + default: { + return false; + } + } +} + export type ReactiveScope = { id: ScopeId; range: MutableRange; diff --git a/compiler/forget/src/Inference/InferReferenceEffects.ts b/compiler/forget/src/Inference/InferReferenceEffects.ts index 2f5892a1cd..d1eabcba22 100644 --- a/compiler/forget/src/Inference/InferReferenceEffects.ts +++ b/compiler/forget/src/Inference/InferReferenceEffects.ts @@ -15,6 +15,7 @@ import { HIRFunction, IdentifierId, InstructionValue, + isMutableEffect, isObjectType, MethodCall, Phi, @@ -679,13 +680,20 @@ function inferBlock( break; } case "FunctionExpression": { + let hasMutableOperand = false; for (const operand of eachInstructionOperand(instr)) { state.reference( operand, operand.effect === Effect.Unknown ? Effect.Read : operand.effect ); + hasMutableOperand ||= isMutableEffect(operand.effect); } - state.initialize(instrValue, ValueKind.Mutable); + // If a closure did not capture any mutable values, then we can consider it to be + // frozen, which allows it to be independently memoized. + state.initialize( + instrValue, + hasMutableOperand ? ValueKind.Mutable : ValueKind.Frozen + ); state.define(instr.lvalue, instrValue); instr.lvalue.effect = Effect.Store; continue; diff --git a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts index 263d03cace..b48791c364 100644 --- a/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -9,10 +9,10 @@ import invariant from "invariant"; import prettyFormat from "pretty-format"; import { CompilerError } from "../CompilerError"; import { - Effect, IdentifierId, InstructionId, isHookType, + isMutableEffect, Pattern, Place, ReactiveFunction, @@ -702,16 +702,3 @@ class PruneScopesTransform extends ReactiveFunctionTransform< } } } - -function isMutableEffect(effect: Effect): boolean { - switch (effect) { - case Effect.Capture: - case Effect.Mutate: - case Effect.Store: { - return true; - } - default: { - return false; - } - } -} diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md index 178d7de69e..f417b0d3aa 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.computed-call-evaluation-order.expect.md @@ -29,19 +29,26 @@ function changeF(o) { } function Component() { - const $ = React.unstable_useMemoCache(1); - let x; + const $ = React.unstable_useMemoCache(2); + let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - x = { f: () => console.log("original") }; + t0 = () => console.log("original"); + $[0] = t0; + } else { + t0 = $[0]; + } + let x; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + x = { f: t0 }; console.log("A"); console.log("B"); changeF(x); console.log("arg"); x.f(1); - $[0] = x; + $[1] = x; } else { - x = $[0]; + x = $[1]; } return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.expect.md index 2cc7b48c22..4b44ec86d2 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/_bug.property-call-evaluation-order.expect.md @@ -29,18 +29,25 @@ function changeF(o) { } function Component() { - const $ = React.unstable_useMemoCache(1); - let x; + const $ = React.unstable_useMemoCache(2); + let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - x = { f: () => console.log("original") }; + t0 = () => console.log("original"); + $[0] = t0; + } else { + t0 = $[0]; + } + let x; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + x = { f: t0 }; console.log("A"); changeF(x); console.log("arg"); x.f(1); - $[0] = x; + $[1] = x; } else { - x = $[0]; + x = $[1]; } return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/array-at-closure.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/array-at-closure.expect.md index 8e418fad9b..c828bc29c0 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/array-at-closure.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/array-at-closure.expect.md @@ -18,7 +18,7 @@ function Component(props) { ```javascript function Component(props) { - const $ = React.unstable_useMemoCache(5); + const $ = React.unstable_useMemoCache(7); const c_0 = $[0] !== props.x; let t0; if (c_0) { @@ -33,18 +33,27 @@ function Component(props) { const c_3 = $[3] !== x; let t1; if (c_2 || c_3) { - const fn = function () { + t1 = function () { const arr = [...bar(props)]; return arr.at(x); }; - t1 = fn(); $[2] = props; $[3] = x; $[4] = t1; } else { t1 = $[4]; } - const fnResult = t1; + const fn = t1; + const c_5 = $[5] !== fn; + let t2; + if (c_5) { + t2 = fn(); + $[5] = fn; + $[6] = t2; + } else { + t2 = $[6]; + } + const fnResult = t2; return fnResult; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-runs-inference.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-runs-inference.expect.md index 311d191aa4..02579f109e 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-runs-inference.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/capturing-function-runs-inference.expect.md @@ -14,7 +14,7 @@ function component(a, b) { ```javascript function component(a, b) { - const $ = React.unstable_useMemoCache(4); + const $ = React.unstable_useMemoCache(6); const c_0 = $[0] !== a; let t0; if (c_0) { @@ -28,14 +28,23 @@ function component(a, b) { const c_2 = $[2] !== z; let t1; if (c_2) { - const p = () => {z}; - t1 = p(); + t1 = () => {z}; $[2] = z; $[3] = t1; } else { t1 = $[3]; } - return t1; + const p = t1; + const c_4 = $[4] !== p; + let t2; + if (c_4) { + t2 = p(); + $[4] = p; + $[5] = t2; + } else { + t2 = $[5]; + } + return t2; } ``` diff --git a/compiler/forget/src/__tests__/fixtures/compiler/function-declaration-simple.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/function-declaration-simple.expect.md index 2aa3d5203c..a91bf0e61e 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/function-declaration-simple.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/function-declaration-simple.expect.md @@ -17,14 +17,21 @@ function component(a) { ```javascript function component(a) { - const $ = React.unstable_useMemoCache(2); + const $ = React.unstable_useMemoCache(3); const c_0 = $[0] !== a; let t; if (c_0) { t = { a }; - const x = function x(p) { - p.foo(); - }; + let t0; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t0 = function x(p) { + p.foo(); + }; + $[2] = t0; + } else { + t0 = $[2]; + } + const x = t0; x(t); $[0] = a; $[1] = t; diff --git a/compiler/forget/src/__tests__/fixtures/compiler/inadvertent-mutability-readonly-lambda.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/inadvertent-mutability-readonly-lambda.expect.md index b4de9d23f9..242c736227 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/inadvertent-mutability-readonly-lambda.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/inadvertent-mutability-readonly-lambda.expect.md @@ -22,14 +22,30 @@ function Component(props) { ```javascript function Component(props) { + const $ = React.unstable_useMemoCache(4); const [value, setValue] = useState(null); - - const onChange = (e) => setValue((value) => value + e.target.value); + const c_0 = $[0] !== setValue; + let t0; + if (c_0) { + t0 = (e) => setValue((value) => value + e.target.value); + $[0] = setValue; + $[1] = t0; + } else { + t0 = $[1]; + } + const onChange = t0; useOtherHook(); - - const x = {}; - foo(x, onChange); + const c_2 = $[2] !== onChange; + let x; + if (c_2) { + x = {}; + foo(x, onChange); + $[2] = onChange; + $[3] = x; + } else { + x = $[3]; + } return x; } diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.expect.md index 717093de0d..f3a0520d8b 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.expect.md @@ -5,6 +5,7 @@ function Component(props) { const item = useMutable(props.itemId); const dispatch = useDispatch(); + useFreeze(dispatch); const exit = useCallback(() => { dispatch(createExitAction()); @@ -30,13 +31,22 @@ function Component(props) { ```javascript function Component(props) { - const $ = React.unstable_useMemoCache(1); + const $ = React.unstable_useMemoCache(3); const item = useMutable(props.itemId); const dispatch = useDispatch(); - - const exit = () => { - dispatch(createExitAction()); - }; + useFreeze(dispatch); + const c_0 = $[0] !== dispatch; + let t0; + if (c_0) { + t0 = () => { + dispatch(createExitAction()); + }; + $[0] = dispatch; + $[1] = t0; + } else { + t0 = $[1]; + } + const exit = t0; useEffect(() => { const cleanup = GlobalEventEmitter.addListener("onInput", () => { @@ -48,14 +58,14 @@ function Component(props) { }, [exit, item]); maybeMutate(item); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 =
; - $[0] = t0; + let t1; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t1 =
; + $[2] = t1; } else { - t0 = $[0]; + t1 = $[2]; } - return t0; + return t1; } ``` diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.js b/compiler/forget/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.js index 48da074a8f..4a1439a287 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.js +++ b/compiler/forget/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.js @@ -1,6 +1,7 @@ function Component(props) { const item = useMutable(props.itemId); const dispatch = useDispatch(); + useFreeze(dispatch); const exit = useCallback(() => { dispatch(createExitAction()); diff --git a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-simple.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-simple.expect.md index 7a15a4931d..261051f078 100644 --- a/compiler/forget/src/__tests__/fixtures/compiler/useMemo-simple.expect.md +++ b/compiler/forget/src/__tests__/fixtures/compiler/useMemo-simple.expect.md @@ -13,27 +13,36 @@ function component(a) { ```javascript function component(a) { - const $ = React.unstable_useMemoCache(4); + const $ = React.unstable_useMemoCache(6); const c_0 = $[0] !== a; let t0; if (c_0) { - t0 = (() => [a])(); + t0 = () => [a]; $[0] = a; $[1] = t0; } else { t0 = $[1]; } - const x = t0; - const c_2 = $[2] !== x; + const c_2 = $[2] !== t0; let t1; if (c_2) { - t1 = ; - $[2] = x; + t1 = t0(); + $[2] = t0; $[3] = t1; } else { t1 = $[3]; } - return t1; + const x = t1; + const c_4 = $[4] !== x; + let t2; + if (c_4) { + t2 = ; + $[4] = x; + $[5] = t2; + } else { + t2 = $[5]; + } + return t2; } ```