[compiler] Optimize emission in normal (non-value) blocks #29883

josephsavona · 2024-06-12T23:36:33Z

Stack from ghstack (oldest at bottom):

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization.

This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this.

So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes.

I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes.

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. [ghstack-poisoned]

vercel · 2024-06-12T23:36:35Z

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	Jun 21, 2024 3:19pm

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. ghstack-source-id: 43d0c3b0195ebc69e472aa232e27dec99c2d63dd Pull Request resolved: #29883

...ugin-react-compiler/src/__tests__/fixtures/compiler/fbt/fbt-whitespace-within-text.expect.md

...act-compiler/src/__tests__/fixtures/compiler/fbt/fbt-whitespace-around-param-value.expect.md

...act-compiler/src/__tests__/fixtures/compiler/fbt/fbtparam-with-jsx-element-content.expect.md

josephsavona · 2024-06-12T23:40:15Z

...c/__tests__/fixtures/compiler/inner-memo-value-not-promoted-to-outer-scope-dynamic.expect.md

@@ -24,69 +24,50 @@ function Component(props) {
 ```javascript
 import { c as _c } from "react/compiler-runtime";
 function Component(props) {
-  const $ = _c(15);
+  const $ = _c(7);


josephsavona · 2024-06-12T23:40:49Z

.../babel-plugin-react-compiler/src/__tests__/fixtures/compiler/merge-scopes-callback.expect.md

@@ -27,7 +27,7 @@ import { c as _c } from "react/compiler-runtime"; // @enableInstructionReorderin
 import { useState } from "react";

 function Component() {
-  const $ = _c(6);
+  const $ = _c(4);


josephsavona · 2024-06-12T23:41:07Z

...react-compiler/src/__tests__/fixtures/compiler/merged-scopes-are-valid-effect-deps.expect.md

@@ -32,7 +32,7 @@ import { c as _c } from "react/compiler-runtime"; // @validateMemoizedEffectDepe
 import { useEffect } from "react";

 function Component(props) {
-  const $ = _c(5);
+  const $ = _c(6);


this regresses, need to investigate

josephsavona · 2024-06-12T23:41:38Z

...ests__/fixtures/compiler/repro-duplicate-instruction-from-merge-consecutive-scopes.expect.md

@@ -29,30 +29,39 @@ import { c as _c } from "react/compiler-runtime";
 import { Stringify } from "shared-runtime";

 function Component(t0) {
-  const $ = _c(3);
+  const $ = _c(5);


also regresses

react-sizebot · 2024-06-12T23:42:03Z

Comparing: 0f56841...e557dbb

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	=	497.93 kB	497.93 kB	=	89.26 kB	89.26 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	=	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 e557dbb

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. [ghstack-poisoned]

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. ghstack-source-id: 567b2299fb7d7b22e7ae913041079b5c2ea87bd9 Pull Request resolved: #29883

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. [ghstack-poisoned]

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. ghstack-source-id: ef609261a7f7a0ca823651683157293158a9f271 Pull Request resolved: #29883

compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts

josephsavona · 2024-06-17T23:36:19Z

...-compiler/src/__tests__/fixtures/compiler/meta-isms/repro-cx-assigned-to-temporary.expect.md

-    "styles/light": true,
-    "styles/dark": theme.getTheme() === DARK,
-  });
+  const t0 = theme.getTheme();


i need to debug why the calls are getting reordered here, i suspect that the cx() call is simply getting inlined in codegen but i'm not sure

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. [ghstack-poisoned]

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. ghstack-source-id: 98824627b66f7a43aeaf141c21efddc60c3cc0b3 Pull Request resolved: #29883

josephsavona · 2024-06-18T21:22:03Z

The example from https://x.com/buildsghost/status/1791582752854458791 shows that we still have room to improve though (just picking this as a good example from the community, we've also seen similar cases internally):

const formatter = new Intl.NumberFormat();
function Component() {
  const [count, setCount] = useState(0);
  return (
    <div>
      <button onClick={() => setCount(count - 1)}>-1</button>
      <span>{formatter.format(count)}</span>
      <button onClick={() => setCount(count + 1)}>+1</button>
    </div>
  )
}

With this PR it gets 13 memo slots, on main its 11. But really all of the JSX is dependent on count and can be merged into a single slot. In this example the instruction reordering can only help so much because of the limitations of MergeScopesThatInvalidateTogether. That pass needs to understand transitive dependencies in order to optimize cases like this one.

In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, _then_ try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause _some_ of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, though i still need to double-check all the output changes. [ghstack-poisoned]

mofeiZ · 2024-06-21T17:59:51Z

compiler/packages/babel-plugin-react-compiler/src/Optimization/InstructionReordering.ts

+      emit(env, locals, shared, nextInstructions, operand.identifier.id);
+    }
+    // Anything not emitted yet is globally reorderable
+    for (const id of Array.from(locals.keys()).reverse()) {


Makes sense, emitting reversed instructions ensures that nodes and their dependencies are grouped together.

I'm guessing that it's almost never the case that two instructions share the same dependency as we serialize reads from the same named variables. e.g.

// Here, not only are the StoreLocals of `x` and `y` non-reorderable, but also so // are the `input` references (despite being reads of inferred-frozen values) function useFoo(input) { const x = foo(input); const y = bar(input); return {x, y}; }

Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks. In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures. [ghstack-poisoned]

josephsavona · 2024-06-21T23:32:29Z

oops, accidentally rebased this into the previous PR

Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks. In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures. [ghstack-poisoned]

Note: due to a bad rebase i included #29883 here. Both were stamped so i'm not gonna bother splitting it back up aain. This PR includes two changes: * First, allow `LoadLocal` to be reordered if a) the load occurs after the last write to a variable and b) the LoadLocal lvalue is used exactly once * Uses a more optimal reordering for statement blocks, while keeping the existing approach for expression blocks. In #29863 I tried to find a clean way to share code for emitting instructions between value blocks and regular blocks. The catch is that value blocks have special meaning for their final instruction — that's the value of the block — so reordering can't change the last instruction. However, in finding a clean way to share code for these two categories of code, i also inadvertently reduced the effectiveness of the optimization. This PR updates to use different strategies for these two kinds of blocks: value blocks use the code from #29863 where we first emit all non-reorderable instructions in their original order, then try to emit reorderable values. The reason this is suboptimal, though, is that we want to move instructions closer to their dependencies so that they can invalidate (merge) together. Emitting the reorderable values last prevents this. So for normal blocks, we now emit terminal operands first. This will invariably cause some of the non-reorderable instructions to be emitted, but it will intersperse reoderable instructions in between, right after their dependencies. This maximizes our ability to merge scopes. I think the complexity cost of two strategies is worth the benefit, as evidenced by the reduced memo slots in the fixtures. ghstack-source-id: ad3e516fa474235ced8c5d56f4541d2a7c413608 Pull Request resolved: #29882

josephsavona mentioned this pull request Jun 12, 2024

[compiler] Allow more instruction kinds btw merged scopes #29881

Merged

This was referenced Jun 12, 2024

[compiler] Instruction reordering #29863

Merged

[compiler] Optimize instruction reordering #29882

Merged

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