compiler: Use types to decide which scopes are eligible for merging

[josephsavona]

Stack from ghstack (oldest at bottom):

• compiler: repro for unmerged scopes due to intermediates #29197
• -> compiler: Use types to decide which scopes are eligible for merging #29157
• compiler: Improve merging of memo scopes that invalidate together #29156
• compiler: fixture for suboptimal jsx sibling memo block merging #29155

In MergeReactiveScopesThatInvalidateTogether when deciding which scopes were eligible for merging, we previously looked specifically at the type of the instruction whose lvalue is declaration. So if a scope declaration was t0, we'd look for the instruction where t0 was the lvalue and look at the instruction type to decide if it is eligible for merging.

Here, we use the inferred type instead (now that the inferred types support the same set of types of instructions we looked at before). This allows us to find more cases where scopes can be merged.

[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	May 23, 2024 0:12am

[react-sizebot]

Comparing: bf046e8...6d4f5c8

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.66 kB	6.66 kB	=	1.82 kB	1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	495.71 kB	495.71 kB	=	88.78 kB	88.78 kB
oss-experimental/react-dom/cjs/react-dom.production.js	=	6.67 kB	6.67 kB	=	1.83 kB	1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js	=	500.51 kB	500.51 kB	=	89.47 kB	89.47 kB
facebook-www/ReactDOM-prod.classic.js	=	592.86 kB	592.86 kB	=	104.28 kB	104.28 kB
facebook-www/ReactDOM-prod.modern.js	=	569.08 kB	569.08 kB	=	100.69 kB	100.69 kB
test_utils/ReactAllWarnings.js	Deleted	64.35 kB	0.00 kB	Deleted	16.05 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	64.35 kB	0.00 kB	Deleted	16.05 kB	0.00 kB

Generated by 🚫 dangerJS against 6d4f5c8

[mofeiZ]

Haven't spent time trying to find a case in which this is an issue, but note that this also affects scopes that produce a "always-invalidating" type transitively, from a nested scope:

In this example, it seems that both scopes @0 and @1 are now "eligble for merging". It looks like the inner decl isn't always guaranteed to invalidate on outer_dep changes though. Maybe I'm missing something though

scope @0 deps=[inner_dep, outer_dep] decls=[inner, outer] {
  outer = makeValue(outer_dep);
  scope@1 deps=[inner_dep] decls=[inner] {
    inner = {inner_dep};
  }
  maybeWrite(outer);
}

[josephsavona]

Interesting, i wasn't thinking about the nested case (more on that later) but about the transitive case. Something like: two values get merged into a single scope eg due to interleaving, x depends on a, y depends on b. Then some later value only depends on eg x, but that means it implicitly resets on changes to b as well:

scope @0 deps=[a, b] decls=[x, y] {
  // imagine this
  x = Array [  ]  
  y = Array [  ]
  x.push( a )
  y.push( b ) 
}
scope @1 deps=[y] decls=[z] {
  z = makeValue( y )
}

With this PR we'd now merge these scopes, and z will reset on changes to a even though y doesn't strictly depend on a. But...even if we didn't merge the scopes, y would still reset on changes to a, which would invalidate z anyway. So merging doesn't change the reset.

For the case you pointed out: the only way to have a subsequent scope depend on a value produced by an inner scope (that i can think of) is something like this:

function Component({ a, b, c }) {
  const x = [];
  let y;
  if (a) {
    y = [b];
  }
  x.push(c);

  const z = [y];

  return [z, x];
}

note y is declared outside the if, making it available for the subsequent z = [y] reference. That instruction sees the result of a phi node (due to the reassignment), and we don't currently infer types for phi nodes. So in practice, this case would not yet get merged.

I'll add this as a test case so we can find regressions.

[josephsavona]

Updated to add a test case confirming that the nested case memoizes as expected (ie does not inadvertently merge). I couldn't easily replicate that level of precision with useMemo, so i added a way to disable the non-forget execution in snap. This way we will get a test failure (not just a change to snapshot output) if we regress on the case you pointed out.