[compiler] PruneNonEscapingScopes understands terminal operands #33062

josephsavona · 2025-04-30T05:19:22Z

Stack from ghstack (oldest at bottom):

-> [compiler] PruneNonEscapingScopes understands terminal operands #33062

Handles a few edge cases relating to for..of terminals, which have a compound init value block:

For..of with a return statement inside would not memoize the rvalue of the init expression (the collection). This is because there was no direct dependency relationship between the value being returned and the outer scope. The fix is to track the set of active scopes and make return values depend on all the active scopes at the point of the return.
For..of that reassigns a variable which later escapes were also not memoizing the collection. This is similar: the escape analysis aliasing didn't consider scopes that reassign, so there was no dependency between the returned variable and the scope that did the reassigning. The fix is to establish that dependency relationship.

In both cases, adding the additional scopes as a dependency of the right variable ensures that the scope's dependencies are treated as escaping (needing memoization), ensuring the collection is memoized.

We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. [ghstack-poisoned]

We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. ghstack-source-id: d4d58c3 Pull Request resolved: #33062

…rands" We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. [ghstack-poisoned]

We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. ghstack-source-id: f59837e Pull Request resolved: #33062

…rands" We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. [ghstack-poisoned]

We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. ghstack-source-id: a477f2c Pull Request resolved: #33062

josephsavona · 2025-04-30T07:02:48Z

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts

- * - rvalues: places that are aliased by the instruction's lvalues.
- * - level: the level of memoization to apply to this value
- */
-function computeMemoizationInputs(


this moves to become a method on CollectDependencies

josephsavona · 2025-04-30T07:03:49Z

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts

+        for (const instr of value.instructions) {
+          this.visitValueForMemoization(instr.id, instr.value, instr.lvalue);
+        }


this is the main change: not just traversing into sequence expressions, but processing the aliasing information into the state

mofeiZ · 2025-04-30T18:57:12Z

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/PruneNonEscapingScopes.ts

+     * If a scope reassigns any variables, set the chain of active scopes as a dependency
+     * of those variables. This ensures that if the variable escapes that we treat the
+     * reassignment scopes — and importantly their dependencies — as needing memoization.
+     */
+    for (const reassignment of scope.scope.reassignments) {


makes sense, thanks for the comment!

This ensures that if the variable escapes that we treat the reassignment scopes — and importantly their dependencies — as needing memoization.

I wonder how this was working previously.. If we were actually incorrectly pruning reassignment scopes, that sounds like a bug (e.g. if some reassignment scopes are pruned and others are kept due to declaring escaping values)

mofeiZ · 2025-04-30T18:58:33Z

...mpiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js

@@ -0,0 +1,16 @@
+function useHook(nodeID, condition) {
+  const graph = useContext(GraphContext);


nit: can we add imports for useContext?

mofeiZ · 2025-04-30T19:02:39Z

...mpiler/src/__tests__/fixtures/compiler/todo.memoize-loops-that-produce-memoizeable-values.js

+  let value;
+  for (const key of Object.keys(node?.fields ?? {})) {
+    if (condition) {
+      // (1) We currently create a scope just for this instruction, then later prune the scope because


Ahh that makes sense. I guess loops are kind of the only case that we might want to lengthen instead of completely pruning scopes. Specifically, we should rewrite / lengthen a scope if it is the outermost one in a loop.

Since flattenReactiveLoops happens before other relevant passes (flatten hook calls, propagate scope deps), this seems pretty straightforward.

Nice find!

…rands" Handles a few edge cases relating to for..of terminals, which have a compound `init` value block: * For..of with a return statement inside would not memoize the rvalue of the init expression (the collection). This is because there was no direct dependency relationship between the value being returned and the outer scope. The fix is to track the set of active scopes and make return values depend on all the active scopes at the point of the return. * For..of that reassigns a variable which later escapes were also not memoizing the collection. This is similar: the escape analysis aliasing didn't consider scopes that reassign, so there was no dependency between the returned variable and the scope that did the reassigning. The fix is to establish that dependency relationship. In both cases, adding the additional scopes as a dependency of the right variable ensures that the scope's dependencies are treated as escaping (needing memoization), ensuring the collection is memoized. [ghstack-poisoned]

We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. ghstack-source-id: 09a2923 Pull Request resolved: #33062

We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. ghstack-source-id: 09a2923 Pull Request resolved: #33062 DiffTrain build for [e9db3cc](e9db3cc)

We weren't treating terminal operands as eligible for memoization in PruneNonEscapingScopes, which meant that they could end up un-memoized. Terminal operands can also be compound ReactiveValues like SequenceExpressions, so part of the fix is to make sure we don't just recurse into compound values but record the full aliasing information we would for top-level instructions. Still WIP, this needs to handle terminals other than for..of. ghstack-source-id: 09a2923 Pull Request resolved: facebook#33062 DiffTrain build for [e9db3cc](facebook@e9db3cc)

facebook-github-bot added the CLA Signed label Apr 30, 2025

josephsavona marked this pull request as draft April 30, 2025 05:20

vercel bot deployed to Preview April 30, 2025 05:20 View deployment

vercel bot deployed to Preview April 30, 2025 06:47 View deployment

vercel bot deployed to Preview April 30, 2025 06:57 View deployment

josephsavona requested a review from mofeiZ April 30, 2025 07:01

josephsavona marked this pull request as ready for review April 30, 2025 07:02

josephsavona commented Apr 30, 2025

View reviewed changes

mofeiZ reviewed Apr 30, 2025

View reviewed changes

mofeiZ approved these changes Apr 30, 2025

View reviewed changes

josephsavona merged commit 2790dd4 into gh/josephsavona/73/base May 1, 2025
18 of 20 checks passed

vercel bot deployed to Preview May 1, 2025 03:41 View deployment

josephsavona deleted the gh/josephsavona/73/head branch May 1, 2025 03:41

vercel-release-bot mentioned this pull request May 5, 2025

Upgrade React from 197d6a04-20250424 to b9cfa0d3-20250505 vercel/next.js#78838

Closed

mvitousek mentioned this pull request May 5, 2025

[compiler] Migrate PruneNonEscapingScopes to HIR #31882

Open

This was referenced May 21, 2025

Upgrade React from 197d6a04-20250424 to 23884812-20250520 vercel/next.js#79462

Closed

Upgrade React from c4676e72-20250520 to 23884812-20250520 vercel/next.js#79467

Closed

eps1lon mentioned this pull request Jun 7, 2025

Upgrade React from 197d6a04-20250424 to 79d9aed7-20250620 vercel/next.js#79477

Merged

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[compiler] PruneNonEscapingScopes understands terminal operands #33062

[compiler] PruneNonEscapingScopes understands terminal operands #33062

Uh oh!

josephsavona commented Apr 30, 2025 •

edited

Loading

Uh oh!

josephsavona Apr 30, 2025

Uh oh!

josephsavona Apr 30, 2025

Uh oh!

mofeiZ Apr 30, 2025

Uh oh!

mofeiZ Apr 30, 2025

Uh oh!

mofeiZ Apr 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,16 @@
		function useHook(nodeID, condition) {
		const graph = useContext(GraphContext);

[compiler] PruneNonEscapingScopes understands terminal operands #33062

[compiler] PruneNonEscapingScopes understands terminal operands #33062

Uh oh!

Conversation

josephsavona commented Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsavona Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

josephsavona Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

mofeiZ Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

mofeiZ Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

mofeiZ Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

josephsavona commented Apr 30, 2025 •

edited

Loading