From 7b22e7ca930d0f2af13816987c4987494509560b Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Mon, 1 Jul 2024 18:02:24 -0700 Subject: [PATCH] [compiler] Fewer assumptions about nonmutability when change detection enabled Summary: Change detection is desgined to determine whether rules of react have been violated, and to do so better we need to loosen Forgets assumptions about what kinds of values don't need to be memoized. For example, the compiler typically doesn't think of `o.x` as something that needs to be memoized, because it does not allocate. However, we want to compare `o.x` in the current render with `o.x` in a previous one, so we now insert a "memoization" (comparison, really) block around this value. [ghstack-poisoned] --- .../src/Entrypoint/Pipeline.ts | 1 + .../ReactiveScopes/CodegenReactiveFunction.ts | 13 ++++ .../InferReactiveScopeVariables.ts | 6 +- .../ReactiveScopes/PruneNonEscapingScopes.ts | 4 +- ...d-other-hook-unpruned-dependency.expect.md | 58 +++++++++++++---- ...-pruned-dependency-change-detect.expect.md | 49 ++++++++++---- .../useState-unpruned-dependency.expect.md | 64 ++++++++++++++----- 7 files changed, 151 insertions(+), 44 deletions(-) diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts index 3290930516e..a05061f9171 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts @@ -269,6 +269,7 @@ function* runWithEnvironment( if ( env.config.enablePreserveExistingManualUseMemo === "scope" || + env.config.enableChangeDetectionForDebugging != null || env.config.disableMemoizationForDebugging ) { memoizeExistingUseMemos(hir); diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts index f0470775407..2fa0dcffa8c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -653,6 +653,7 @@ function codegenReactiveScope( const cacheLoadOldValueStatements: Array = []; const changeDetectionStatements: Array = []; const idempotenceDetectionStatements: Array = []; + const restoreOldValueStatements: Array = []; for (const { name, index, value } of cacheLoads) { const loadName = cx.synthesizeName(`old$${name.name}`); @@ -669,6 +670,17 @@ function codegenReactiveScope( t.variableDeclarator(t.identifier(loadName), slot), ]) ); + if (scope.source) { + restoreOldValueStatements.push( + t.expressionStatement( + t.assignmentExpression( + "=", + t.cloneNode(name, true), + t.identifier(loadName) + ) + ) + ); + } changeDetectionStatements.push( t.expressionStatement( t.callExpression(t.identifier(detectionFunction), [ @@ -709,6 +721,7 @@ function codegenReactiveScope( t.blockStatement([ ...cacheLoadOldValueStatements, ...changeDetectionStatements, + ...restoreOldValueStatements, ]) ), ...cacheStoreStatements, diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts index 2d1bdfbdb70..a411753e1f7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/InferReactiveScopeVariables.ts @@ -366,7 +366,11 @@ export function findDisjointMutableValues( } for (const instr of block.instructions) { - const operands = collectMutableOperands(fn, instr, false); + const operands = collectMutableOperands( + fn, + instr, + fn.env.config.enableChangeDetectionForDebugging != null + ); if (operands.length !== 0) { scopeIdentifiers.union(operands); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts index ae0741a0310..5796bd751fe 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts @@ -811,7 +811,9 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor { this.env = env; this.options = { memoizeJsxElements: !this.env.config.enableForest, - forceMemoizePrimitives: this.env.config.enableForest, + forceMemoizePrimitives: + this.env.config.enableForest || + this.env.config.enableChangeDetectionForDebugging != null, }; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md index 63203246d69..db34cac6e41 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-and-other-hook-unpruned-dependency.expect.md @@ -39,10 +39,10 @@ function useOther(x) { } function Component(props) { - const $ = _c(4); + const $ = _c(8); let t0; { - t0 = f(props.x); + t0 = props.x; let condition = $[0] !== props.x; if (!condition) { let old$t0 = $[1]; @@ -51,31 +51,63 @@ function Component(props) { $[0] = props.x; $[1] = t0; if (condition) { - t0 = f(props.x); + t0 = props.x; $structuralCheck($[1], t0, "t0", "Component", "recomputed", "(8:8)"); t0 = $[1]; } } - const w = t0; - const z = useOther(w); - const [x] = useState(z); let t1; { - t1 =
{x}
; - let condition = $[2] !== x; + t1 = f(t0); + let condition = $[2] !== t0; if (!condition) { let old$t1 = $[3]; - $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(11:11)"); + $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(8:8)"); } - $[2] = x; + $[2] = t0; $[3] = t1; if (condition) { - t1 =
{x}
; - $structuralCheck($[3], t1, "t1", "Component", "recomputed", "(11:11)"); + t1 = f(t0); + $structuralCheck($[3], t1, "t1", "Component", "recomputed", "(8:8)"); t1 = $[3]; } } - return t1; + const w = t1; + const z = useOther(w); + const t2 = useState(z); + let x; + { + [x] = t2; + let condition = $[4] !== t2; + if (!condition) { + let old$x = $[5]; + $structuralCheck(old$x, x, "x", "Component", "cached", "(10:10)"); + } + $[4] = t2; + $[5] = x; + if (condition) { + [x] = t2; + $structuralCheck($[5], x, "x", "Component", "recomputed", "(10:10)"); + x = $[5]; + } + } + let t3; + { + t3 =
{x}
; + let condition = $[6] !== x; + if (!condition) { + let old$t3 = $[7]; + $structuralCheck(old$t3, t3, "t3", "Component", "cached", "(11:11)"); + } + $[6] = x; + $[7] = t3; + if (condition) { + t3 =
{x}
; + $structuralCheck($[7], t3, "t3", "Component", "recomputed", "(11:11)"); + t3 = $[7]; + } + } + return t3; } function f(x) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md index 4ae84cfdf24..93776908569 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-pruned-dependency-change-detect.expect.md @@ -20,32 +20,55 @@ import { c as _c } from "react/compiler-runtime"; // @enableChangeDetectionForDe import { useState } from "react"; function Component(props) { - const $ = _c(3); + const $ = _c(6); let t0; if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = f(props.x); + t0 = props.x; $[0] = t0; } else { t0 = $[0]; } - const [x] = useState(t0); let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = f(t0); + $[1] = t1; + } else { + t1 = $[1]; + } + const t2 = useState(t1); + let x; + { + [x] = t2; + let condition = $[2] !== t2; + if (!condition) { + let old$x = $[3]; + $structuralCheck(old$x, x, "x", "Component", "cached", "(5:5)"); + } + $[2] = t2; + $[3] = x; + if (condition) { + [x] = t2; + $structuralCheck($[3], x, "x", "Component", "recomputed", "(5:5)"); + x = $[3]; + } + } + let t3; { - t1 =
{x}
; - let condition = $[1] !== x; + t3 =
{x}
; + let condition = $[4] !== x; if (!condition) { - let old$t1 = $[2]; - $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(6:6)"); + let old$t3 = $[5]; + $structuralCheck(old$t3, t3, "t3", "Component", "cached", "(6:6)"); } - $[1] = x; - $[2] = t1; + $[4] = x; + $[5] = t3; if (condition) { - t1 =
{x}
; - $structuralCheck($[2], t1, "t1", "Component", "recomputed", "(6:6)"); - t1 = $[2]; + t3 =
{x}
; + $structuralCheck($[5], t3, "t3", "Component", "recomputed", "(6:6)"); + t3 = $[5]; } } - return t1; + return t3; } ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md index 8ca0d23ba87..9d107a2a432 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useState-unpruned-dependency.expect.md @@ -35,10 +35,10 @@ import { c as _c } from "react/compiler-runtime"; import { useState } from "react"; // @enableChangeDetectionForDebugging function Component(props) { - const $ = _c(5); + const $ = _c(9); let t0; { - t0 = f(props.x); + t0 = props.x; let condition = $[0] !== props.x; if (!condition) { let old$t0 = $[1]; @@ -47,41 +47,73 @@ function Component(props) { $[0] = props.x; $[1] = t0; if (condition) { - t0 = f(props.x); + t0 = props.x; $structuralCheck($[1], t0, "t0", "Component", "recomputed", "(4:4)"); t0 = $[1]; } } - const w = t0; - const [x] = useState(w); let t1; { - t1 = ( + t1 = f(t0); + let condition = $[2] !== t0; + if (!condition) { + let old$t1 = $[3]; + $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(4:4)"); + } + $[2] = t0; + $[3] = t1; + if (condition) { + t1 = f(t0); + $structuralCheck($[3], t1, "t1", "Component", "recomputed", "(4:4)"); + t1 = $[3]; + } + } + const w = t1; + const t2 = useState(w); + let x; + { + [x] = t2; + let condition = $[4] !== t2; + if (!condition) { + let old$x = $[5]; + $structuralCheck(old$x, x, "x", "Component", "cached", "(5:5)"); + } + $[4] = t2; + $[5] = x; + if (condition) { + [x] = t2; + $structuralCheck($[5], x, "x", "Component", "recomputed", "(5:5)"); + x = $[5]; + } + } + let t3; + { + t3 = (
{x} {w}
); - let condition = $[2] !== x || $[3] !== w; + let condition = $[6] !== x || $[7] !== w; if (!condition) { - let old$t1 = $[4]; - $structuralCheck(old$t1, t1, "t1", "Component", "cached", "(7:10)"); + let old$t3 = $[8]; + $structuralCheck(old$t3, t3, "t3", "Component", "cached", "(7:10)"); } - $[2] = x; - $[3] = w; - $[4] = t1; + $[6] = x; + $[7] = w; + $[8] = t3; if (condition) { - t1 = ( + t3 = (
{x} {w}
); - $structuralCheck($[4], t1, "t1", "Component", "recomputed", "(7:10)"); - t1 = $[4]; + $structuralCheck($[8], t3, "t3", "Component", "recomputed", "(7:10)"); + t3 = $[8]; } } - return t1; + return t3; } function f(x) {