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

mofeiZ · 2024-09-23T21:36:30Z

Stack from ghstack (oldest at bottom):

Adds HIR version of PropagateScopeDeps to handle optional chaining.

Internally, this improves memoization on ~4% of compiled files (internal links: 1)

Summarizing the changes in this PR.

CollectOptionalChainDependencies recursively traverses optional blocks down to the base. From the base, we build up a set of baseIdentifier.propertyA?.propertyB mappings.
The tricky bit here is that optional blocks sometimes reference other optional blocks that are not part of the same chain e.g. a(c?.d)?.d. See code + comments in traverseOptionalBlock for how we avoid concatenating unrelated blocks.
Adding optional chains into non-null object calculation.
(Note that marking a?.b as 'non-null' means that a?.b.c is safe to evaluate, not (a?.b).c. Happy to rename this / reword comments accordingly if there's a better term)
This pass is split into two stages. (1) collecting non-null objects by block and (2) propagating non-null objects across blocks. The only significant change here was to (2). We add an extra reduce step X=Reduce(Union(X, Intersect(X_neighbors))) to merge optional and non-optional nodes (e.g. nonNulls={a, a?.b} reduces to {a, a.b})
Adding optional chains into dependency calculation.
This was the trickiest. We need to take the "maximal" property chain as a dependency. Prior to this PR, we avoided taking subpaths e.g. a.b of a.b.c as dependencies by only visiting non-PropertyLoad/LoadLocal instructions. This effectively only recorded the property-path at site-of-use.

Unfortunately, this quite doesn't work for optional chains for a few reasons:
- We would need to skip relevant StoreLocal/Branch terminal instructions (but only those within optional blocks that have been successfully read).
- Given an optional chain, either (1) only a subpath or (2) the entire path can be represented as a PropertyLoad. We cannot directly add the last hoistable optional-block as a dependency as MethodCalls are an edge case e.g. given a?.b.c(), we should depend on a?.b, not a?.b.c
  This means that we add its dependency at either the innermost unhoistable optional-block or when encountering it within its phi-join.
Handle optional chains in DeriveMinimalDependenciesHIR.
This was also a bit tricky to formulate. Ideally, we would avoid a 2^3 case join (cond | uncond cfg, optional | not optional load, access | dependency). This PR attempts to simplify by building two trees
1. First add each hoistable path into a tree containing Optional | NonOptional nodes.
2. Then add each dependency into another tree containing Optional | NonOptional, Access | Dependency nodes, truncating the dependency at the earliest non-hoistable node (i.e. non-matching pair when walking the hoistable tree)

[ghstack-poisoned]

vercel · 2024-09-23T21:36:32Z

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 2, 2024 4:58pm

ghstack-source-id: d57e1d9ad2df9b3747524df4811aaa6cbb4bc304 Pull Request resolved: #31037

mofeiZ · 2024-09-23T21:57:42Z

...ures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-as-memo-dep.expect.md

@@ -0,0 +1,48 @@
+


Fixture output is now exactly the same as the non hir-rewrite version (previously bailed out with CannotPreserveMemoization)

mofeiZ · 2024-09-23T21:59:51Z

...propagate-scope-deps-hir-fork/optional-member-expression-single-with-unconditional.expect.md

@@ -0,0 +1,62 @@
+


Fixture output is now exactly the same as the non hir-rewrite version (previously bailed out with CannotPreserveMemoization)

mofeiZ · 2024-09-23T22:00:06Z

.../fixtures/compiler/propagate-scope-deps-hir-fork/optional-member-expression-single.expect.md

@@ -0,0 +1,63 @@
+


Fixture output is now exactly the same as the non hir-rewrite version (previously bailed out with CannotPreserveMemoization)

mofeiZ · 2024-09-23T22:01:52Z

...-deps-hir-fork/reactive-dependencies-non-optional-properties-inside-optional-chain.expect.md

@@ -16,9 +16,9 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR
 function Component(props) {
  const $ = _c(2);
  let t0;
-  if ($[0] !== props.post.feedback.comments) {
+  if ($[0] !== props.post.feedback.comments?.edges) {


Note that this is an improvement: the non-hir version depends on props.post.feedback.comments (see fixture)

This dependency is correct and hoistable because:
(1) props.post.feedback.comment is inferred to be a reorderable read as it's evaluated unconditionally and there are no early returns / scope-exiting jumps between scope start and the read. This means that props.post.feedback is non-null.

(2) props.post.feedback.comments might be null, but an optional-chain read will not throw

mofeiZ · 2024-09-23T22:05:53Z

...ompiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/conditional-member-expr.expect.md

@@ -31,10 +31,10 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR
 function Component(props) {
  const $ = _c(2);
  let x;
-  if ($[0] !== props.a) {
+  if ($[0] !== props.a?.b) {


Fixture output is now exactly the same as the non hir-rewrite version

mofeiZ · 2024-09-23T22:06:23Z

.../propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain.expect.md

@@ -46,11 +46,11 @@ import { c as _c } from "react/compiler-runtime"; // @enablePropagateDepsInHIR
 function Component(props) {
  const $ = _c(2);
  let x;
-  if ($[0] !== props.a) {
+  if ($[0] !== props.a.b) {


Fixture output is now exactly the same as the non hir-rewrite version

mofeiZ · 2024-09-23T22:06:28Z

...propagate-scope-deps-hir-fork/reduce-reactive-deps/memberexpr-join-optional-chain2.expect.md

  let x;
-  if ($[0] !== props.items) {
+  if ($[0] !== props.items?.length || $[1] !== props.items?.edges) {


Fixture output is now exactly the same as the non hir-rewrite version

mofeiZ · 2024-09-23T22:11:25Z

...gate-scope-deps-hir-fork/reduce-reactive-deps/merge-uncond-optional-chain-and-cond.expect.md

@@ -0,0 +1,92 @@
+


This is an extreme edge case and not code we'd expect any reasonable developer to write. In most cases (a?.b != null ? a.b : DEFAULT), we do want to take a dependency on a?.b.

I found this trying to come up with edge cases that break the current dependency + CFG merging logic. I think it makes sense to error on the side of correctness. After all, we still take a as a dependency if users write a != null ? a.b : DEFAULT, and the same fix (understanding the hoistable != null test expression) works for both. Can be convinced otherwise though!

mofeiZ · 2024-09-23T22:11:52Z

.../fixtures/compiler/propagate-scope-deps-hir-fork/repro-scope-missing-mutable-range.expect.md

@@ -24,14 +24,14 @@ function HomeDiscoStoreItemTileRating(props) {
  const $ = _c(4);
  const item = useFragment();
  let count;
-  if ($[0] !== item) {
+  if ($[0] !== item?.aggregates) {


Fixture output is now exactly the same as the non hir-rewrite version

[ghstack-poisoned]

ghstack-source-id: 62ed6a3ded4274211fc2db5a1dc766d645a47cb6 Pull Request resolved: #31037

[ghstack-poisoned]

ghstack-source-id: e18991566179f1ceecffcdad38e3513688012e81 Pull Request resolved: #31037

[ghstack-poisoned]

Found when writing #31037, summary copied from comments: This is an extreme edge case and not code we'd expect any reasonable developer to write. In most cases e.g. `(a?.b != null ? a.b : DEFAULT)`, we do want to take a dependency on `a?.b`. I found this trying to come up with edge cases that break the current dependency + CFG merging logic. I think it makes sense to error on the side of correctness. After all, we still take `a` as a dependency if users write `a != null ? a.b : DEFAULT`, and the same fix (understanding the `<hoistable> != null` test expression) works for both. Can be convinced otherwise though! ghstack-source-id: cc06afda59f7681e228495f5e35a596c20f875f5 Pull Request resolved: #31035

[ghstack-poisoned]

mofeiZ · 2024-09-30T20:01:30Z

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

@@ -96,17 +118,21 @@ export type BlockInfo = {
 */
 type RootNode = {
  properties: Map<string, PropertyPathNode>;
+  optionalProperties: Map<string, PropertyPathNode>;


properties now holds loads that are not the start of an optional chain (i.e. property in <base>.property). optionalProperties hold properties that start an optional chain (i.e. <base>?.property)

Note that we are still unable to represent expressions like (a?.b).c (an unconditional load from an optional chain). I can't think of a legitimate use of these

hmm, that's totally valid. a could be nullable, but if it's non-null the value is guaranteed to have .b.c

mofeiZ · 2024-09-30T20:04:09Z

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

+      assumedNonNullObjects,
+    });
+    const maybeOptionalChain = hoistableFromOptionals.get(block.id);
+    if (maybeOptionalChain != null) {


This is already computed by collectOptionalChainDependencies to be non-optional loads (i.e. a?.b in a?.b.c), so changes to core logic here are minimal

mofeiZ · 2024-09-30T20:07:34Z

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

+ * If unconditional reads from <base> are hoistable, we can replace all
+ * <base>?.PROPERTY_STRING subpaths with <base>.PROPERTY_STRING
+ */
+function reduceMaybeOptionalChains(


This is the only other significant change in this file. Previous logic was X=Union(X, Intersect(X_neighbors)). Now, we add an extra step X=reduce(Union(X, Intersect(X_neighbors)))

[ghstack-poisoned]

josephsavona

Looks good, this has gotten pretty complicated but that's kind of unavoidable given the domain. The implementation seems good, just a couple comments/questions.

See the comments on the fixture, though, because i'm wondering if there may be bugs given that the actual output doesn't align with your comments about the expected output.

josephsavona · 2024-10-01T16:02:45Z

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

 */
 export function collectHoistablePropertyLoads(
  fn: HIRFunction,
  temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>,
+  hoistableFromOptionals: ReadonlyMap<BlockId, ReactiveScopeDependency>,


Hmm, why only one dep per block?

These map optional-blocks to the path they represent. Each hoistable optional block can represent exactly one path e.g. a?.b.c

josephsavona · 2024-10-01T16:03:57Z

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

@@ -96,17 +118,21 @@ export type BlockInfo = {
 */
 type RootNode = {
  properties: Map<string, PropertyPathNode>;
+  optionalProperties: Map<string, PropertyPathNode>;


hmm, that's totally valid. a could be nullable, but if it's non-null the value is guaranteed to have .b.c

josephsavona · 2024-10-01T17:24:36Z

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

+          nextEntry,
+        );
+      }
+      if (currNode !== original) {


this condition seems like it could be true even if there was no change, ie if every item on the path ended up using the same entry. or does getOrCreatePropertyEntry() guarantee it will reuse the same value if the input was the same?

getOrCreatePropertyEntry() is guaranteed to return the same node on every invocation (it searches a backing tree from root identifier up)

josephsavona · 2024-10-01T21:00:13Z

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

+    const path = maybeTest.instructions.slice(1).map((entry, i) => {
+      const instrVal = entry.value;
+      const prevEntry = maybeTest.instructions[i];
+      if (
+        instrVal.kind === 'PropertyLoad' &&
+        instrVal.object.identifier.id === prevEntry.lvalue.identifier.id
+      ) {
+        return {property: instrVal.property, optional: false};
+      } else {
+        return null;
+      }
+    });
+    if (!arrayNonNulls(path)) {
+      return null;
+    }


nit: this would be more efficient as a for loop: it avoids the array copy from the slice() and allows exiting early for non-PropertyLoad instructions (and avoids the arrayNonNulls iteration too)

josephsavona · 2024-10-01T21:01:58Z

...t-compiler/src/__tests__/fixtures/compiler/infer-sequential-optional-chain-nonnull.expect.md

+### Eval output
+(kind: ok) [[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]]
+[[ (exception in render) TypeError: Cannot read properties of null (reading 'b') ]]
+[[ (exception in render) TypeError: Cannot read properties of undefined (reading 'c') ]]


intentional?

oops, fixed -- thanks for noticing!

josephsavona · 2024-10-01T21:03:21Z

...babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.expect.md

+  // prop1?.value should be hoisted as the dependency of x
+  const x = identity(prop1?.value)?.toString();
+
+  // prop2?.inner.value should be hoisted as the dependency of y


is it expected that we're not actually inferring this as the dep? the actual inferred dep is just prop2?.inner (no .value)

@josephsavona This fixture had different output when toggling @enablePropagateDepsInHIR, so it lives in both propagate-scope-hir-fork and the base fixtures directory. This is the non hir-rewrite version

Will make sure to leave a PR comment next time I add fixtures whose output conflict with comments. Sorry for the confusion!

ahhhh got it

josephsavona · 2024-10-01T21:04:00Z

...babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.expect.md

+  // prop2?.inner.value should be hoisted as the dependency of y
+  const y = identity(prop2?.inner.value)?.toString();
+
+  // prop3 and prop4?.inner should be hoisted as the dependency of z


similar here, the inferred dep is prop4, not prop4?.inner

josephsavona · 2024-10-01T21:04:12Z

...babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nested-optional-chains.expect.md

+  // prop3 and prop4?.inner should be hoisted as the dependency of z
+  const z = prop3?.fn(prop4?.inner.value).toString();
+
+  // prop5 and prop6?.inner should be hoisted as the dependency of zz


similar here too

josephsavona

My main concern was the fixture not reflecting comments which you noted is due to duplicate fixtures. Let's go ahead!

[ghstack-poisoned]

Adds HIR version of `PropagateScopeDeps` to handle optional chaining. Internally, this improves memoization on ~4% of compiled files (internal links: [1](https://www.internalfb.com/intern/paste/P1610406497/)) Summarizing the changes in this PR. 1. `CollectOptionalChainDependencies` recursively traverses optional blocks down to the base. From the base, we build up a set of `baseIdentifier.propertyA?.propertyB` mappings. The tricky bit here is that optional blocks sometimes reference other optional blocks that are *not* part of the same chain e.g. a(c?.d)?.d. See code + comments in `traverseOptionalBlock` for how we avoid concatenating unrelated blocks. 2. Adding optional chains into non-null object calculation. (Note that marking `a?.b` as 'non-null' means that `a?.b.c` is safe to evaluate, *not* `(a?.b).c`. Happy to rename this / reword comments accordingly if there's a better term) This pass is split into two stages. (1) collecting non-null objects by block and (2) propagating non-null objects across blocks. The only significant change here was to (2). We add an extra reduce step `X=Reduce(Union(X, Intersect(X_neighbors)))` to merge optional and non-optional nodes (e.g. nonNulls=`{a, a?.b}` reduces to `{a, a.b}`) 3. Adding optional chains into dependency calculation. This was the trickiest. We need to take the "maximal" property chain as a dependency. Prior to this PR, we avoided taking subpaths e.g. `a.b` of `a.b.c` as dependencies by only visiting non-PropertyLoad/LoadLocal instructions. This effectively only recorded the property-path at site-of-use. Unfortunately, this *quite* doesn't work for optional chains for a few reasons: - We would need to skip relevant `StoreLocal`/`Branch terminal` instructions (but only those within optional blocks that have been successfully read). - Given an optional chain, either (1) only a subpath or (2) the entire path can be represented as a PropertyLoad. We cannot directly add the last hoistable optional-block as a dependency as MethodCalls are an edge case e.g. given a?.b.c(), we should depend on `a?.b`, not `a?.b.c` This means that we add its dependency at either the innermost unhoistable optional-block or when encountering it within its phi-join. 4. Handle optional chains in DeriveMinimalDependenciesHIR. This was also a bit tricky to formulate. Ideally, we would avoid a 2^3 case join (cond | uncond cfg, optional | not optional load, access | dependency). This PR attempts to simplify by building two trees 1. First add each hoistable path into a tree containing `Optional | NonOptional` nodes. 2. Then add each dependency into another tree containing `Optional | NonOptional`, `Access | Dependency` nodes, truncating the dependency at the earliest non-hoistable node (i.e. non-matching pair when walking the hoistable tree) ghstack-source-id: a2170f26280dfbf65a4893d8a658f863a0fd0c88 Pull Request resolved: #31037

Update

bb02868

[ghstack-poisoned]

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

[compiler][wip] Optional chaining for dependencies (HIR rewrite)

5543f49

ghstack-source-id: d57e1d9ad2df9b3747524df4811aaa6cbb4bc304 Pull Request resolved: #31037

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

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

mofeiZ commented Sep 23, 2024

View reviewed changes

Update

0889c9a

[ghstack-poisoned]

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

[compiler][wip] Optional chaining for dependencies (HIR rewrite)

82d0a43

ghstack-source-id: 62ed6a3ded4274211fc2db5a1dc766d645a47cb6 Pull Request resolved: #31037

vercel bot deployed to Preview September 25, 2024 00:33 View deployment

Update

2da1cdc

[ghstack-poisoned]

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

[compiler][wip] Optional chaining for dependencies (HIR rewrite)

6f1d26b

ghstack-source-id: e18991566179f1ceecffcdad38e3513688012e81 Pull Request resolved: #31037

vercel bot deployed to Preview September 25, 2024 22:59 View deployment

mofeiZ changed the title ~~[compiler][wip] Optional chaining for dependencies (HIR rewrite)~~ [compiler] Optional chaining for dependencies (HIR rewrite) Sep 25, 2024

mofeiZ marked this pull request as ready for review September 25, 2024 23:01

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

Update

2c4979e

[ghstack-poisoned]

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

Update

ada4ed9

[ghstack-poisoned]

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

Update

23561fc

[ghstack-poisoned]

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

Update

9119289

[ghstack-poisoned]

mofeiZ commented Sep 30, 2024

View reviewed changes

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

mofeiZ commented Sep 30, 2024

View reviewed changes

Update

48fb60a

[ghstack-poisoned]

vercel bot deployed to Preview October 1, 2024 17:56 View deployment

josephsavona reviewed Oct 1, 2024

View reviewed changes

josephsavona approved these changes Oct 2, 2024

View reviewed changes

Update

2d5f99e

[ghstack-poisoned]

vercel bot deployed to Preview October 2, 2024 16:58 View deployment

mofeiZ merged commit 2d5f99e into gh/mofeiZ/22/base Oct 2, 2024
19 checks passed

mofeiZ deleted the gh/mofeiZ/22/head branch October 2, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mofeiZ commented Sep 23, 2024 •

edited

Loading

vercel bot commented Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024

mofeiZ Sep 23, 2024

mofeiZ Sep 23, 2024

mofeiZ Sep 23, 2024

mofeiZ Sep 23, 2024

mofeiZ Sep 30, 2024

josephsavona Oct 1, 2024

mofeiZ Sep 30, 2024

mofeiZ Sep 30, 2024

josephsavona left a comment

josephsavona Oct 1, 2024

mofeiZ Oct 2, 2024

josephsavona Oct 1, 2024

josephsavona Oct 1, 2024

mofeiZ Oct 2, 2024

josephsavona Oct 1, 2024

josephsavona Oct 1, 2024

mofeiZ Oct 2, 2024

josephsavona Oct 1, 2024

mofeiZ Oct 1, 2024

josephsavona Oct 1, 2024

josephsavona Oct 1, 2024

josephsavona Oct 1, 2024

josephsavona left a comment

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

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

Conversation

mofeiZ commented Sep 23, 2024 • edited Loading

vercel bot commented Sep 23, 2024 • edited Loading

mofeiZ Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

mofeiZ Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

mofeiZ Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

mofeiZ Sep 23, 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

Choose a reason for hiding this comment

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

josephsavona left a comment

Choose a reason for hiding this comment

mofeiZ commented Sep 23, 2024 •

edited

Loading

vercel bot commented Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024 •

edited

Loading

mofeiZ Sep 23, 2024 •

edited

Loading