[compiler] Delete LoweredFunction.dependencies and hoisted instructions #32096

mofeiZ · 2025-01-16T16:37:40Z

LoweredFunction dependencies were exclusively used for dependency extraction (in propagateScopeDeps). Now that we have a propagateScopeDepsHIR that recursively traverses into nested functions, we can delete dependencies and their associated synthetic LoadLocal/PropertyLoad instructions.

Internal snapshot diff for this change shows ~.2% of files changed. I read through ~60 of the changed files

most changes are due to better outlining (due to better DCE)
a few changes in memo inference are due to changed ordering

// source
arr.map(() => contextVar.inner);

// previous instructions
$0 = LoadLocal arr
$1 = $0.map
// Below instructions are synthetic
$2 = LoadLocal contextVar
$3 = $2.inner
$4 = Function deps=$3 context=contextVar {
  ...
}

a few changes are effectively bugfixes (see aliased-nested-scope-fn-expr)

Stack created with Sapling. Best reviewed with ReviewStack.

josephsavona

Super exciting! A couple questions that i'd really like to understand before we move forward but this is great!

josephsavona · 2025-01-21T18:17:08Z

compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts

+        if (
+          (instr.value.kind === 'FunctionExpression' ||
+            instr.value.kind === 'ObjectMethod') &&
+          place.identifier.type.kind === 'Primitive'
+        ) {
+          continue;
+        }


Can you double-check this case a bit more? Every time we have to special case something like this there's a chance that we're overlooking the larger rule. Why do we need to skip primitives, and why only for functionexpr/objectmethod?

If nothing else, this repeatedly checks properties of the instruction value for every single operand, which is inefficient, but i'm more concerned with the check itself.

Makes sense! This change special cases function calls that capture identifiers whose type is Primitive but have a scope for other reasons. I added this to reduce the number of files changes by this refactor.

We can remove this check (which leads to more merged scopes) or generalize it in visitPlace in a later PR.

josephsavona · 2025-01-21T18:26:04Z

compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts

      const context = instrValue.loweredFunc.func.context
-        .map(dep => printPlace(dep))
+        .map(dep => `${printPlace(dep)}`)


nit: the original printPlace(dep) is more clear

josephsavona · 2025-01-21T18:28:43Z

compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts

+    CompilerError.invariant(operand.effect === Effect.Unknown, {
+      reason: 'Unexpected unknown effect',


we expect the effect to be unknown in the condition, but the error message says the opposite (?)

josephsavona · 2025-01-21T18:31:52Z

compiler/packages/babel-plugin-react-compiler/src/Inference/InferReferenceEffects.ts

+              operandValues.size === 1 &&
+              operandValues.values().next().value?.kind === 'DeclareContext'


hmm why only consider hoisted context variables if there is one operand? could you take the example below and also add some other context reference and break this?

Please see compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/hoisting-setstate.expect.md for an explanation. I think this check is semantically important, but happy to refactor to make it cleaner

TODO: check if we can just "never freeze declare contexts"

josephsavona · 2025-01-21T18:32:57Z

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

+            if (
+              instr.value.kind === 'FunctionExpression' ||
+              instr.value.kind === 'ObjectMethod'
+            ) {
+              if (operand.identifier.type.kind === 'Primitive') {
+                continue;
+              }
+            }


see comment above, i'd like to understand this more. why this exact combination and not primitives of other instruction types, for example.

(Let's continue discussion here https://github.com/facebook/react/pull/32096/files#r1956773709)

josephsavona · 2025-01-21T18:33:18Z

compiler/packages/babel-plugin-react-compiler/src/SSA/EliminateRedundantPhi.ts

+  if (DEBUG) {
+    for (const [, block] of ir.blocks) {
+      for (const phi of block.phis) {
+        CompilerError.invariant(!rewrites.has(phi.place.identifier), {
+          reason: '[EliminateRedundantPhis]: rewrite not complete',
+          loc: phi.place.loc,
+        });
+        for (const [, operand] of phi.operands) {
+          CompilerError.invariant(!rewrites.has(operand.identifier), {
+            reason: '[EliminateRedundantPhis]: rewrite not complete',
+            loc: phi.place.loc,
+          });
+        }
+      }
+    }
+  }


revert this file? or set DEBUG=false

josephsavona · 2025-01-21T18:37:07Z

...abel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-func-mutate-2.expect.md

@@ -27,7 +27,6 @@ export const FIXTURE_ENTRYPOINT = {
 import { c as _c } from "react/compiler-runtime";
 function component(a, b) {
  const $ = _c(2);
-  const y = { b };


I wondered about the exact mechanism that was kicking in to let us prune this. We have always had code that recursively calls DCE on inner functions (its part of AnalyzeFunctions). And DCE prunes unused context variables on the outermost function its called on. So we were pruning the context access for y. But because we didn't prune unused deps, the outer DCE thought that y was still used. Now the inner DCE removes y from context, and then the outer DCE can see that y is unused and prune it.

Yep that's completely right. I thought about writing a separate pass to prune unused deps and their source instructions to reduce noise of the snapshot diffing for this PR. Ultimately it didn't seem worth the time

josephsavona · 2025-01-21T18:41:12Z

compiler/scripts/bundle-meta.sh

what's this doing here?

Small patch to pass aliased context values into `Object|ArrayExpression`s --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32093). * #32099 * #32104 * #32098 * #32097 * #32096 * #32095 * #32094 * __->__ #32093

See test fixture --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32094). * #32099 * #32104 * #32098 * #32097 * #32096 * #32095 * __->__ #32094 * #32093

See test fixture --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32095). * #32099 * #32104 * #32098 * #32097 * #32096 * __->__ #32095 * #32094 * #32093

mofeiZ · 2025-02-14T22:08:39Z

compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts

-    ...context,
-    temporaries: actuallyEvaluatedTemporaries,
-  });
+  const nodes = collectNonNullsInBlocks(fn, context);


This can now be cleaned up 🎉

mofeiZ · 2025-02-14T22:14:55Z

compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts

 // Helper class to track indirections such as LoadLocal and PropertyLoad.
 export class IdentifierState {
-  properties: Map<Identifier, Dependency> = new Map();
+  properties: Map<IdentifierId, Identifier> = new Map();


Now that we're no longer tracking dependencies, we also don't need to track a granular Property mapping.

We keep this aliasing logic around in this PR to reduce the number of files changed by the refactor. This is deleted altogether in #32097

mofeiZ · 2025-02-14T22:18:41Z

compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts

+    } else if (knownMutated.has(operand)) {
+      operand.effect = Effect.Mutate;


This PR (the rewrite) preserves existing behavior, but the knownMutated logic is removed altogether in #32097 (which simplifies this pass)

josephsavona

We reviewed in-person, this looks great! Super exciting, nice work as always!

LoweredFunction dependencies were exclusively used for dependency extraction (in `propagateScopeDeps`). Now that we have a `propagateScopeDepsHIR` that recursively traverses into nested functions, we can delete `dependencies` and their associated synthetic `LoadLocal`/`PropertyLoad` instructions. [Internal snapshot diff](https://www.internalfb.com/phabricator/paste/view/P1716950202) for this change shows ~.2% of files changed. I [read through ~60 of the changed files](https://www.internalfb.com/phabricator/paste/view/P1733074307) - most changes are due to better outlining (due to better DCE) - a few changes in memo inference are due to changed ordering ``` // source arr.map(() => contextVar.inner); // previous instructions $0 = LoadLocal arr $1 = $0.map // Below instructions are synthetic $2 = LoadLocal contextVar $3 = $2.inner $4 = Function deps=$3 context=contextVar { ... } ``` - a few changes are effectively bugfixes (see `aliased-nested-scope-fn-expr`)

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 16, 2025

vercel bot had a problem deploying to Preview January 16, 2025 16:38 Failure

mofeiZ force-pushed the pr32096 branch from 15ec9d3 to 3819d3a Compare January 16, 2025 20:33

mofeiZ mentioned this pull request Jan 16, 2025

[compiler] Clean up deadcode: DeriveMinimalDeps (non-hir fork) #32104

Merged

vercel bot deployed to Preview January 16, 2025 20:38 View deployment

mofeiZ force-pushed the pr32096 branch from 3819d3a to c5bf40b Compare January 16, 2025 22:43

vercel bot deployed to Preview January 16, 2025 22:48 View deployment

josephsavona reviewed Jan 21, 2025

View reviewed changes

mofeiZ force-pushed the pr32096 branch from c5bf40b to 4583a53 Compare January 22, 2025 21:26

vercel bot deployed to Preview January 22, 2025 21:28 View deployment

mofeiZ force-pushed the pr32096 branch from 4583a53 to 3b541c5 Compare January 31, 2025 22:55

vercel bot deployed to Preview January 31, 2025 22:58 View deployment

mofeiZ force-pushed the pr32096 branch from 3b541c5 to 4dc840f Compare February 1, 2025 00:03

vercel bot deployed to Preview February 1, 2025 00:04 View deployment

mofeiZ force-pushed the pr32096 branch from 4dc840f to 271c49a Compare February 1, 2025 01:37

This was referenced Feb 1, 2025

[compiler] Represent array accesses with PropertyLoad #32287

Merged

[compiler] Prepare HIRBuilder to be used by later passes #32286

Open

vercel bot deployed to Preview February 1, 2025 01:40 View deployment

mofeiZ force-pushed the pr32096 branch from 271c49a to 5addb0b Compare February 14, 2025 21:19

mofeiZ marked this pull request as ready for review February 14, 2025 21:19

vercel bot deployed to Preview February 14, 2025 21:22 View deployment

mofeiZ commented Feb 14, 2025

View reviewed changes

mofeiZ force-pushed the pr32096 branch from 5addb0b to de220a6 Compare February 15, 2025 01:10

vercel bot deployed to Preview February 15, 2025 01:12 View deployment

josephsavona approved these changes Feb 16, 2025

View reviewed changes

mofeiZ force-pushed the pr32096 branch from de220a6 to 7c0c9ab Compare February 18, 2025 16:26

vercel bot deployed to Preview February 18, 2025 16:27 View deployment

mofeiZ merged commit d99f8bb into main Feb 18, 2025
22 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Delete LoweredFunction.dependencies and hoisted instructions #32096

[compiler] Delete LoweredFunction.dependencies and hoisted instructions #32096

mofeiZ commented Jan 16, 2025 •

edited

Loading

josephsavona left a comment

josephsavona Jan 21, 2025

mofeiZ Feb 14, 2025

josephsavona Jan 21, 2025

josephsavona Jan 21, 2025

josephsavona Jan 21, 2025

mofeiZ Feb 14, 2025

mofeiZ Feb 14, 2025

josephsavona Jan 21, 2025

mofeiZ Feb 14, 2025

josephsavona Jan 21, 2025

josephsavona Jan 21, 2025

mofeiZ Jan 22, 2025

josephsavona Jan 21, 2025

mofeiZ Feb 14, 2025

mofeiZ Feb 14, 2025

mofeiZ Feb 14, 2025 •

edited

Loading

josephsavona left a comment

		CompilerError.invariant(operand.effect === Effect.Unknown, {
		reason: 'Unexpected unknown effect',

		operandValues.size === 1 &&
		operandValues.values().next().value?.kind === 'DeclareContext'

		} else if (knownMutated.has(operand)) {
		operand.effect = Effect.Mutate;

[compiler] Delete LoweredFunction.dependencies and hoisted instructions #32096

[compiler] Delete LoweredFunction.dependencies and hoisted instructions #32096

Conversation

mofeiZ commented Jan 16, 2025 • edited Loading

josephsavona left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mofeiZ Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

josephsavona left a comment

Choose a reason for hiding this comment

mofeiZ commented Jan 16, 2025 •

edited

Loading

mofeiZ Feb 14, 2025 •

edited

Loading