Skip to content

Commit 3c9fb3e

Browse files
committed
[compiler] Optimize props spread for common cases (#34900)
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. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34900). * __->__ #34900 * #34887 DiffTrain build for [c35f6a3](c35f6a3)
1 parent 7adecb0 commit 3c9fb3e

35 files changed

+200
-89
lines changed

compiled/eslint-plugin-react-hooks/index.js

Lines changed: 114 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40545,7 +40545,7 @@ function inferMutationAliasingEffects(fn, { isFunctionExpression } = {
4054540545
}
4054640546
queue(fn.body.entry, initialState);
4054740547
const hoistedContextDeclarations = findHoistedContextDeclarations(fn);
40548-
const context = new Context$1(isFunctionExpression, fn, hoistedContextDeclarations);
40548+
const context = new Context$1(isFunctionExpression, fn, hoistedContextDeclarations, findNonMutatedDestructureSpreads(fn));
4054940549
let iterationCount = 0;
4055040550
while (queuedStates.size !== 0) {
4055140551
iterationCount++;
@@ -40609,7 +40609,7 @@ function findHoistedContextDeclarations(fn) {
4060940609
return hoisted;
4061040610
}
4061140611
let Context$1 = class Context {
40612-
constructor(isFunctionExpression, fn, hoistedContextDeclarations) {
40612+
constructor(isFunctionExpression, fn, hoistedContextDeclarations, nonMutatingSpreads) {
4061340613
this.internedEffects = new Map();
4061440614
this.instructionSignatureCache = new Map();
4061540615
this.effectInstructionValueCache = new Map();
@@ -40619,6 +40619,7 @@ let Context$1 = class Context {
4061940619
this.isFuctionExpression = isFunctionExpression;
4062040620
this.fn = fn;
4062140621
this.hoistedContextDeclarations = hoistedContextDeclarations;
40622+
this.nonMutatingSpreads = nonMutatingSpreads;
4062240623
}
4062340624
cacheApplySignature(signature, effect, f) {
4062440625
const inner = getOrInsertDefault(this.applySignatureCache, signature, new Map());
@@ -40634,6 +40635,114 @@ let Context$1 = class Context {
4063440635
return interned;
4063540636
}
4063640637
};
40638+
function findNonMutatedDestructureSpreads(fn) {
40639+
const knownFrozen = new Set();
40640+
if (fn.fnType === 'Component') {
40641+
const [props] = fn.params;
40642+
if (props != null && props.kind === 'Identifier') {
40643+
knownFrozen.add(props.identifier.id);
40644+
}
40645+
}
40646+
else {
40647+
for (const param of fn.params) {
40648+
if (param.kind === 'Identifier') {
40649+
knownFrozen.add(param.identifier.id);
40650+
}
40651+
}
40652+
}
40653+
const candidateNonMutatingSpreads = new Map();
40654+
for (const block of fn.body.blocks.values()) {
40655+
if (candidateNonMutatingSpreads.size !== 0) {
40656+
for (const phi of block.phis) {
40657+
for (const operand of phi.operands.values()) {
40658+
const spread = candidateNonMutatingSpreads.get(operand.identifier.id);
40659+
if (spread != null) {
40660+
candidateNonMutatingSpreads.delete(spread);
40661+
}
40662+
}
40663+
}
40664+
}
40665+
for (const instr of block.instructions) {
40666+
const { lvalue, value } = instr;
40667+
switch (value.kind) {
40668+
case 'Destructure': {
40669+
if (!knownFrozen.has(value.value.identifier.id) ||
40670+
!(value.lvalue.kind === InstructionKind.Let ||
40671+
value.lvalue.kind === InstructionKind.Const) ||
40672+
value.lvalue.pattern.kind !== 'ObjectPattern') {
40673+
continue;
40674+
}
40675+
for (const item of value.lvalue.pattern.properties) {
40676+
if (item.kind !== 'Spread') {
40677+
continue;
40678+
}
40679+
candidateNonMutatingSpreads.set(item.place.identifier.id, item.place.identifier.id);
40680+
}
40681+
break;
40682+
}
40683+
case 'LoadLocal': {
40684+
const spread = candidateNonMutatingSpreads.get(value.place.identifier.id);
40685+
if (spread != null) {
40686+
candidateNonMutatingSpreads.set(lvalue.identifier.id, spread);
40687+
}
40688+
break;
40689+
}
40690+
case 'StoreLocal': {
40691+
const spread = candidateNonMutatingSpreads.get(value.value.identifier.id);
40692+
if (spread != null) {
40693+
candidateNonMutatingSpreads.set(lvalue.identifier.id, spread);
40694+
candidateNonMutatingSpreads.set(value.lvalue.place.identifier.id, spread);
40695+
}
40696+
break;
40697+
}
40698+
case 'JsxFragment':
40699+
case 'JsxExpression': {
40700+
break;
40701+
}
40702+
case 'PropertyLoad': {
40703+
break;
40704+
}
40705+
case 'CallExpression':
40706+
case 'MethodCall': {
40707+
const callee = value.kind === 'CallExpression' ? value.callee : value.property;
40708+
if (getHookKind(fn.env, callee.identifier) != null) {
40709+
if (!isRefOrRefValue(lvalue.identifier)) {
40710+
knownFrozen.add(lvalue.identifier.id);
40711+
}
40712+
}
40713+
else {
40714+
if (candidateNonMutatingSpreads.size !== 0) {
40715+
for (const operand of eachInstructionValueOperand(value)) {
40716+
const spread = candidateNonMutatingSpreads.get(operand.identifier.id);
40717+
if (spread != null) {
40718+
candidateNonMutatingSpreads.delete(spread);
40719+
}
40720+
}
40721+
}
40722+
}
40723+
break;
40724+
}
40725+
default: {
40726+
if (candidateNonMutatingSpreads.size !== 0) {
40727+
for (const operand of eachInstructionValueOperand(value)) {
40728+
const spread = candidateNonMutatingSpreads.get(operand.identifier.id);
40729+
if (spread != null) {
40730+
candidateNonMutatingSpreads.delete(spread);
40731+
}
40732+
}
40733+
}
40734+
}
40735+
}
40736+
}
40737+
}
40738+
const nonMutatingSpreads = new Set();
40739+
for (const [key, value] of candidateNonMutatingSpreads) {
40740+
if (key === value) {
40741+
nonMutatingSpreads.add(key);
40742+
}
40743+
}
40744+
return nonMutatingSpreads;
40745+
}
4063740746
function inferParam(param, initialState, paramKind) {
4063840747
const place = param.kind === 'Identifier' ? param : param.place;
4063940748
const value = {
@@ -41888,7 +41997,9 @@ function computeSignatureForInstruction(context, env, instr) {
4188841997
kind: 'Create',
4188941998
into: place,
4189041999
reason: ValueReason.Other,
41891-
value: ValueKind.Mutable,
42000+
value: context.nonMutatingSpreads.has(place.identifier.id)
42001+
? ValueKind.Frozen
42002+
: ValueKind.Mutable,
4189242003
});
4189342004
effects.push({
4189442005
kind: 'Capture',

compiled/facebook-www/REVISION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
adbc32de32bc52f9014cedb5ff5a502be35aff51
1+
c35f6a3041816613e704772ca9dafb26568d9f89
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
adbc32de32bc52f9014cedb5ff5a502be35aff51
1+
c35f6a3041816613e704772ca9dafb26568d9f89

compiled/facebook-www/React-dev.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1478,7 +1478,7 @@ __DEV__ &&
14781478
exports.useTransition = function () {
14791479
return resolveDispatcher().useTransition();
14801480
};
1481-
exports.version = "19.3.0-www-classic-adbc32de-20251017";
1481+
exports.version = "19.3.0-www-classic-c35f6a30-20251017";
14821482
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
14831483
"function" ===
14841484
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-dev.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1478,7 +1478,7 @@ __DEV__ &&
14781478
exports.useTransition = function () {
14791479
return resolveDispatcher().useTransition();
14801480
};
1481-
exports.version = "19.3.0-www-modern-adbc32de-20251017";
1481+
exports.version = "19.3.0-www-modern-c35f6a30-20251017";
14821482
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
14831483
"function" ===
14841484
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-prod.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,4 +606,4 @@ exports.useSyncExternalStore = function (
606606
exports.useTransition = function () {
607607
return ReactSharedInternals.H.useTransition();
608608
};
609-
exports.version = "19.3.0-www-classic-adbc32de-20251017";
609+
exports.version = "19.3.0-www-classic-c35f6a30-20251017";

compiled/facebook-www/React-prod.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -606,4 +606,4 @@ exports.useSyncExternalStore = function (
606606
exports.useTransition = function () {
607607
return ReactSharedInternals.H.useTransition();
608608
};
609-
exports.version = "19.3.0-www-modern-adbc32de-20251017";
609+
exports.version = "19.3.0-www-modern-c35f6a30-20251017";

compiled/facebook-www/React-profiling.classic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ exports.useSyncExternalStore = function (
610610
exports.useTransition = function () {
611611
return ReactSharedInternals.H.useTransition();
612612
};
613-
exports.version = "19.3.0-www-classic-adbc32de-20251017";
613+
exports.version = "19.3.0-www-classic-c35f6a30-20251017";
614614
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
615615
"function" ===
616616
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/React-profiling.modern.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ exports.useSyncExternalStore = function (
610610
exports.useTransition = function () {
611611
return ReactSharedInternals.H.useTransition();
612612
};
613-
exports.version = "19.3.0-www-modern-adbc32de-20251017";
613+
exports.version = "19.3.0-www-modern-c35f6a30-20251017";
614614
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
615615
"function" ===
616616
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

compiled/facebook-www/ReactART-dev.classic.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20371,10 +20371,10 @@ __DEV__ &&
2037120371
(function () {
2037220372
var internals = {
2037320373
bundleType: 1,
20374-
version: "19.3.0-www-classic-adbc32de-20251017",
20374+
version: "19.3.0-www-classic-c35f6a30-20251017",
2037520375
rendererPackageName: "react-art",
2037620376
currentDispatcherRef: ReactSharedInternals,
20377-
reconcilerVersion: "19.3.0-www-classic-adbc32de-20251017"
20377+
reconcilerVersion: "19.3.0-www-classic-c35f6a30-20251017"
2037820378
};
2037920379
internals.overrideHookState = overrideHookState;
2038020380
internals.overrideHookStateDeletePath = overrideHookStateDeletePath;
@@ -20409,7 +20409,7 @@ __DEV__ &&
2040920409
exports.Shape = Shape;
2041020410
exports.Surface = Surface;
2041120411
exports.Text = Text;
20412-
exports.version = "19.3.0-www-classic-adbc32de-20251017";
20412+
exports.version = "19.3.0-www-classic-c35f6a30-20251017";
2041320413
"undefined" !== typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ &&
2041420414
"function" ===
2041520415
typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.registerInternalModuleStop &&

0 commit comments

Comments
 (0)