[compiler] Stop using function `dependencies` in propagateScopeDeps #31200

mofeiZ · 2024-10-11T20:58:00Z

Recursively visit inner function instructions to extract dependencies instead of using LoweredFunction.dependencies directly.

This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete LoweredFunction.dependencies altogether (#31204).

Some nice side effects

optional-chaining deps for inner functions
full DCE and outlining for inner functions (see [compiler][be] Clean up nested function context in DCE #31202)
fewer extraneous instructions (see [compiler] Delete LoweredFunction.dependencies and hoisted instructions #31204)

Stack created with Sapling. Best reviewed with ReviewStack.

vercel · 2024-10-11T20:58:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 15, 2024 5:06pm

Recursively collect identifier / property loads and optional chains from inner functions. This PR is in preparation for #31200 Previously, we only did this in `collectHoistablePropertyLoads` to understand hoistable property loads from inner functions. 1. collectTemporariesSidemap 2. collectOptionalChainSidemap 3. collectHoistablePropertyLoads - ^ this recursively calls `collectTemporariesSidemap`, `collectOptionalChainSidemap`, and `collectOptionalChainSidemap` on inner functions 4. collectDependencies Now, we have 1. collectTemporariesSidemap - recursively record identifiers in inner functions. Note that we track all temporaries in the same map as `IdentifierIds` are currently unique across functions 2. collectOptionalChainSidemap - recursively records optional chain sidemaps in inner functions 3. collectHoistablePropertyLoads - (unchanged, except to remove recursive collection of temporaries) 4. collectDependencies - unchanged: to be modified to recursively collect dependencies in next PR '

mofeiZ · 2024-10-26T03:41:32Z

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

@@ -230,6 +230,8 @@ const EnvironmentConfigSchema = z.object({
   */
  enableUseTypeAnnotations: z.boolean().default(false),

+  enableFunctionDependencyRewrite: z.boolean().default(true),


Note that this is deleted in #31204

mofeiZ · 2024-10-26T03:42:38Z

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

-        const maybeOptionalChain = temporaries.get(operand[1].identifier.id);
-        if (maybeOptionalChain) {
-          context.visitDependency(maybeOptionalChain);
+  const handleFunction = (fn: HIRFunction): void => {


This change is code movement (moving logic into handleFunction) so that we can recursively visit nested functions

mofeiZ · 2024-10-26T03:43:03Z

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

+          context.inInnerFn = true;
+          handleFunction(instr.value.loweredFunc.func);
+          context.inInnerFn = wasInInnerFn;


Recursive call. See explanation in Context.#declare for why inInnerFn is needed

mofeiZ · 2024-10-26T03:44:17Z

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

-  }
-  const y = t0;
+  const $ = _c(2);
+  const y = { b };


const y = { b } is dce'd in #31204 (after we delete LoweredFunction.dependencies)

Recursively collect identifier / property loads and optional chains from inner functions. This PR is in preparation for #31200 Previously, we only did this in `collectHoistablePropertyLoads` to understand hoistable property loads from inner functions. 1. collectTemporariesSidemap 2. collectOptionalChainSidemap 3. collectHoistablePropertyLoads - ^ this recursively calls `collectTemporariesSidemap`, `collectOptionalChainSidemap`, and `collectOptionalChainSidemap` on inner functions 4. collectDependencies Now, we have 1. collectTemporariesSidemap - recursively record identifiers in inner functions. Note that we track all temporaries in the same map as `IdentifierIds` are currently unique across functions 2. collectOptionalChainSidemap - recursively records optional chain sidemaps in inner functions 3. collectHoistablePropertyLoads - (unchanged, except to remove recursive collection of temporaries) 4. collectDependencies - unchanged: to be modified to recursively collect dependencies in next PR '

`enablePropagateScopeDepsHIR` is now used extensively in Meta. This has been tested for over two weeks in our e2e tests and production. The rest of this stack deletes `LoweredFunction.dependencies`, which the non-hir version of `PropagateScopeDeps` depends on. To avoid a more forked HIR (non-hir with dependencies and hir with no dependencies), let's go ahead and clean up the non-hir version of PropagateScopeDepsHIR. Note that all fixture changes in this PR were previously reviewed when they were copied to `propagate-scope-deps-hir-fork`. Will clean up / merge these duplicate fixtures in a later PR ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31199). * #31202 * #31203 * #31201 * #31200 * #31346 * __->__ #31199

…ns (#31346) Recursively collect identifier / property loads and optional chains from inner functions. This PR is in preparation for #31200 Previously, we only did this in `collectHoistablePropertyLoads` to understand hoistable property loads from inner functions. 1. collectTemporariesSidemap 2. collectOptionalChainSidemap 3. collectHoistablePropertyLoads - ^ this recursively calls `collectTemporariesSidemap`, `collectOptionalChainSidemap`, and `collectOptionalChainSidemap` on inner functions 4. collectDependencies Now, we have 1. collectTemporariesSidemap - recursively record identifiers in inner functions. Note that we track all temporaries in the same map as `IdentifierIds` are currently unique across functions 2. collectOptionalChainSidemap - recursively records optional chain sidemaps in inner functions 3. collectHoistablePropertyLoads - (unchanged, except to remove recursive collection of temporaries) 4. collectDependencies - unchanged: to be modified to recursively collect dependencies in next PR ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31346). * #31202 * #31203 * #31201 * #31200 * __->__ #31346 * #31199

josephsavona · 2024-11-12T01:11:15Z

...ler/preserve-memo-validation/error.todo-useCallback-captures-reassigned-context-property.tsx

+ * TODO: we're currently bailing out because `contextVar` is a context variable
+ * and not recorded into the PropagateScopeDeps LoadLocal / PropertyLoad
+ * sidemap. Previously, we were able to avoid this as `BuildHIR` hoisted
+ * `LoadContext` and `PropertyLoad` instructions into the outer function, which
+ * we took as eligible dependencies.
+ *
+ * One solution is to simply record `LoadContext` identifiers into the
+ * temporaries sidemap when the instruction occurs *after* the context
+ * variable's mutable range.


just checking, but how often does this case occur internally? would be great to double-check by syncing, if you haven't already

We only observe this ~10 times in our biggest internal project: see comparison diff for this PR

josephsavona · 2024-11-12T01:11:55Z

...pe-deps-hir-fork/reduce-reactive-deps/todo-infer-function-uncond-optionals-hoisted.expect.md

@@ -34,9 +34,9 @@ function useFoo(t0) {
  const $ = _c(2);
  const { a } = t0;
  let t1;
-  if ($[0] !== a.b) {
+  if ($[0] !== a.b?.c.d?.e) {


josephsavona · 2024-11-12T01:12:16Z

...ckages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/react-namespace.expect.md

  React.useContext(FooContext);
  const ref = React.useRef();
  const [x, setX] = React.useState(false);
  let t0;
-  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
+  if ($[0] !== ref) {


hmmm we shouldn't be taking refs as deps, this seems off?

Should be fixed in #31521. Will rebase and update

josephsavona

Overall looks good but see fixtures - looks like this causes us to take refs as deps in some cases which isn't quite right

mofeiZ · 2024-11-12T18:51:39Z

Overall looks good but see fixtures - looks like this causes us to take refs as deps in some cases which isn't quite right

Ohh good catch. Debugged further and it looks like the underlying issue is how we special case ref.current accesses in propagateScopeDeps. Added a fix in #31521 which should also fix this

josephsavona

nice nice

We were previously filtering out `ref.current` dependencies in propagateScopeDependencies:checkValidDependency`. This is incorrect. Instead, we now always take a dependency on ref values (the outer box) as they may be reactive. Pruning is done in pruneNonReactiveDependencies. This PR includes a small patch to `collectReactiveIdentifier`. Prior to this, we conservatively assumed that pruned scopes always produced reactive declarations. This assumption fixed a bug with non-reactivity, but some of these declarations are `useRef` calls. Now we have special handling for this case ```js // This often produces a pruned scope React.useRef(1); ```

Recursively visit inner function instructions to extract dependencies instead of using `LoweredFunction.dependencies` directly. This is currently gated by enableFunctionDependencyRewrite, which needs to be removed before we delete `LoweredFunction.dependencies` altogether (#31204). Some nice side effects - optional-chaining deps for inner functions - full DCE and outlining for inner functions (see #31202) - fewer extraneous instructions (see #31204) -

We were previously filtering out `ref.current` dependencies in propagateScopeDependencies:checkValidDependency`. This is incorrect. Instead, we now always take a dependency on ref values (the outer box) as they may be reactive. Pruning is done in pruneNonReactiveDependencies. This PR includes a small patch to `collectReactiveIdentifier`. Prior to this, we conservatively assumed that pruned scopes always produced reactive declarations. This assumption fixed a bug with non-reactivity, but some of these declarations are `useRef` calls. Now we have special handling for this case ```js // This often produces a pruned scope React.useRef(1); ``` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31521). * #31202 * #31203 * #31201 * #31200 * __->__ #31521

`JSXMemberExpression` is currently the only instruction (that I know of) that directly references identifier lvalues without a corresponding `LoadLocal`. This has some side effects: - deadcode elimination and constant propagation now reach JSXMemberExpressions - we can delete `LoweredFunction.dependencies` without dangling references (previously, the only reference to JSXMemberExpression objects in HIR was in function dependencies) - JSXMemberExpression now is consistent with all other instructions (e.g. has a rvalue-producing LoadLocal) ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31201). * #31202 * #31203 * __->__ #31201 * #31200 * #31521

Add more `FIXTURE_ENTRYPOINT`s ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31203). * #31202 * __->__ #31203 * #31201 * #31200 * #31521

Now that we rely on function context exclusively, let's clean up `HIRFunction.context` after DCE. This PR is in preparation of #31204, which would otherwise have unnecessary declarations (of context values that become entirely DCE'd) ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31202). * __->__ #31202 * #31203 * #31201 * #31200 * #31521

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 11, 2024

vercel bot deployed to Preview October 11, 2024 21:02 View deployment

mofeiZ force-pushed the pr31200 branch from af227d3 to c7169c0 Compare October 24, 2024 18:01

mofeiZ changed the title ~~[compiler][needs cleanup]: stop using function deps in PropagateScopeDepsHIR~~ [compiler] Stop using function dependencies in propagateScopeDeps Oct 24, 2024

This was referenced Oct 24, 2024

[compiler] bugfix for hoistable deps for nested functions #31345

Merged

[compiler] Collect temporaries and optional chains from inner functions #31346

Merged

vercel bot deployed to Preview October 24, 2024 18:04 View deployment

mofeiZ force-pushed the pr31200 branch 2 times, most recently from a84a59d to c5b7421 Compare October 24, 2024 18:13

vercel bot deployed to Preview October 24, 2024 18:18 View deployment

mofeiZ marked this pull request as ready for review October 24, 2024 19:19

mofeiZ force-pushed the pr31200 branch from c5b7421 to 754d933 Compare October 24, 2024 19:24

vercel bot deployed to Preview October 24, 2024 19:27 View deployment

mofeiZ commented Oct 26, 2024

View reviewed changes

mofeiZ force-pushed the pr31200 branch from 754d933 to a19e4bb Compare November 5, 2024 19:06

vercel bot deployed to Preview November 5, 2024 19:12 View deployment

mofeiZ force-pushed the pr31200 branch from 2b51f3b to 12fe556 Compare November 5, 2024 23:40

vercel bot deployed to Preview November 5, 2024 23:43 View deployment

mofeiZ force-pushed the pr31200 branch from 12fe556 to e8a7ef6 Compare November 5, 2024 23:55

vercel bot deployed to Preview November 5, 2024 23:58 View deployment

josephsavona reviewed Nov 12, 2024

View reviewed changes

mofeiZ force-pushed the pr31200 branch from e8a7ef6 to 7812226 Compare November 12, 2024 19:14

mofeiZ mentioned this pull request Nov 12, 2024

[compiler] Fix: ref.current now correctly reactive #31521

Merged

vercel bot deployed to Preview November 12, 2024 19:18 View deployment

josephsavona approved these changes Nov 12, 2024

View reviewed changes

mofeiZ added 2 commits November 15, 2024 12:02

mofeiZ force-pushed the pr31200 branch from 7812226 to 6483117 Compare November 15, 2024 17:05

vercel bot deployed to Preview November 15, 2024 17:06 View deployment

mofeiZ merged commit c09402a into main Nov 15, 2024
19 checks passed

poteto deleted the pr31200 branch November 18, 2024 15:18

vercel-release-bot mentioned this pull request Nov 20, 2024

Upgrade React from 380f5d67-20241113 to 64f89510-20241119 vercel/next.js#73011

Closed

This was referenced Dec 4, 2024

Upgrade React from b01722d5-20241114 to 1b1283ad-20241203 vercel/next.js#73506

Merged

Upgrade React from b01722d5-20241114 to de68d2f4-20241204 vercel/next.js#73522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Stop using function `dependencies` in propagateScopeDeps #31200

[compiler] Stop using function `dependencies` in propagateScopeDeps #31200

mofeiZ commented Oct 11, 2024 •

edited

Loading

vercel bot commented Oct 11, 2024 •

edited

Loading

mofeiZ Oct 26, 2024

mofeiZ Oct 26, 2024

mofeiZ Oct 26, 2024

mofeiZ Oct 26, 2024

josephsavona Nov 12, 2024

mofeiZ Nov 12, 2024

josephsavona Nov 12, 2024

josephsavona Nov 12, 2024

mofeiZ Nov 12, 2024

josephsavona left a comment

mofeiZ commented Nov 12, 2024 •

edited

Loading

josephsavona left a comment

[compiler] Stop using function dependencies in propagateScopeDeps #31200

[compiler] Stop using function dependencies in propagateScopeDeps #31200

Conversation

mofeiZ commented Oct 11, 2024 • edited Loading

vercel bot commented Oct 11, 2024 • edited Loading

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

josephsavona left a comment

Choose a reason for hiding this comment

mofeiZ commented Nov 12, 2024 • edited Loading

josephsavona left a comment

Choose a reason for hiding this comment

[compiler] Stop using function `dependencies` in propagateScopeDeps #31200

[compiler] Stop using function `dependencies` in propagateScopeDeps #31200

mofeiZ commented Oct 11, 2024 •

edited

Loading

vercel bot commented Oct 11, 2024 •

edited

Loading

mofeiZ commented Nov 12, 2024 •

edited

Loading