[compiler][rewrite] Patch logic for aligning scopes to non-value blocks #29891

mofeiZ · 2024-06-13T20:10:24Z

Stack from ghstack (oldest at bottom):

Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g.

// source
a();
if (...) {
  b();
}
c();

// HIR
bb0:
a()
if test=... consequent=bb1 fallthrough=bb2

bb1:
b()
goto bb2

bb2:
c()

// AlignReactiveScopesToBlockScopesHIR nodes
Root node (maps to both bb0 and bb2)
  |- bb1
  |- ...

There are two issues with the existing implementation:

Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin within the block-fallthrough range.

\# This case gets handled correctly
         ┌──────────────┐
         │              │
         block start    block end

scope start     scope end
│               │
└───────────────┘

\# But not this one!
┌──────────────┐
│              │
block start    block end

          scope start     scope end
          │               │
          └───────────────┘

Only scopes that are directly used by a block is considered. See the align-scopes-nested-block-structure fixture for details.

[ghstack-poisoned]

vercel · 2024-06-13T20:10:26Z

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 25, 2024 8:20pm

ghstack-source-id: 18fa632d430e36b62a103de48475d03fac4a915b Pull Request resolved: #29891

react-sizebot · 2024-06-13T20:17:28Z

Comparing: 133ada7...baf7098

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	+0.05%	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.10 kB	597.10 kB	=	105.31 kB	105.31 kB
facebook-www/ReactDOM-prod.modern.js	=	571.44 kB	571.44 kB	=	101.24 kB	101.24 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 baf7098

…alue blocks" [ghstack-poisoned]

Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. ghstack-source-id: d9051284cec15caa1e0a6840cd20ead7acff122a Pull Request resolved: #29891

…-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]

Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. ghstack-source-id: d9051284cec15caa1e0a6840cd20ead7acff122a Pull Request resolved: #29891

…-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]

Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. ghstack-source-id: d9051284cec15caa1e0a6840cd20ead7acff122a Pull Request resolved: #29891

mofeiZ · 2024-06-14T00:58:55Z

Tested by syncing internally. See that we previously had many bailouts in P1413569380. After syncing this change, there are no bailouts of this category in P1413721911. Also observe that no compilation output changed (P1414504972)

josephsavona · 2024-06-14T01:14:19Z

Awesome, thanks for the thorough testing! I’ll review in the morning

josephsavona · 2024-06-14T15:49:23Z

...plugin-react-compiler/src/__tests__/fixtures/compiler/align-scopes-nested-block-structure.ts

@@ -41,7 +38,7 @@
 *  │return s;  │◄──┘
 *  └───────────┘


important question: what did you use to make this diagram?

I used https://asciiflow.com with a little manual editing!

josephsavona

I need to take another read through this to internalize and might have more feedback, but let's ship given the thorough testing.

josephsavona · 2024-06-14T15:52:26Z

...ckages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts

+function retainWhere_Set<T>(
+  items: Set<T>,
+  predicate: (item: T) => boolean
+): void {
+  for (const item of items) {
+    if (!predicate(item)) {
+      items.delete(item);
+    }
+  }
+}


nit: let's move to utils

josephsavona · 2024-06-14T15:56:46Z

...ckages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts

-        description: `No node for block bb${block.id} (${block.kind})`,
-      });
+    const startingId = block.instructions[0]?.id ?? block.terminal.id;
+    retainWhere_Set(activeScopes, (scope) => scope.range.end >= startingId);


This should be >, right? End is exclusive. I think this probably happens to work just because any mutating instruction within the inner blocks will be followed by a terminal successor s.t. it doesn't matter, but it's more technically correct to use > here.

josephsavona · 2024-06-14T17:04:34Z

...ckages/babel-plugin-react-compiler/src/ReactiveScopes/AlignReactiveScopesToBlockScopesHIR.ts

-    id: makeInstructionId(0),
-  };
-  blockNodes.set(fn.body.entry, rootNode);
+  const activeInnerBlockRanges: Array<{


The naming of this was confusing, since it's really tracking outer block -> fallthrough ranges. We use this to extend the range of inner blocks, but it doesn't store inner block ranges. Let's rename?

…-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]

josephsavona

Nice job, the algorithm makes sense to me, especially w the renaming to clarify intent. Tests look great but one question on a case of missing memoization (?)

josephsavona · 2024-06-24T23:25:43Z

...ler/src/__tests__/fixtures/compiler/align-scopes-trycatch-nested-overlapping-range.expect.md

+  try {
+    let thing = null;
+    if (cond) {
+      thing = makeObject_Primitives();


just curious, why do we lose memoization on this?

Ahh memoization was pruned because we didn't return item. Just updated the fixture to return item to avoid pruning!

…-value blocks" Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. [ghstack-poisoned]

Our previous logic for aligning scopes to block scopes constructs a tree of block and scope nodes. We ensured that blocks always mapped to the same node as their fallthroughs. e.g. ```js // source a(); if (...) { b(); } c(); // HIR bb0: a() if test=... consequent=bb1 fallthrough=bb2 bb1: b() goto bb2 bb2: c() // AlignReactiveScopesToBlockScopesHIR nodes Root node (maps to both bb0 and bb2) |- bb1 |- ... ``` There are two issues with the existing implementation: 1. Only scopes that overlap with the beginning of a block are aligned correctly. This is because the traversal does not store information about the block-fallthrough pair for scopes that begin *within* the block-fallthrough range. ``` \# This case gets handled correctly ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ \# But not this one! ┌──────────────┐ │ │ block start block end scope start scope end │ │ └───────────────┘ ``` 2. Only scopes that are directly used by a block is considered. See the `align-scopes-nested-block-structure` fixture for details. ghstack-source-id: 327dec5019483666f81c8156ac0c666ccad511b3 Pull Request resolved: #29891

[compiler][draft] Patch logic for aligning scopes to non-value blocks

2d6a434

[ghstack-poisoned]

mofeiZ mentioned this pull request Jun 13, 2024

[compiler][fixtures] test repros: codegen, alignScope, phis #29878

Merged

mofeiZ added a commit that referenced this pull request Jun 13, 2024

[compiler][draft] Patch logic for aligning scopes to non-value blocks

2162d08

ghstack-source-id: 18fa632d430e36b62a103de48475d03fac4a915b Pull Request resolved: #29891

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

vercel bot deployed to Preview June 13, 2024 20:15 View deployment

Update on "[compiler][draft] Patch logic for aligning scopes to non-v…

1ba7219

…alue blocks" [ghstack-poisoned]

mofeiZ changed the title ~~[compiler][draft] Patch logic for aligning scopes to non-value blocks~~ [compiler][rewrite] Patch logic for aligning scopes to non-value blocks Jun 14, 2024

mofeiZ marked this pull request as ready for review June 14, 2024 00:34

vercel bot deployed to Preview June 14, 2024 00:38 View deployment

josephsavona reviewed Jun 14, 2024

View reviewed changes

josephsavona approved these changes Jun 14, 2024

View reviewed changes

josephsavona reviewed Jun 14, 2024

View reviewed changes

josephsavona approved these changes Jun 24, 2024

View reviewed changes

vercel bot deployed to Preview June 24, 2024 23:30 View deployment

vercel bot deployed to Preview June 25, 2024 20:20 View deployment

mofeiZ merged commit baf7098 into gh/mofeiZ/2/base Jun 25, 2024
36 checks passed

mofeiZ deleted the gh/mofeiZ/2/head branch June 25, 2024 21:00

eps1lon mentioned this pull request Jul 16, 2024

Update React from dfd30974ab-20240613 to 01172397-20240716 vercel/next.js#67810

Closed

This was referenced Jul 25, 2024

Update React from 6230622a1a-20240610 to 76002254-20240724 vercel/next.js#68149

Merged

Update React from 6230622a1a-20240610 to 3208e73e-20240730 vercel/next.js#68330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler][rewrite] Patch logic for aligning scopes to non-value blocks #29891

[compiler][rewrite] Patch logic for aligning scopes to non-value blocks #29891

mofeiZ commented Jun 13, 2024 •

edited

Loading

vercel bot commented Jun 13, 2024 •

edited

Loading

react-sizebot commented Jun 13, 2024 •

edited

Loading

mofeiZ commented Jun 14, 2024

josephsavona commented Jun 14, 2024

josephsavona Jun 14, 2024 •

edited

Loading

mofeiZ Jun 14, 2024

josephsavona left a comment

josephsavona Jun 14, 2024

josephsavona Jun 14, 2024

josephsavona Jun 14, 2024

josephsavona left a comment

josephsavona Jun 24, 2024

mofeiZ Jun 25, 2024

		@@ -41,7 +38,7 @@
		* │return s; │◄──┘
		* └───────────┘

[compiler][rewrite] Patch logic for aligning scopes to non-value blocks #29891

[compiler][rewrite] Patch logic for aligning scopes to non-value blocks #29891

Conversation

mofeiZ commented Jun 13, 2024 • edited Loading

vercel bot commented Jun 13, 2024 • edited Loading

react-sizebot commented Jun 13, 2024 • edited Loading

Critical size changes

Significant size changes

mofeiZ commented Jun 14, 2024

josephsavona commented Jun 14, 2024

josephsavona Jun 14, 2024 • edited Loading

Choose a reason for hiding this comment

mofeiZ Jun 14, 2024

Choose a reason for hiding this comment

josephsavona left a comment

Choose a reason for hiding this comment

josephsavona Jun 14, 2024

Choose a reason for hiding this comment

josephsavona Jun 14, 2024

Choose a reason for hiding this comment

josephsavona Jun 14, 2024

Choose a reason for hiding this comment

josephsavona left a comment

Choose a reason for hiding this comment

josephsavona Jun 24, 2024

Choose a reason for hiding this comment

mofeiZ Jun 25, 2024

Choose a reason for hiding this comment

mofeiZ commented Jun 13, 2024 •

edited

Loading

vercel bot commented Jun 13, 2024 •

edited

Loading

react-sizebot commented Jun 13, 2024 •

edited

Loading

josephsavona Jun 14, 2024 •

edited

Loading