(abandoned) [compiler][rewrite] PropagateScopeDeps hir rewrite

[mofeiZ]

Stack from ghstack (oldest at bottom):

• -> (abandoned) [compiler][rewrite] PropagateScopeDeps hir rewrite #30079

Quick background:

Rvalues / temporaries:

In the compiler, unnamed temporaries that represents the evaluation of an expression
In the code snippet below, $1, $2, $3, and $4 are temporaries.

// input
function Component({ bar} ) {
  const x = {a: foo(bar), b: {}};
}
// gets lowered to
[1] $2 = LoadGlobal(global) foo
[2] $3 = LoadLocal bar$1
[3] $4 = Call $2(<unknown> $3)
[4] $5 = Object {  }
[5] $6 = Object { a: $4, b: $5 }
[6] $8 = StoreLocal Const x$7 = $6

The compiler currently treats temporaries and named variables (e.g. x) differently in this pass.

• named variables may be reassigned (in fact, since we're running after LeaveSSA, a single named identifier's IdentifierId may map to multiple Identifier instances -- each with its own scope and mutable range)
• temporaries are replaced with their represented expressions during codegen. This is correct (mostly correct, see [compiler][fixtures] test repros: codegen, alignScope, phis #29878) as we're careful to always lower the correct evaluation semantics. However, since we rewrite reactive scopes entirely (to if/else blocks), we need to track temporaries that a scope produces in ReactiveScope.declarations and later promote them to named variables.
  In the same example, $4, $5, and $6 need to be promoted: $2 ->t0, $5 ->t1, and $6 ->t2.

[1] $2 = LoadGlobal(global) foo
[2] $3 = LoadLocal bar$1
scope 0:
  [3] $4 = Call $2(<unknown> $3)
scope 1:
  [4] $5 = Object {  }
scope 2:
  [5] $6 = Object { a: $4, b: $5 }
[6] $8 = StoreLocal Const x$7 = $6

Dependencies

ReactiveScope.dependencies records the set of (read-only) values that a reactive scope is dependent on. This is currently limited to just variables (named variables from source and promoted temporaries) and property-loads.
All dependencies we record need to be hoistable -- i.e. reordered to just before the ReactiveScope begins. Not all PropertyLoads are hoistable.

In this example, we should not evaluate obj.a.b without before creating x and checking objIsNull.

// reduce-reactive-deps/no-uncond.js
function useFoo({ obj, objIsNull }) {
  const x = [];
  if (isFalse(objIsNull)) {
    x.push(obj.a.b);
  }
  return x;
}

While other memoization strategies with different constraints exist, the current compiler requires that ReactiveScope.dependencies be re-orderable to the beginning of the reactive scope. But.. PropertyLoads from null values will throw TypeError. This means that evaluating hoisted dependencies should throw if and only if the source program throws. (It is also a bug if source throws and compiler output does not throw. See https://github.com/facebook/react-forget/pull/2709)

---

Rough high level overview

1. Pass 1
   Walk over instructions to gather every temporary used outside of its defining scope (same as ReactiveFunction version). These determine the sidemaps we produce, as temporaries used outside of their declaring scopes get promoted to named variables later (and are not considered hoistable rvals).
2. Pass 2 (collectHoistablePropertyLoads)
   Walk over instructions to generate sidemaps:
   a. temporary identifier -> named variable and property path (e.g. $3 -> {obj: props, path: ["a", "b"]})
   b. block -> accessed variables and properties (e.g. bb0 -> [ {obj: props, path: ["a", "b"]} ])
   c. Walk over control flow graph to understand the set of object and property paths that can be read by each basic block. This analysis:
   • relies on post-dominator trees
   • traverses the CFG from entry (producing the set of variables/paths unconditionally evaluated before a block).
   • traverses the CFG from exit (producing the set of variables/paths unconditionally evaluated after a block).
3. Pass 3: (collectDependencies)
   Walks over instructions again to record dependencies and declarations, using the previously produced sidemaps

Will add more fixture tests (although most cases should be covered in reduce-reactive-deps).
Tested by syncing internally and checking compilation output differences (internal link)

---

Followups:

1. Rewrite function expression deps
   This change produces much more optimal output as the compiler now uses the function CFG to understand which variables / paths are assumed to be non-null. However, it may exacerbate this function-expr hoisting bug. A short term fix here is to simply call some form of collectNonNullObjects on every function expression to find hoistable variable / paths. In the longer term, we should refactor out FunctionExpression.deps.

2. Enable optional paths
   (a) don't count optional load temporaries as dependencies (e.g. collectOptionalLoadRValues(...)).
   (b) add optional paths back. This is a bit tricky as we'll want to implement some merging logic for ConditionalAccess | OptionalChain | UnconditionalAccess. In addition, our current optional chain lowering is slightly incorrect / imprecise

[vercel]

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	Jul 10, 2024 11:27pm

[react-sizebot]

Comparing: 6aea169...e0acdd8

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.js	=	6.68 kB	6.68 kB	=	1.83 kB	1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	497.93 kB	497.93 kB	=	89.26 kB	89.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.69 kB	6.69 kB	=	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	=	502.75 kB	502.75 kB	=	89.96 kB	89.96 kB
facebook-www/ReactDOM-prod.classic.js	=	597.17 kB	597.17 kB	=	105.33 kB	105.33 kB
facebook-www/ReactDOM-prod.modern.js	=	571.52 kB	571.52 kB	=	101.27 kB	101.26 kB
test_utils/ReactAllWarnings.js	Deleted	62.88 kB	0.00 kB	Deleted	15.69 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	62.88 kB	0.00 kB	Deleted	15.69 kB	0.00 kB

Generated by 🚫 dangerJS against e0acdd8

[josephsavona]

nit: it's confusing to destructure props and name the identifier props, pick a different name?

[josephsavona]

could we simplify the initial version of this by not supporting this mode?

[mofeiZ]

Sure, happy to add this in a later PR in the stack!

[josephsavona]

The bookkeeping/traversal logic is making it really hard to follow the core logic of this pass. I would have expected that a non-null object analysis could use the dominator tree rather than the walking forward/backwards that's happening here. Did you try using dominators? If yeah, what was the challenge?

[mofeiZ]

From my understanding (please correct if I'm misunderstanding something, my algo knowledge is much weaker than yours), dominator tree would work but misses some cases the non-hir implementation currently covers.

Take this example. Here, we know that it's safe to access a.b on line 5 even though no dominator of its basic block accesses a.b.

0: if(...) {
1:   a.b;
2: } else {
3:   a.b;
4: }
5: // It's safe to take `a.b` as a dependency of scope@0 (i.e. hoist the a.b access)
6: scope @0 {
7:   if (cond) {
8:     // access a.b;
9:   }
10:}

I believe a more precise formation would be that a.b is safe to read in $bb_i$ if any of the following hold:

• $bb_i$ itself accesses a.b
• a.b is safe to access in every pred of a.b
• a.b is safe to access in every successor of a.b

Basic block cycles can be handled by filtering / dropping the offending block (e.g. reducing x = x && y && ... to x = y && ...)

[josephsavona]

Gotcha. That makes sense. I suspect this can still be simplified - in particular, i had to remap the variable names in my head to help keep track of things. See comments for the most unclear cases.

One thing that strikes me about this algorithm is that it isn't accurate in the sense that it doesn't account for the actual live range of values. During the initial collectNodes stage we account for mutable ranges when deciding if a property access can locally be considered to be non-nullable. But then in the deriveNonNull phase we don't have access to the mutable ranges anymore. AFAICT, if you have:

bb0:
  let x = null
  if cond then bb1 else bb2
bb1:
  x = {}
  goto bb3
bb2:
  ...
  goto bb3:
bb3:
  x.foo.bar

this algorithm will infer x.foo.bar as non-nullable for bb0, via the intersection of bb1 & bb2 which in turn comes from bb3. So the results are actually inaccurate, but that's okay because of how we filter out dependencies in PropagateScopeDeps.

This isn't necessarily a blocker, but it isn't great to have a pass that returns results that are technically incorrect — it's very easy to accidentally use them in a different way later, w/o realizing that important context. Thoughts on this, and whether we could make the results guaranteed accurate?

[josephsavona]

This abstraction kind of divides up when processing happens (scope entry/exit ends up on the side) separate from processing the block itself. Just feels weird, for lack of a better word, and a bit hard to follow.

At least for me, it tends to be easier to follow something where there's a simple for loop over the blocks in standard (RPO) order, and we do stuff for each block. Maybe pulling this out into a side-table, that would allow looking up scope start/exit per block, would be easier to follow?

[josephsavona]

Ok i spent a bunch of time reading though. There's a ton here, but a lot of the core logic is retained from the existing implementation which makes it a bit easier. See comments. In particular, the note about updating our feature flag setup — it's time to drop the non-HIR based versions of older passes so that we can more easily compare the results of this new pass, and fall back quickly if necessary.

[josephsavona]

does activeScopes need to be an array? could it be a Set instead to make this faster/clearer? or expose an API like isScopeActive(scope) and currentScope(): Scope | null

[josephsavona]

it's intentional that we're not visiting lvalues right?

[josephsavona]

i've mentioned this on previous PRs, but putting the most important function at the bottom makes it really tedious to get back to when reading.

[josephsavona]

why not just store the Block itself, which has the preds on it already?

[josephsavona]

nit: name this more clearly? nonNullablesByBlock or similar?

[josephsavona]

👍

[josephsavona]

:-)

[josephsavona]

Gotcha. That makes sense. I suspect this can still be simplified - in particular, i had to remap the variable names in my head to help keep track of things. See comments for the most unclear cases.

One thing that strikes me about this algorithm is that it isn't accurate in the sense that it doesn't account for the actual live range of values. During the initial collectNodes stage we account for mutable ranges when deciding if a property access can locally be considered to be non-nullable. But then in the deriveNonNull phase we don't have access to the mutable ranges anymore. AFAICT, if you have:

bb0:
  let x = null
  if cond then bb1 else bb2
bb1:
  x = {}
  goto bb3
bb2:
  ...
  goto bb3:
bb3:
  x.foo.bar

this algorithm will infer x.foo.bar as non-nullable for bb0, via the intersection of bb1 & bb2 which in turn comes from bb3. So the results are actually inaccurate, but that's okay because of how we filter out dependencies in PropagateScopeDeps.

This isn't necessarily a blocker, but it isn't great to have a pass that returns results that are technically incorrect — it's very easy to accidentally use them in a different way later, w/o realizing that important context. Thoughts on this, and whether we could make the results guaranteed accurate?

[josephsavona]

maybe we should translate nodes into a Map keyed by scope earlier?

[josephsavona]

Given that enableReactiveScopesInHIR has been enabled by default for some time with strong validation of the results, i think as a precursor to this diff we should promote enableReactiveScopesInHIR to stable, removing the alternative code path. Let's repurpose the feature flag for the remainder of the conversion, including this one.

Basically, it would be nice to just compare the old/new PropagateScopeDeps behavior without also including all the other bugs from non-enableReactiveScopesInHIR mode.

[josephsavona]

See previous comments, also from the fixtures it looks like there is an issue with the non-null inference

[josephsavona]

hmm this seems wrong, props.value is unconditionally accessed and should be retained as the dep

[mofeiZ]

Good catch! This is a bit of a weird one. The reason we're not inferring props.value as unconditionally accessed is because we check mutable ranges before adding to assumedNonNullObjects. This is to avoid recording loads within an object's mutable range (e.g. read(x.y.z); x.y = null;)

// in function collectNodes
      if (value.kind === "PropertyLoad") {
        const propertyNode = c.declareProperty(...);
        if ( !isMutable(instr, value.object))  // <----- this line
          let curr = propertyNode.parent;
          while (curr != null) {
            assumedNonNullObjects.add(curr);
            curr = curr.parent;
          }
        }

The edge case here is that we're inferring a mutable range for props due to it being directly aliased into a context variable, which gets captured / mutated within a lambda.

[josephsavona]

same here

[josephsavona]

nice

[josephsavona]

nice, so we know that props is non-null and therefore can hoist the props.a, even though that property load is optional