From 484930e69400305e9c3fabfbca3abf9d5487c793 Mon Sep 17 00:00:00 2001 From: Mike Vitousek Date: Wed, 29 May 2024 12:30:11 -0700 Subject: [PATCH] [compiler] Recompute values every time Summary: This PR expands the analysis from the previous in the stack in order to also capture when a value can incorrectly change within a single render, rather than just changing between two renders. In the case where dependencies have changed and so a new value is being computed, we now compute the value twice and compare the results. This would, for example, catch when we call Math.random() in render. The generated code is a little convoluted, because we don't want to have to traverse the generated code and substitute variable names with new ones. Instead, we save the initial value to the cache as normal, then run the computation block again and compare the resulting values to the cached ones. Then, to make sure that the cached values are identical to the computed ones, we reassign the cached values into the output variables. [ghstack-poisoned] --- .../ReactiveScopes/CodegenReactiveFunction.ts | 207 ++++++++++-------- ...-pruned-dependency-change-detect.expect.md | 11 +- .../react-compiler-runtime/src/index.ts | 5 +- 3 files changed, 124 insertions(+), 99 deletions(-) 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 9f1b5edbe32..39808f9a855 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/CodegenReactiveFunction.ts @@ -461,6 +461,11 @@ function codegenReactiveScope( ): void { const cacheStoreStatements: Array = []; const cacheLoadStatements: Array = []; + const cacheLoads: Array<{ + name: t.Identifier; + index: number; + value: t.Expression; + }> = []; const changeExpressions: Array = []; const changeExpressionComments: Array = []; const outputComments: Array = []; @@ -488,6 +493,10 @@ function codegenReactiveScope( } else { changeExpressions.push(comparison); } + /* + * Adding directly to cacheStoreStatements rather than cacheLoads, because there + * is no corresponding cacheLoadStatement for dependencies + */ cacheStoreStatements.push( t.expressionStatement( t.assignmentExpression( @@ -523,32 +532,7 @@ function codegenReactiveScope( t.variableDeclaration("let", [t.variableDeclarator(name)]) ); } - cacheStoreStatements.push( - t.expressionStatement( - t.assignmentExpression( - "=", - t.memberExpression( - t.identifier(cx.synthesizeName("$")), - t.numericLiteral(index), - true - ), - wrapCacheDep(cx, name) - ) - ) - ); - cacheLoadStatements.push( - t.expressionStatement( - t.assignmentExpression( - "=", - name, - t.memberExpression( - t.identifier(cx.synthesizeName("$")), - t.numericLiteral(index), - true - ) - ) - ) - ); + cacheLoads.push({ name, index, value: wrapCacheDep(cx, name) }); cx.declare(identifier); } for (const reassignment of scope.reassignments) { @@ -558,34 +542,10 @@ function codegenReactiveScope( } const name = convertIdentifier(reassignment); outputComments.push(name.name); - - cacheStoreStatements.push( - t.expressionStatement( - t.assignmentExpression( - "=", - t.memberExpression( - t.identifier(cx.synthesizeName("$")), - t.numericLiteral(index), - true - ), - wrapCacheDep(cx, name) - ) - ) - ); - cacheLoadStatements.push( - t.expressionStatement( - t.assignmentExpression( - "=", - name, - t.memberExpression( - t.identifier(cx.synthesizeName("$")), - t.numericLiteral(index), - true - ) - ) - ) - ); + cacheLoads.push({ name, index, value: wrapCacheDep(cx, name) }); } + + // (changeExpressions.length > 0) || (changeExpressions.length let testCondition = (changeExpressions as Array).reduce( (acc: t.Expression | null, ident: t.Expression) => { if (acc == null) { @@ -622,62 +582,121 @@ function codegenReactiveScope( ); } let computationBlock = codegenBlock(cx, block); + let memoStatement; - const memoBlock = t.blockStatement(cacheLoadStatements); if ( cx.env.config.enableChangeDetectionForDebugging != null && changeExpressions.length > 0 ) { const detectionFunction = cx.env.config.enableChangeDetectionForDebugging.importSpecifierName; + const cacheLoadOldValueStatements: Array = []; const changeDetectionStatements: Array = []; - memoBlock.body.forEach((stmt) => { - if ( - stmt.type === "ExpressionStatement" && - stmt.expression.type === "AssignmentExpression" && - stmt.expression.left.type === "Identifier" - ) { - const name = stmt.expression.left.name; - const loadName = cx.synthesizeName(`old$${name}`); - statements.push( - t.variableDeclaration("let", [ - t.variableDeclarator(t.identifier(loadName)), + const idempotenceDetectionStatements: Array = []; + + for (const { name, index, value } of cacheLoads) { + const loadName = cx.synthesizeName(`old$${name.name}`); + const slot = t.memberExpression( + t.identifier(cx.synthesizeName("$")), + t.numericLiteral(index), + true + ); + statements.push( + t.variableDeclaration("let", [ + t.variableDeclarator(t.identifier(loadName)), + ]) + ); + cacheStoreStatements.push( + t.expressionStatement(t.assignmentExpression("=", slot, value)) + ); + cacheLoadOldValueStatements.push( + t.expressionStatement( + t.assignmentExpression("=", t.identifier(loadName), slot) + ) + ); + changeDetectionStatements.push( + t.expressionStatement( + t.callExpression(t.identifier(detectionFunction), [ + t.identifier(loadName), + name, + t.stringLiteral(name.name), + t.stringLiteral(cx.fnName), + t.stringLiteral("cached"), ]) - ); - stmt.expression.left = t.identifier(loadName); - changeDetectionStatements.push( - t.expressionStatement( - t.callExpression(t.identifier(detectionFunction), [ - t.identifier(loadName), - t.identifier(name), - t.stringLiteral(name), - t.stringLiteral(cx.fnName), - ]) - ) - ); - changeDetectionStatements.push( - t.expressionStatement( - t.assignmentExpression( - "=", - t.identifier(name), - t.identifier(loadName) - ) - ) - ); - } - }); + ) + ); + idempotenceDetectionStatements.push( + t.expressionStatement( + t.callExpression(t.identifier(detectionFunction), [ + slot, + name, + t.stringLiteral(name.name), + t.stringLiteral(cx.fnName), + t.stringLiteral("recomputed"), + ]) + ) + ); + idempotenceDetectionStatements.push( + t.expressionStatement(t.assignmentExpression("=", name, slot)) + ); + } + const condition = cx.synthesizeName("condition"); memoStatement = t.blockStatement([ ...computationBlock.body, + t.variableDeclaration("let", [ + t.variableDeclarator(t.identifier(condition), testCondition), + ]), t.ifStatement( - t.unaryExpression("!", testCondition), - t.blockStatement([...memoBlock.body, ...changeDetectionStatements]) + t.unaryExpression("!", t.identifier(condition)), + t.blockStatement([ + ...cacheLoadOldValueStatements, + ...changeDetectionStatements, + ]) ), ...cacheStoreStatements, + t.ifStatement( + t.identifier(condition), + t.blockStatement([ + ...computationBlock.body, + ...idempotenceDetectionStatements, + ]) + ), ]); } else { + for (const { name, index, value } of cacheLoads) { + cacheStoreStatements.push( + t.expressionStatement( + t.assignmentExpression( + "=", + t.memberExpression( + t.identifier(cx.synthesizeName("$")), + t.numericLiteral(index), + true + ), + value + ) + ) + ); + cacheLoadStatements.push( + t.expressionStatement( + t.assignmentExpression( + "=", + name, + t.memberExpression( + t.identifier(cx.synthesizeName("$")), + t.numericLiteral(index), + true + ) + ) + ) + ); + } computationBlock.body.push(...cacheStoreStatements); - - memoStatement = t.ifStatement(testCondition, computationBlock, memoBlock); + memoStatement = t.ifStatement( + testCondition, + computationBlock, + t.blockStatement(cacheLoadStatements) + ); } if (cx.env.config.enableMemoizationComments) { @@ -719,9 +738,9 @@ function codegenReactiveScope( true ); } - if (memoBlock.body.length > 0) { + if (cacheLoadStatements.length > 0) { t.addComment( - memoBlock.body[0]!, + cacheLoadStatements[0]!, "leading", ` Inputs did not change, use cached value`, true 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 2aefa2173a1..4aae2a0b2a8 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 @@ -33,13 +33,18 @@ function Component(props) { let old$t1; { t1 =
{x}
; - if (!($[1] !== x)) { + let condition = $[1] !== x; + if (!condition) { old$t1 = $[2]; - $structuralCheck(old$t1, t1, "t1", "Component"); - t1 = old$t1; + $structuralCheck(old$t1, t1, "t1", "Component", "cached"); } $[1] = x; $[2] = t1; + if (condition) { + t1 =
{x}
; + $structuralCheck($[2], t1, "t1", "Component", "recomputed"); + t1 = $[2]; + } } return t1; } diff --git a/compiler/packages/react-compiler-runtime/src/index.ts b/compiler/packages/react-compiler-runtime/src/index.ts index aca78194ef2..f7583198113 100644 --- a/compiler/packages/react-compiler-runtime/src/index.ts +++ b/compiler/packages/react-compiler-runtime/src/index.ts @@ -258,10 +258,11 @@ export function $structuralCheck( oldValue: any, newValue: any, variableName: string, - fnName: string + fnName: string, + kind: string ): void { function error(l: string, r: string, path: string, depth: number) { - const str = `${fnName}: ${variableName}${path} changed from ${l} to ${r} at depth ${depth}`; + const str = `${fnName}: [${kind}] ${variableName}${path} changed from ${l} to ${r} at depth ${depth}`; if (seenErrors.has(str)) { return; }