Skip to content

Commit b028021

Browse files
committed
[compiler] Optimize props spread for common cases
As part of the new inference model we updated to (correctly) treat destructuring spread as creating a new mutable object. This had the unfortunate side-effect of reducing precision on destructuring of props, though: ```js function Component({x, ...rest}) { const z = rest.z; identity(z); return <Stringify x={x} z={z} />; } ``` Memoized as the following, where we don't realize that `z` is actually frozen: ```js function Component(t0) { const $ = _c(6); let x; let z; if ($[0] !== t0) { const { x: t1, ...rest } = t0; x = t1; z = rest.z; identity(z); ... ``` #34341 was our first thought of how to do this (thanks @poteto for exploring this idea!). But during review it became clear that it was a bit more complicated than I had thought. So this PR explores a more conservative alternative. The idea is: * Track known sources of frozen values: component props, hook params, and hook return values. * Find all object spreads where the rvalue is a known frozen value. * Look at how such objects are used, and if they are only used to access properties (PropertyLoad/Destructure), pass to hooks, or pass to jsx then we can be very confident the object is not mutated. We consider any such objects to be frozen, even though technically spread creates a new object. See new fixtures for more examples.
1 parent 38b39be commit b028021

9 files changed

+455
-1
lines changed

compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts

Lines changed: 163 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
Environment,
2020
FunctionExpression,
2121
GeneratedSource,
22+
getHookKind,
2223
HIRFunction,
2324
Hole,
2425
IdentifierId,
@@ -198,6 +199,7 @@ export function inferMutationAliasingEffects(
198199
isFunctionExpression,
199200
fn,
200201
hoistedContextDeclarations,
202+
findNonMutatedDestructureSpreads(fn),
201203
);
202204

203205
let iterationCount = 0;
@@ -287,15 +289,18 @@ class Context {
287289
isFuctionExpression: boolean;
288290
fn: HIRFunction;
289291
hoistedContextDeclarations: Map<DeclarationId, Place | null>;
292+
nonMutatingSpreads: Set<IdentifierId>;
290293

291294
constructor(
292295
isFunctionExpression: boolean,
293296
fn: HIRFunction,
294297
hoistedContextDeclarations: Map<DeclarationId, Place | null>,
298+
nonMutatingSpreads: Set<IdentifierId>,
295299
) {
296300
this.isFuctionExpression = isFunctionExpression;
297301
this.fn = fn;
298302
this.hoistedContextDeclarations = hoistedContextDeclarations;
303+
this.nonMutatingSpreads = nonMutatingSpreads;
299304
}
300305

301306
cacheApplySignature(
@@ -322,6 +327,161 @@ class Context {
322327
}
323328
}
324329

330+
/**
331+
* Finds objects created via ObjectPattern spread destructuring
332+
* (`const {x, ...spread} = ...`) where a) the rvalue is known frozen and
333+
* b) the spread value cannot possibly be directly mutated. The idea is that
334+
* for this set of values, we can treat the spread object as frozen.
335+
*
336+
* The primary use case for this is props spreading:
337+
*
338+
* ```
339+
* function Component({prop, ...otherProps}) {
340+
* const transformedProp = transform(prop, otherProps.foo);
341+
* // pass `otherProps` down:
342+
* return <Foo {...otherProps} prop={transformedProp} />;
343+
* }
344+
* ```
345+
*
346+
* Here we know that since `otherProps` cannot be mutated, we don't have to treat
347+
* it as mutable: `otherProps.foo` only reads a value that must be frozen, so it
348+
* can be treated as frozen too.
349+
*/
350+
function findNonMutatedDestructureSpreads(fn: HIRFunction): Set<IdentifierId> {
351+
const knownFrozen = new Set<IdentifierId>();
352+
if (fn.fnType === 'Component') {
353+
const [props] = fn.params;
354+
if (props != null && props.kind === 'Identifier') {
355+
knownFrozen.add(props.identifier.id);
356+
}
357+
} else {
358+
for (const param of fn.params) {
359+
if (param.kind === 'Identifier') {
360+
knownFrozen.add(param.identifier.id);
361+
}
362+
}
363+
}
364+
365+
// Map of temporaries to identifiers for spread objects
366+
const candidateNonMutatingSpreads = new Map<IdentifierId, IdentifierId>();
367+
for (const block of fn.body.blocks.values()) {
368+
if (candidateNonMutatingSpreads.size !== 0) {
369+
for (const phi of block.phis) {
370+
for (const operand of phi.operands.values()) {
371+
const spread = candidateNonMutatingSpreads.get(operand.identifier.id);
372+
if (spread != null) {
373+
candidateNonMutatingSpreads.delete(spread);
374+
}
375+
}
376+
}
377+
}
378+
for (const instr of block.instructions) {
379+
const {lvalue, value} = instr;
380+
switch (value.kind) {
381+
case 'Destructure': {
382+
if (
383+
!knownFrozen.has(value.value.identifier.id) ||
384+
!(
385+
value.lvalue.kind === InstructionKind.Let ||
386+
value.lvalue.kind === InstructionKind.Const
387+
) ||
388+
value.lvalue.pattern.kind !== 'ObjectPattern'
389+
) {
390+
continue;
391+
}
392+
for (const item of value.lvalue.pattern.properties) {
393+
if (item.kind !== 'Spread') {
394+
continue;
395+
}
396+
candidateNonMutatingSpreads.set(
397+
item.place.identifier.id,
398+
item.place.identifier.id,
399+
);
400+
}
401+
break;
402+
}
403+
case 'LoadLocal': {
404+
const spread = candidateNonMutatingSpreads.get(
405+
value.place.identifier.id,
406+
);
407+
if (spread != null) {
408+
candidateNonMutatingSpreads.set(lvalue.identifier.id, spread);
409+
}
410+
break;
411+
}
412+
case 'StoreLocal': {
413+
const spread = candidateNonMutatingSpreads.get(
414+
value.value.identifier.id,
415+
);
416+
if (spread != null) {
417+
candidateNonMutatingSpreads.set(lvalue.identifier.id, spread);
418+
candidateNonMutatingSpreads.set(
419+
value.lvalue.place.identifier.id,
420+
spread,
421+
);
422+
}
423+
break;
424+
}
425+
case 'JsxFragment':
426+
case 'JsxExpression': {
427+
// Passing objects created with spread to jsx can't mutate them
428+
break;
429+
}
430+
case 'PropertyLoad': {
431+
// Properties must be frozen since the original value was frozen
432+
break;
433+
}
434+
case 'CallExpression':
435+
case 'MethodCall': {
436+
const callee =
437+
value.kind === 'CallExpression' ? value.callee : value.property;
438+
if (getHookKind(fn.env, callee.identifier) != null) {
439+
// Hook calls have frozen arguments, and non-ref returns are frozen
440+
if (!isRefOrRefValue(lvalue.identifier)) {
441+
knownFrozen.add(lvalue.identifier.id);
442+
}
443+
} else {
444+
// Non-hook calls check their operands, since they are potentially mutable
445+
if (candidateNonMutatingSpreads.size !== 0) {
446+
// Otherwise any reference to the spread object itself may mutate
447+
for (const operand of eachInstructionValueOperand(value)) {
448+
const spread = candidateNonMutatingSpreads.get(
449+
operand.identifier.id,
450+
);
451+
if (spread != null) {
452+
candidateNonMutatingSpreads.delete(spread);
453+
}
454+
}
455+
}
456+
}
457+
break;
458+
}
459+
default: {
460+
if (candidateNonMutatingSpreads.size !== 0) {
461+
// Otherwise any reference to the spread object itself may mutate
462+
for (const operand of eachInstructionValueOperand(value)) {
463+
const spread = candidateNonMutatingSpreads.get(
464+
operand.identifier.id,
465+
);
466+
if (spread != null) {
467+
candidateNonMutatingSpreads.delete(spread);
468+
}
469+
}
470+
}
471+
}
472+
}
473+
}
474+
}
475+
476+
const nonMutatingSpreads = new Set<IdentifierId>();
477+
for (const [key, value] of candidateNonMutatingSpreads) {
478+
if (key === value) {
479+
nonMutatingSpreads.add(key);
480+
}
481+
}
482+
return nonMutatingSpreads;
483+
}
484+
325485
function inferParam(
326486
param: Place | SpreadPattern,
327487
initialState: InferenceState,
@@ -2054,7 +2214,9 @@ function computeSignatureForInstruction(
20542214
kind: 'Create',
20552215
into: place,
20562216
reason: ValueReason.Other,
2057-
value: ValueKind.Mutable,
2217+
value: context.nonMutatingSpreads.has(place.identifier.id)
2218+
? ValueKind.Frozen
2219+
: ValueKind.Mutable,
20582220
});
20592221
effects.push({
20602222
kind: 'Capture',
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {identity, Stringify, useIdentity} from 'shared-runtime';
6+
7+
function Component(props) {
8+
const {x, ...rest} = useIdentity(props);
9+
const z = rest.z;
10+
identity(z);
11+
return <Stringify x={x} z={z} />;
12+
}
13+
14+
export const FIXTURE_ENTRYPOINT = {
15+
fn: Component,
16+
params: [{x: 'Hello', z: 'World'}],
17+
};
18+
19+
```
20+
21+
## Code
22+
23+
```javascript
24+
import { c as _c } from "react/compiler-runtime";
25+
import { identity, Stringify, useIdentity } from "shared-runtime";
26+
27+
function Component(props) {
28+
const $ = _c(6);
29+
const t0 = useIdentity(props);
30+
let rest;
31+
let x;
32+
if ($[0] !== t0) {
33+
({ x, ...rest } = t0);
34+
$[0] = t0;
35+
$[1] = rest;
36+
$[2] = x;
37+
} else {
38+
rest = $[1];
39+
x = $[2];
40+
}
41+
const z = rest.z;
42+
identity(z);
43+
let t1;
44+
if ($[3] !== x || $[4] !== z) {
45+
t1 = <Stringify x={x} z={z} />;
46+
$[3] = x;
47+
$[4] = z;
48+
$[5] = t1;
49+
} else {
50+
t1 = $[5];
51+
}
52+
return t1;
53+
}
54+
55+
export const FIXTURE_ENTRYPOINT = {
56+
fn: Component,
57+
params: [{ x: "Hello", z: "World" }],
58+
};
59+
60+
```
61+
62+
### Eval output
63+
(kind: ok) <div>{"x":"Hello","z":"World"}</div>
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import {identity, Stringify, useIdentity} from 'shared-runtime';
2+
3+
function Component(props) {
4+
const {x, ...rest} = useIdentity(props);
5+
const z = rest.z;
6+
identity(z);
7+
return <Stringify x={x} z={z} />;
8+
}
9+
10+
export const FIXTURE_ENTRYPOINT = {
11+
fn: Component,
12+
params: [{x: 'Hello', z: 'World'}],
13+
};
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
2+
## Input
3+
4+
```javascript
5+
import {identity, Stringify} from 'shared-runtime';
6+
7+
function Component({x, ...rest}) {
8+
return <Stringify {...rest} x={x} />;
9+
}
10+
11+
export const FIXTURE_ENTRYPOINT = {
12+
fn: Component,
13+
params: [{x: 'Hello', z: 'World'}],
14+
};
15+
16+
```
17+
18+
## Code
19+
20+
```javascript
21+
import { c as _c } from "react/compiler-runtime";
22+
import { identity, Stringify } from "shared-runtime";
23+
24+
function Component(t0) {
25+
const $ = _c(6);
26+
let rest;
27+
let x;
28+
if ($[0] !== t0) {
29+
({ x, ...rest } = t0);
30+
$[0] = t0;
31+
$[1] = rest;
32+
$[2] = x;
33+
} else {
34+
rest = $[1];
35+
x = $[2];
36+
}
37+
let t1;
38+
if ($[3] !== rest || $[4] !== x) {
39+
t1 = <Stringify {...rest} x={x} />;
40+
$[3] = rest;
41+
$[4] = x;
42+
$[5] = t1;
43+
} else {
44+
t1 = $[5];
45+
}
46+
return t1;
47+
}
48+
49+
export const FIXTURE_ENTRYPOINT = {
50+
fn: Component,
51+
params: [{ x: "Hello", z: "World" }],
52+
};
53+
54+
```
55+
56+
### Eval output
57+
(kind: ok) <div>{"z":"World","x":"Hello"}</div>
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import {identity, Stringify} from 'shared-runtime';
2+
3+
function Component({x, ...rest}) {
4+
return <Stringify {...rest} x={x} />;
5+
}
6+
7+
export const FIXTURE_ENTRYPOINT = {
8+
fn: Component,
9+
params: [{x: 'Hello', z: 'World'}],
10+
};

0 commit comments

Comments
 (0)