[compiler][hir] Only hoist always-accessed PropertyLoads from function decls #31066

mofeiZ · 2024-09-25T23:41:42Z

Stack from ghstack (oldest at bottom):

-> [compiler][hir] Only hoist always-accessed PropertyLoads from function decls #31066
[compiler][hir-rewrite] Check mutability of base identifier when hoisting #31032

Prior to this PR, we consider all of a nested function's accessed paths as 'hoistable' (to the basic block in which the function was defined). Now, we traverse nested functions and find all paths hoistable to their entry block.

Note that this only replaces the hoisting part of function declarations, not dependencies. This realistically only affects optional chains within functions, which always get truncated to its inner non-optional path (see todo-infer-function-uncond-optionals-hoisted.tsx)

See newly added test fixtures for details

Update: Note that toggling enableTreatFunctionDepsAsConditional makes a non-trivial impact on granularity of inferred deps (i.e. we find that function declarations uniquely identify some paths as hoistable). Snapshot comparison of internal code shows ~2.5% of files get worse dependencies (internal link)

vercel · 2024-09-25T23:41:44Z

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	Oct 3, 2024 6:33pm

…ecls ghstack-source-id: da8ae28a969cd027c076e36a9a815665a3af5754 Pull Request resolved: #31066

…n decls ghstack-source-id: c0a4b9548787365d7183a02ed26be88101a234cb Pull Request resolved: #31066

…n decls ghstack-source-id: 32e551e45a9d48085dd2a7ab3cc5efc112808900 Pull Request resolved: #31066

…n decls ghstack-source-id: 485a44f7203083d0bdff4eaf042e71d30a0a7897 Pull Request resolved: #31066

mofeiZ · 2024-10-01T15:43:09Z

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

+    temporaries: actuallyEvaluatedTemporaries,
+    knownImmutableIdentifiers,
    hoistableFromOptionals,
    registry,
-  );
+    nestedFnImmutableContext,


Note that we could technically reuse knownImmutableIdentifiers for non-mutable captured identifiers. This is a bit more explicit

mofeiZ · 2024-10-01T16:29:05Z

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

+      const maybeNonNull = getMaybeNonNullInInstruction(instr.value, context);
+      if (
+        maybeNonNull != null &&
+        isImmutableAtInstr(maybeNonNull.fullPath.identifier, instr.id, context)
+      ) {
+        assumedNonNullObjects.add(maybeNonNull);
+      }


This is just code movement. All mutability check logic moved to isImmutableAtInstr because it got more complex ("if we're within a nested function, check if the base identifier is in nestedFnImmutableContext, otherwise do the scope / mutable range check")

mofeiZ · 2024-10-01T16:31:55Z

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

+      if (
+        instr.value.kind === 'FunctionExpression' &&
+        !fn.env.config.enableTreatFunctionDepsAsConditional
+      ) {


This is the core logic for collecting hoistable paths in inner functions

mofeiZ · 2024-10-01T16:33:32Z

.../fixtures/compiler/reduce-reactive-deps/bug-infer-function-cond-access-not-hoisted.expect.md

+  if ($[0] !== shouldReadA || $[1] !== a.b.c) {
+    t1 = (
+      <Stringify
+        fn={() => {
+          if (shouldReadA) {
+            return a.b.c;
+          }
+          return null;
+        }}
+        shouldInvokeFns={true}
+      />
+    );
+    $[0] = shouldReadA;
+    $[1] = a.b.c;
+    $[2] = t1;


Note that the propagate-hir-fork version of this fixture (correctly) only adds a as a dependency

mofeiZ · 2024-10-01T16:34:47Z

...gate-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-local-var.expect.md

+    local = $[1];
+  }
+  let t1;
+  if ($[2] !== shouldReadA || $[3] !== local) {


conditional dependency within a function is truncated down to non-null load (e.g. local)

mofeiZ · 2024-10-01T16:35:08Z

...te-scope-deps-hir-fork/reduce-reactive-deps/infer-function-cond-access-not-hoisted.expect.md

+  if ($[0] !== shouldReadA || $[1] !== a) {
+    t1 = (
+      <Stringify
+        fn={() => {
+          if (shouldReadA) {
+            return a.b.c;
+          }
+          return null;
+        }}
+        shouldInvokeFns={true}
+      />
+    );
+    $[0] = shouldReadA;
+    $[1] = a;
+    $[2] = t1;
+  } else {


conditional dependency within a function is truncated down to non-null load (e.g. a)

…ting (#31032) Stack from [ghstack](https://github.com/ezyang/ghstack) (oldest at bottom): * #31066 * __->__ #31032 Prior to this PR, we check whether the property load source (e.g. the evaluation of `<base>` in `<base>.property`) is mutable + scoped to determine whether the property load itself is eligible for hoisting. This changes to check the base identifier of the load. - This is needed for the next PR #31066. We want to evaluate whether the base identifier is mutable within the context of the *outermost function*. This is because all LoadLocals and PropertyLoads within a nested function declaration have mutable-ranges within the context of the function, but the base identifier is a context variable. - A side effect is that we no longer infer loads from props / other function arguments as mutable in edge cases (e.g. props escaping out of try-blocks or being assigned to context variables)

…rtyLoads from function decls

Extends #31066 to ObjectMethods (somehow missed this before).

`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions

Extends #31066 to ObjectMethods (somehow missed this before).

Extends #31066 to ObjectMethods (somehow missed this before). -

`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions -

Extends #31066 to ObjectMethods (somehow missed this before). '

`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions '

Extends #31066 to ObjectMethods (somehow missed this before). '

`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions '

Extends #31066 to ObjectMethods (somehow missed this before). '

`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions '

Extends #31066 to ObjectMethods (somehow missed this before). ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31197). * #31204 * #31202 * #31203 * #31201 * #31200 * #31346 * #31199 * #31431 * #31345 * __->__ #31197

`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions ' --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31345). * #31204 * #31202 * #31203 * #31201 * #31200 * #31346 * #31199 * #31431 * __->__ #31345 * #31197

facebook-github-bot added the CLA Signed label Sep 25, 2024

This was referenced Sep 25, 2024

[compiler] repro for dep merging edge case (non-hir) #31035

Merged

[compiler] Renames and no-op refactor for next PR #31036

Merged

[compiler] Optional chaining for dependencies (HIR rewrite) #31037

Merged

mofeiZ added a commit that referenced this pull request Sep 25, 2024

[compiler][hir-rewrite] Stop hoisting PropertyLoads out of function d…

0962b20

…ecls ghstack-source-id: da8ae28a969cd027c076e36a9a815665a3af5754 Pull Request resolved: #31066

facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Sep 25, 2024

vercel bot deployed to Preview September 25, 2024 23:46 View deployment

mofeiZ mentioned this pull request Sep 26, 2024

[compiler][fixtures] Patch error-handling edge case in snap evaluator #31082

Merged

mofeiZ added a commit that referenced this pull request Sep 26, 2024

[compiler][hir] Only hoist always-accessed PropertyLoads from functio…

dd2fccf

…n decls ghstack-source-id: c0a4b9548787365d7183a02ed26be88101a234cb Pull Request resolved: #31066

vercel bot deployed to Preview September 26, 2024 21:36 View deployment

mofeiZ changed the title ~~[compiler][hir-rewrite] Stop hoisting PropertyLoads out of function decls~~ [compiler][hir] Only hoist always-accessed PropertyLoads from function decls Sep 26, 2024

mofeiZ added a commit that referenced this pull request Sep 26, 2024

[compiler][hir] Only hoist always-accessed PropertyLoads from functio…

a9e003b

…n decls ghstack-source-id: 32e551e45a9d48085dd2a7ab3cc5efc112808900 Pull Request resolved: #31066

vercel bot deployed to Preview September 26, 2024 22:44 View deployment

mofeiZ added a commit that referenced this pull request Sep 30, 2024

[compiler][hir] Only hoist always-accessed PropertyLoads from functio…

8273cd1

…n decls ghstack-source-id: 485a44f7203083d0bdff4eaf042e71d30a0a7897 Pull Request resolved: #31066

vercel bot deployed to Preview September 30, 2024 16:39 View deployment

mofeiZ mentioned this pull request Sep 30, 2024

[compiler] Test fixture: non-reactive phi creates 'dangling ref' scope #31103

Merged

vercel bot deployed to Preview September 30, 2024 18:29 View deployment

vercel bot deployed to Preview September 30, 2024 20:01 View deployment

mofeiZ commented Oct 1, 2024

View reviewed changes

mofeiZ changed the base branch from gh/mofeiZ/23/base to main October 3, 2024 17:56

mofeiZ changed the base branch from main to gh/mofeiZ/23/base October 3, 2024 18:26

Force push update to [compiler][hir] Only hoist always-accessed Prope…

e0701a3

…rtyLoads from function decls

mofeiZ force-pushed the gh/mofeiZ/23/head branch from d57086e to e0701a3 Compare October 3, 2024 18:32

mofeiZ changed the base branch from gh/mofeiZ/23/base to main October 3, 2024 18:32

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

mofeiZ merged commit 1460d67 into main Oct 3, 2024
5 checks passed

This was referenced Oct 24, 2024

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

Merged

[compiler][ez] Patch hoistability for ObjectMethods #31197

Merged

mofeiZ added a commit that referenced this pull request Oct 24, 2024

[compiler][ez] Patch hoistability for ObjectMethods

515134d

Extends #31066 to ObjectMethods (somehow missed this before).

mofeiZ added a commit that referenced this pull request Oct 24, 2024

[compiler][ez] Patch hoistability for ObjectMethods

9c02c87

Extends #31066 to ObjectMethods (somehow missed this before).

mofeiZ added a commit that referenced this pull request Oct 24, 2024

[compiler][ez] Patch hoistability for ObjectMethods

cbd12f5

Extends #31066 to ObjectMethods (somehow missed this before). -

mofeiZ added a commit that referenced this pull request Oct 24, 2024

[compiler][ez] Patch hoistability for ObjectMethods

1c6a90e

Extends #31066 to ObjectMethods (somehow missed this before). '

mofeiZ added a commit that referenced this pull request Oct 24, 2024

[compiler][ez] Patch hoistability for ObjectMethods

d0c31e9

Extends #31066 to ObjectMethods (somehow missed this before). '

mofeiZ added a commit that referenced this pull request Nov 5, 2024

[compiler][ez] Patch hoistability for ObjectMethods

7ac7b52

Extends #31066 to ObjectMethods (somehow missed this before). '

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler][hir] Only hoist always-accessed PropertyLoads from function decls #31066

[compiler][hir] Only hoist always-accessed PropertyLoads from function decls #31066

mofeiZ commented Sep 25, 2024 •

edited

Loading

vercel bot commented Sep 25, 2024 •

edited

Loading

mofeiZ Oct 1, 2024

mofeiZ Oct 1, 2024

mofeiZ Oct 1, 2024

mofeiZ Oct 1, 2024 •

edited

Loading

mofeiZ Oct 1, 2024

mofeiZ Oct 1, 2024

[compiler][hir] Only hoist always-accessed PropertyLoads from function decls #31066

[compiler][hir] Only hoist always-accessed PropertyLoads from function decls #31066

Conversation

mofeiZ commented Sep 25, 2024 • edited Loading

vercel bot commented Sep 25, 2024 • edited Loading

mofeiZ Oct 1, 2024

Choose a reason for hiding this comment

mofeiZ Oct 1, 2024

Choose a reason for hiding this comment

mofeiZ Oct 1, 2024

Choose a reason for hiding this comment

mofeiZ Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

mofeiZ Oct 1, 2024

Choose a reason for hiding this comment

mofeiZ Oct 1, 2024

Choose a reason for hiding this comment

mofeiZ commented Sep 25, 2024 •

edited

Loading

vercel bot commented Sep 25, 2024 •

edited

Loading

mofeiZ Oct 1, 2024 •

edited

Loading