Skip to content

Commit ddf8bc3

Browse files
authored
[compiler] Improve merging of scopes that invalidate together (#34049)
We try to merge consecutive reactive scopes that will always invalidate together, but there's one common case that isn't handled. ```js const y = [[x]]; ``` Here we'll create two consecutive scopes for the inner and outer array expressions. Because the input to the second scope is a temporary, they'll merge into one scope. But if we name the inner array, the merging stops: ```js const array = [x]; const y = [array]; ``` This is because the merging logic checks if all the dependencies of the second scope are outputs of the first scope, but doesn't account for renaming due to LoadLocal/StoreLocal. The fix is to track these temporaries. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34049). * __->__ #34049 * #34047 * #34044
1 parent 0860b9c commit ddf8bc3

File tree

48 files changed

+401
-733
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

48 files changed

+401
-733
lines changed

compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/MergeReactiveScopesThatInvalidateTogether.ts

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ class FindLastUsageVisitor extends ReactiveFunctionVisitor<void> {
119119

120120
class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | null> {
121121
lastUsage: Map<DeclarationId, InstructionId>;
122+
temporaries: Map<DeclarationId, DeclarationId> = new Map();
122123

123124
constructor(lastUsage: Map<DeclarationId, InstructionId>) {
124125
super();
@@ -215,6 +216,12 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
215216
current.lvalues.add(
216217
instr.instruction.lvalue.identifier.declarationId,
217218
);
219+
if (instr.instruction.value.kind === 'LoadLocal') {
220+
this.temporaries.set(
221+
instr.instruction.lvalue.identifier.declarationId,
222+
instr.instruction.value.place.identifier.declarationId,
223+
);
224+
}
218225
}
219226
break;
220227
}
@@ -236,6 +243,13 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
236243
)) {
237244
current.lvalues.add(lvalue.identifier.declarationId);
238245
}
246+
this.temporaries.set(
247+
instr.instruction.value.lvalue.place.identifier
248+
.declarationId,
249+
this.temporaries.get(
250+
instr.instruction.value.value.identifier.declarationId,
251+
) ?? instr.instruction.value.value.identifier.declarationId,
252+
);
239253
} else {
240254
log(
241255
`Reset scope @${current.block.scope.id} from StoreLocal in [${instr.instruction.id}]`,
@@ -260,7 +274,7 @@ class Transform extends ReactiveFunctionTransform<ReactiveScopeDependencies | nu
260274
case 'scope': {
261275
if (
262276
current !== null &&
263-
canMergeScopes(current.block, instr) &&
277+
canMergeScopes(current.block, instr, this.temporaries) &&
264278
areLValuesLastUsedByScope(
265279
instr.scope,
266280
current.lvalues,
@@ -426,6 +440,7 @@ function areLValuesLastUsedByScope(
426440
function canMergeScopes(
427441
current: ReactiveScopeBlock,
428442
next: ReactiveScopeBlock,
443+
temporaries: Map<DeclarationId, DeclarationId>,
429444
): boolean {
430445
// Don't merge scopes with reassignments
431446
if (
@@ -465,20 +480,27 @@ function canMergeScopes(
465480
(next.scope.dependencies.size !== 0 &&
466481
[...next.scope.dependencies].every(
467482
dep =>
483+
dep.path.length === 0 &&
468484
isAlwaysInvalidatingType(dep.identifier.type) &&
469485
Iterable_some(
470486
current.scope.declarations.values(),
471487
decl =>
472-
decl.identifier.declarationId === dep.identifier.declarationId,
488+
decl.identifier.declarationId === dep.identifier.declarationId ||
489+
decl.identifier.declarationId ===
490+
temporaries.get(dep.identifier.declarationId),
473491
),
474492
))
475493
) {
476494
log(` outputs of prev are input to current`);
477495
return true;
478496
}
479497
log(` cannot merge scopes:`);
480-
log(` ${printReactiveScopeSummary(current.scope)}`);
481-
log(` ${printReactiveScopeSummary(next.scope)}`);
498+
log(
499+
` ${printReactiveScopeSummary(current.scope)} ${[...current.scope.declarations.values()].map(decl => decl.identifier.declarationId)}`,
500+
);
501+
log(
502+
` ${printReactiveScopeSummary(next.scope)} ${[...next.scope.dependencies].map(dep => `${dep.identifier.declarationId} ${temporaries.get(dep.identifier.declarationId) ?? dep.identifier.declarationId}`)}`,
503+
);
482504
return false;
483505
}
484506

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-at-closure.expect.md

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ function Component(props) {
1919
```javascript
2020
import { c as _c } from "react/compiler-runtime";
2121
function Component(props) {
22-
const $ = _c(7);
22+
const $ = _c(5);
2323
let t0;
2424
if ($[0] !== props.x) {
2525
t0 = foo(props.x);
@@ -31,26 +31,19 @@ function Component(props) {
3131
const x = t0;
3232
let t1;
3333
if ($[2] !== props || $[3] !== x) {
34-
t1 = function () {
34+
const fn = function () {
3535
const arr = [...bar(props)];
3636
return arr.at(x);
3737
};
38+
39+
t1 = fn();
3840
$[2] = props;
3941
$[3] = x;
4042
$[4] = t1;
4143
} else {
4244
t1 = $[4];
4345
}
44-
const fn = t1;
45-
let t2;
46-
if ($[5] !== fn) {
47-
t2 = fn();
48-
$[5] = fn;
49-
$[6] = t2;
50-
} else {
51-
t2 = $[6];
52-
}
53-
const fnResult = t2;
46+
const fnResult = t1;
5447
return fnResult;
5548
}
5649

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-captures-receiver-noAlias.expect.md

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -23,34 +23,18 @@ export const FIXTURE_ENTRYPOINT = {
2323
```javascript
2424
import { c as _c } from "react/compiler-runtime";
2525
function Component(props) {
26-
const $ = _c(6);
26+
const $ = _c(2);
2727
let t0;
2828
if ($[0] !== props.a) {
29-
t0 = { a: props.a };
29+
const item = { a: props.a };
30+
const items = [item];
31+
t0 = items.map(_temp);
3032
$[0] = props.a;
3133
$[1] = t0;
3234
} else {
3335
t0 = $[1];
3436
}
35-
const item = t0;
36-
let t1;
37-
if ($[2] !== item) {
38-
t1 = [item];
39-
$[2] = item;
40-
$[3] = t1;
41-
} else {
42-
t1 = $[3];
43-
}
44-
const items = t1;
45-
let t2;
46-
if ($[4] !== items) {
47-
t2 = items.map(_temp);
48-
$[4] = items;
49-
$[5] = t2;
50-
} else {
51-
t2 = $[5];
52-
}
53-
const mapped = t2;
37+
const mapped = t0;
5438
return mapped;
5539
}
5640
function _temp(item_0) {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-noAlias-escaping-function.expect.md

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,26 +21,18 @@ export const FIXTURE_ENTRYPOINT = {
2121
```javascript
2222
import { c as _c } from "react/compiler-runtime";
2323
function Component(props) {
24-
const $ = _c(4);
24+
const $ = _c(2);
2525
const f = _temp;
2626
let t0;
2727
if ($[0] !== props.items) {
28-
t0 = [...props.items].map(f);
28+
const x = [...props.items].map(f);
29+
t0 = [x, f];
2930
$[0] = props.items;
3031
$[1] = t0;
3132
} else {
3233
t0 = $[1];
3334
}
34-
const x = t0;
35-
let t1;
36-
if ($[2] !== x) {
37-
t1 = [x, f];
38-
$[2] = x;
39-
$[3] = t1;
40-
} else {
41-
t1 = $[3];
42-
}
43-
return t1;
35+
return t0;
4436
}
4537
function _temp(item) {
4638
return item;

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-arrow-function-1.expect.md

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,27 +23,19 @@ export const FIXTURE_ENTRYPOINT = {
2323
```javascript
2424
import { c as _c } from "react/compiler-runtime";
2525
function component(a) {
26-
const $ = _c(4);
26+
const $ = _c(2);
2727
let t0;
2828
if ($[0] !== a) {
29-
t0 = { a };
29+
const z = { a };
30+
t0 = () => {
31+
console.log(z);
32+
};
3033
$[0] = a;
3134
$[1] = t0;
3235
} else {
3336
t0 = $[1];
3437
}
35-
const z = t0;
36-
let t1;
37-
if ($[2] !== z) {
38-
t1 = () => {
39-
console.log(z);
40-
};
41-
$[2] = z;
42-
$[3] = t1;
43-
} else {
44-
t1 = $[3];
45-
}
46-
const x = t1;
38+
const x = t0;
4739
return x;
4840
}
4941

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-1.expect.md

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,27 +23,19 @@ export const FIXTURE_ENTRYPOINT = {
2323
```javascript
2424
import { c as _c } from "react/compiler-runtime";
2525
function component(a) {
26-
const $ = _c(4);
26+
const $ = _c(2);
2727
let t0;
2828
if ($[0] !== a) {
29-
t0 = { a };
29+
const z = { a };
30+
t0 = function () {
31+
console.log(z);
32+
};
3033
$[0] = a;
3134
$[1] = t0;
3235
} else {
3336
t0 = $[1];
3437
}
35-
const z = t0;
36-
let t1;
37-
if ($[2] !== z) {
38-
t1 = function () {
39-
console.log(z);
40-
};
41-
$[2] = z;
42-
$[3] = t1;
43-
} else {
44-
t1 = $[3];
45-
}
46-
const x = t1;
38+
const x = t0;
4739
return x;
4840
}
4941

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-function-runs-inference.expect.md

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -22,35 +22,19 @@ export const FIXTURE_ENTRYPOINT = {
2222
import { c as _c } from "react/compiler-runtime";
2323
import { Stringify } from "shared-runtime";
2424
function Component(t0) {
25-
const $ = _c(6);
25+
const $ = _c(2);
2626
const { a } = t0;
2727
let t1;
2828
if ($[0] !== a) {
29-
t1 = { a };
29+
const z = { a };
30+
const p = () => <Stringify>{z}</Stringify>;
31+
t1 = p();
3032
$[0] = a;
3133
$[1] = t1;
3234
} else {
3335
t1 = $[1];
3436
}
35-
const z = t1;
36-
let t2;
37-
if ($[2] !== z) {
38-
t2 = () => <Stringify>{z}</Stringify>;
39-
$[2] = z;
40-
$[3] = t2;
41-
} else {
42-
t2 = $[3];
43-
}
44-
const p = t2;
45-
let t3;
46-
if ($[4] !== p) {
47-
t3 = p();
48-
$[4] = p;
49-
$[5] = t3;
50-
} else {
51-
t3 = $[5];
52-
}
53-
return t3;
37+
return t1;
5438
}
5539

5640
export const FIXTURE_ENTRYPOINT = {

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-variable-in-nested-block.expect.md

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -25,27 +25,19 @@ export const FIXTURE_ENTRYPOINT = {
2525
```javascript
2626
import { c as _c } from "react/compiler-runtime";
2727
function component(a) {
28-
const $ = _c(4);
28+
const $ = _c(2);
2929
let t0;
3030
if ($[0] !== a) {
31-
t0 = { a };
31+
const z = { a };
32+
t0 = function () {
33+
console.log(z);
34+
};
3235
$[0] = a;
3336
$[1] = t0;
3437
} else {
3538
t0 = $[1];
3639
}
37-
const z = t0;
38-
let t1;
39-
if ($[2] !== z) {
40-
t1 = function () {
41-
console.log(z);
42-
};
43-
$[2] = z;
44-
$[3] = t1;
45-
} else {
46-
t1 = $[3];
47-
}
48-
const x = t1;
40+
const x = t0;
4941
return x;
5042
}
5143

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/capturing-variable-in-nested-function.expect.md

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,29 +25,21 @@ export const FIXTURE_ENTRYPOINT = {
2525
```javascript
2626
import { c as _c } from "react/compiler-runtime";
2727
function component(a) {
28-
const $ = _c(4);
28+
const $ = _c(2);
2929
let t0;
3030
if ($[0] !== a) {
31-
t0 = { a };
32-
$[0] = a;
33-
$[1] = t0;
34-
} else {
35-
t0 = $[1];
36-
}
37-
const z = t0;
38-
let t1;
39-
if ($[2] !== z) {
40-
t1 = function () {
31+
const z = { a };
32+
t0 = function () {
4133
(function () {
4234
console.log(z);
4335
})();
4436
};
45-
$[2] = z;
46-
$[3] = t1;
37+
$[0] = a;
38+
$[1] = t0;
4739
} else {
48-
t1 = $[3];
40+
t0 = $[1];
4941
}
50-
const x = t1;
42+
const x = t0;
5143
return x;
5244
}
5345

0 commit comments

Comments
 (0)