From d1d6310f25cd74ce9f79ab22ec947fec67eecec5 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Wed, 6 Mar 2024 11:07:07 -0800 Subject: [PATCH] Conditional assignment of unmemoized value doesnt invalidate later scopes The previous implementation used IdentifierId, but since this pass operates after LeaveSSA the identifier ids are no longer distinct for different SSA instances. Instead we use the Identifier instance, which preserves SSA information (even ever LeaveSSA) and allows distinguishing between variables whose value always changes vs variables that may be reassigned such that they don't always invalidate. In the future when we use HIR everywhere, this pass should use the HIR CFG to understand that phi nodes whose operands all will always invalidate can also be treated as always invalidating. ## Test Plan Synced to www, 91 files have output changes (https://fburl.com/everpaste/3e3hjpjs). I spot checked these and confirmed that they are all from cases where there was already missing memoization of earlier values, where we now can prune later reactive scopes that depend on the un-memoized values. --- .../PruneAlwaysInvalidatingScopes.ts | 38 +++++----- ...-whose-deps-may-invalidate-array.expect.md | 70 +++++++++++++++++++ ...-scopes-whose-deps-may-invalidate-array.js | 19 +++++ 3 files changed, 108 insertions(+), 19 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.expect.md create mode 100644 compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.js diff --git a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts index d83df61345..9c1adc12c2 100644 --- a/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts +++ b/compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts @@ -11,7 +11,7 @@ import { visitReactiveFunction, } from "."; import { - IdentifierId, + Identifier, ReactiveFunction, ReactiveInstruction, ReactiveScopeBlock, @@ -33,8 +33,8 @@ export function pruneAlwaysInvalidatingScopes(fn: ReactiveFunction): void { } class Transform extends ReactiveFunctionTransform { - alwaysInvalidatingValues: Set = new Set(); - unmemoizedValues: Set = new Set(); + alwaysInvalidatingValues: Set = new Set(); + unmemoizedValues: Set = new Set(); override transformInstruction( instruction: ReactiveInstruction, @@ -50,34 +50,34 @@ class Transform extends ReactiveFunctionTransform { case "JsxFragment": case "NewExpression": { if (lvalue !== null) { - this.alwaysInvalidatingValues.add(lvalue.identifier.id); + this.alwaysInvalidatingValues.add(lvalue.identifier); if (!withinScope) { - this.unmemoizedValues.add(lvalue.identifier.id); + this.unmemoizedValues.add(lvalue.identifier); } } break; } case "StoreLocal": { - if (this.alwaysInvalidatingValues.has(value.value.identifier.id)) { - this.alwaysInvalidatingValues.add(value.lvalue.place.identifier.id); + if (this.alwaysInvalidatingValues.has(value.value.identifier)) { + this.alwaysInvalidatingValues.add(value.lvalue.place.identifier); } - if (this.unmemoizedValues.has(value.value.identifier.id)) { - this.unmemoizedValues.add(value.lvalue.place.identifier.id); + if (this.unmemoizedValues.has(value.value.identifier)) { + this.unmemoizedValues.add(value.lvalue.place.identifier); } break; } case "LoadLocal": { if ( lvalue !== null && - this.alwaysInvalidatingValues.has(value.place.identifier.id) + this.alwaysInvalidatingValues.has(value.place.identifier) ) { - this.alwaysInvalidatingValues.add(lvalue.identifier.id); + this.alwaysInvalidatingValues.add(lvalue.identifier); } if ( lvalue !== null && - this.unmemoizedValues.has(value.place.identifier.id) + this.unmemoizedValues.has(value.place.identifier) ) { - this.unmemoizedValues.add(lvalue.identifier.id); + this.unmemoizedValues.add(lvalue.identifier); } break; } @@ -92,19 +92,19 @@ class Transform extends ReactiveFunctionTransform { this.visitScope(scopeBlock, true); for (const dep of scopeBlock.scope.dependencies) { - if (this.unmemoizedValues.has(dep.identifier.id)) { + if (this.unmemoizedValues.has(dep.identifier)) { /* * This scope depends on an always-invalidating value so the scope will always invalidate: * prune it to avoid wasted comparisons */ - for (const [id, _decl] of scopeBlock.scope.declarations) { - if (this.alwaysInvalidatingValues.has(id)) { - this.unmemoizedValues.add(id); + for (const [_, decl] of scopeBlock.scope.declarations) { + if (this.alwaysInvalidatingValues.has(decl.identifier)) { + this.unmemoizedValues.add(decl.identifier); } } for (const identifier of scopeBlock.scope.reassignments) { - if (this.alwaysInvalidatingValues.has(identifier.id)) { - this.unmemoizedValues.add(identifier.id); + if (this.alwaysInvalidatingValues.has(identifier)) { + this.unmemoizedValues.add(identifier); } } return { kind: "replace-many", value: scopeBlock.instructions }; diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.expect.md b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.expect.md new file mode 100644 index 0000000000..96385c823f --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.expect.md @@ -0,0 +1,70 @@ + +## Input + +```javascript +import { useHook, identity } from "shared-runtime"; + +function Component(props) { + let x = 42; + if (props.cond) { + x = []; + } + useHook(); // intersperse a hook call to prevent memoization of x + identity(x); + + const y = [x]; + + return [y]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: "sathya" }], +}; + +``` + +## Code + +```javascript +import { unstable_useMemoCache as useMemoCache } from "react"; +import { useHook, identity } from "shared-runtime"; + +function Component(props) { + const $ = useMemoCache(4); + let x = 42; + if (props.cond) { + x = []; + } + + useHook(); + identity(x); + let t0; + if ($[0] !== x) { + t0 = [x]; + $[0] = x; + $[1] = t0; + } else { + t0 = $[1]; + } + const y = t0; + let t1; + if ($[2] !== y) { + t1 = [y]; + $[2] = y; + $[3] = t1; + } else { + t1 = $[3]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: "sathya" }], +}; + +``` + +### Eval output +(kind: ok) [[42]] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.js b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.js new file mode 100644 index 0000000000..b3678c52cd --- /dev/null +++ b/compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.js @@ -0,0 +1,19 @@ +import { useHook, identity } from "shared-runtime"; + +function Component(props) { + let x = 42; + if (props.cond) { + x = []; + } + useHook(); // intersperse a hook call to prevent memoization of x + identity(x); + + const y = [x]; + + return [y]; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ value: "sathya" }], +};