Memoize arrays/objects created with destructuring spread
We were treating Destructuring as if it could never allocate and therefore didn't have to be memoized. That's only true if there are no rest spreads though. This PR teaches the compiler to treat rest spreads differently for scoping and memoization purposes, fixing the newly added test case and some existing bugs.
Joe Savona committed
Mar 9, 2023 at 12:44 UTC
d376cf1e378bfab8dc2ec11dac992863eb79eaa0
6 files changed
+235
-77
compiler/forget/src/HIR/visitors.ts
+28
@@ -185,6 +185,34 @@ export function* eachInstructionValueOperand(
185
}
186
}
187
188
+export function doesPatternContainSpreadElement(pattern: Pattern): boolean {
189
+ switch (pattern.kind) {
190
+ case "ArrayPattern": {
191
+ for (const item of pattern.items) {
192
+ if (item.kind === "Spread") {
193
+ return true;
194
+ }
195
+ }
196
+ break;
197
+ }
198
+ case "ObjectPattern": {
199
+ for (const property of pattern.properties) {
200
+ if (property.kind === "Spread") {
201
+ return true;
202
+ }
203
+ }
204
+ break;
205
+ }
206
+ default: {
207
+ assertExhaustive(
208
+ pattern,
209
+ `Unexpected pattern kind '${(pattern as any).kind}'`
210
+ );
211
+ }
212
+ }
213
+ return false;
214
+}
215
+
216
export function* eachPatternOperand(pattern: Pattern): Iterable<Place> {
217
switch (pattern.kind) {
218
case "ArrayPattern": {
compiler/forget/src/ReactiveScopes/InferReactiveScopeVariables.ts
+8
-2
@@ -15,7 +15,11 @@ import {
15
Place,
16
ReactiveScope,
17
} from "../HIR/HIR";
18
-import { eachInstructionOperand, eachPatternOperand } from "../HIR/visitors";
18
+import {
19
+ doesPatternContainSpreadElement,
20
+ eachInstructionOperand,
21
+ eachPatternOperand,
22
+} from "../HIR/visitors";
23
import DisjointSet from "../Utils/DisjointSet";
24
import { assertExhaustive } from "../Utils/utils";
25
@@ -198,7 +202,9 @@ function isMutable({ id }: Instruction, place: Place): boolean {
202
203
function mayAllocate(value: InstructionValue): boolean {
204
switch (value.kind) {
201
- case "Destructure":
205
+ case "Destructure": {
206
+ return doesPatternContainSpreadElement(value.lvalue.pattern);
207
+ }
208
case "StoreLocal":
209
case "LoadGlobal":
210
case "TypeCastExpression":
compiler/forget/src/ReactiveScopes/PruneNonEscapingScopes.ts
+106
-43
@@ -12,6 +12,7 @@ import {
12
Effect,
13
IdentifierId,
14
InstructionId,
15
+ Pattern,
16
Place,
17
ReactiveFunction,
18
ReactiveInstruction,
@@ -22,7 +23,6 @@ import {
23
ReactiveValue,
24
ScopeId,
25
} from "../HIR";
25
-import { eachPatternOperand } from "../HIR/visitors";
26
import { log } from "../Utils/logger";
27
import { assertExhaustive } from "../Utils/utils";
28
import { getPlaceScope } from "./BuildReactiveBlocks";
@@ -319,43 +319,48 @@ function computeMemoizationInputs(
319
lvalue: Place | null
320
): {
321
// can optionally return a custom set of lvalues per instruction
322
- lvalues: Array<Place>;
322
+ lvalues: Array<LValueMemoization>;
323
rvalues: Array<Place>;
324
- level: MemoizationLevel;
324
} {
325
switch (value.kind) {
326
case "ConditionalExpression": {
327
return {
329
- lvalues: lvalue !== null ? [lvalue] : [],
328
+ // Only need to memoize if the rvalues are memoized
329
+ lvalues:
330
+ lvalue !== null
331
+ ? [{ place: lvalue, level: MemoizationLevel.Conditional }]
332
+ : [],
333
rvalues: [
334
// Conditionals do not alias their test value.
335
...computeMemoizationInputs(value.consequent, null).rvalues,
336
...computeMemoizationInputs(value.alternate, null).rvalues,
337
],
335
- // Only need to memoize if the rvalues are memoized
336
- level: MemoizationLevel.Conditional,
338
};
339
}
340
case "LogicalExpression": {
341
return {
341
- lvalues: lvalue !== null ? [lvalue] : [],
342
+ // Only need to memoize if the rvalues are memoized
343
+ lvalues:
344
+ lvalue !== null
345
+ ? [{ place: lvalue, level: MemoizationLevel.Conditional }]
346
+ : [],
347
rvalues: [
348
...computeMemoizationInputs(value.left, null).rvalues,
349
...computeMemoizationInputs(value.right, null).rvalues,
350
],
346
- // Only need to memoize if the rvalues are memoized
347
- level: MemoizationLevel.Conditional,
351
};
352
}
353
case "SequenceExpression": {
354
return {
352
- lvalues: lvalue !== null ? [lvalue] : [],
355
+ // Only need to memoize if the rvalues are memoized
356
+ lvalues:
357
+ lvalue !== null
358
+ ? [{ place: lvalue, level: MemoizationLevel.Conditional }]
359
+ : [],
360
// Only the final value of the sequence is a true rvalue:
361
// values from the sequence's instructions are evaluated
362
// as separate nodes
363
rvalues: computeMemoizationInputs(value.value, null).rvalues,
357
- // Only memoize if the final value was memoized
358
- level: MemoizationLevel.Conditional,
364
};
365
}
366
case "JsxExpression": {
@@ -374,20 +379,24 @@ function computeMemoizationInputs(
379
}
380
}
381
return {
377
- lvalues: lvalue !== null ? [lvalue] : [],
378
- rvalues: operands,
382
// JSX elements themselves are not memoized unless forced to
383
// avoid breaking downstream memoization
381
- level: MemoizationLevel.Unmemoized,
384
+ lvalues:
385
+ lvalue !== null
386
+ ? [{ place: lvalue, level: MemoizationLevel.Unmemoized }]
387
+ : [],
388
+ rvalues: operands,
389
};
390
}
391
case "JsxFragment": {
392
return {
386
- lvalues: lvalue !== null ? [lvalue] : [],
387
- rvalues: value.children,
393
// JSX elements themselves are not memoized unless forced to
394
// avoid breaking downstream memoization
390
- level: MemoizationLevel.Unmemoized,
395
+ lvalues:
396
+ lvalue !== null
397
+ ? [{ place: lvalue, level: MemoizationLevel.Unmemoized }]
398
+ : [],
399
+ rvalues: value.children,
400
};
401
}
402
case "ComputedDelete":
@@ -399,68 +408,84 @@ function computeMemoizationInputs(
408
case "BinaryExpression":
409
case "UnaryExpression": {
410
return {
402
- lvalues: lvalue !== null ? [lvalue] : [],
403
- rvalues: [],
411
// All of these instructions return a primitive value and never need to be memoized
405
- level: MemoizationLevel.Never,
412
+ lvalues:
413
+ lvalue !== null
414
+ ? [{ place: lvalue, level: MemoizationLevel.Never }]
415
+ : [],
416
+ rvalues: [],
417
};
418
}
419
case "TypeCastExpression": {
420
return {
410
- lvalues: lvalue !== null ? [lvalue] : [],
421
// Indirection for the inner value, memoized if the value is
422
+ lvalues:
423
+ lvalue !== null
424
+ ? [{ place: lvalue, level: MemoizationLevel.Conditional }]
425
+ : [],
426
rvalues: [value.value],
413
- level: MemoizationLevel.Conditional,
427
};
428
}
429
case "LoadLocal": {
430
return {
418
- lvalues: lvalue !== null ? [lvalue] : [],
431
// Indirection for the inner value, memoized if the value is
432
+ lvalues:
433
+ lvalue !== null
434
+ ? [{ place: lvalue, level: MemoizationLevel.Conditional }]
435
+ : [],
436
rvalues: [value.place],
421
- level: MemoizationLevel.Conditional,
437
};
438
}
439
case "StoreLocal": {
440
+ const lvalues = [
441
+ { place: value.lvalue.place, level: MemoizationLevel.Conditional },
442
+ ];
443
+ if (lvalue !== null) {
444
+ lvalues.push({ place: lvalue, level: MemoizationLevel.Conditional });
445
+ }
446
return {
426
- lvalues:
427
- lvalue !== null ? [lvalue, value.lvalue.place] : [value.lvalue.place],
447
// Indirection for the inner value, memoized if the value is
448
+ lvalues,
449
rvalues: [value.value],
430
- level: MemoizationLevel.Conditional,
450
};
451
}
452
case "Destructure": {
453
+ // Indirection for the inner value, memoized if the value is
454
const lvalues = [];
455
if (lvalue !== null) {
436
- lvalues.push(lvalue);
456
+ lvalues.push({ place: lvalue, level: MemoizationLevel.Conditional });
457
}
438
- lvalues.push(...eachPatternOperand(value.lvalue.pattern));
458
+ lvalues.push(...computePatternLValues(value.lvalue.pattern));
459
return {
460
lvalues: lvalues,
441
- // Indirection for the inner value, memoized if the value is
461
rvalues: [value.value],
443
- level: MemoizationLevel.Conditional,
462
};
463
}
464
case "ComputedLoad":
465
case "PropertyLoad": {
466
return {
449
- lvalues: lvalue !== null ? [lvalue] : [],
467
// Indirection for the inner value, memoized if the value is
468
+ lvalues:
469
+ lvalue !== null
470
+ ? [{ place: lvalue, level: MemoizationLevel.Conditional }]
471
+ : [],
472
// Only the object is aliased to the result, and the result only needs to be
473
// memoized if the object is
474
rvalues: [value.object],
454
- level: MemoizationLevel.Conditional,
475
};
476
}
477
case "ComputedStore": {
478
// The object being stored to acts as an lvalue (it aliases the value), but
479
// the computed key is not aliased
480
+ const lvalues = [
481
+ { place: value.object, level: MemoizationLevel.Conditional },
482
+ ];
483
+ if (lvalue !== null) {
484
+ lvalues.push({ place: lvalue, level: MemoizationLevel.Conditional });
485
+ }
486
return {
461
- lvalues: lvalue !== null ? [lvalue, value.object] : [value.object],
487
+ lvalues,
488
rvalues: [value.value],
463
- level: MemoizationLevel.Conditional,
489
};
490
}
491
case "FunctionExpression":
@@ -475,16 +500,15 @@ function computeMemoizationInputs(
500
// All of these instructions may produce new values which must be memoized if
501
// reachable from a return value. Any mutable rvalue may alias any other rvalue
502
const operands = [...eachReactiveValueOperand(value)];
478
- const lvalues = operands.filter((operand) =>
479
- isMutableEffect(operand.effect)
480
- );
503
+ const lvalues = operands
504
+ .filter((operand) => isMutableEffect(operand.effect))
505
+ .map((place) => ({ place, level: MemoizationLevel.Memoized }));
506
if (lvalue !== null) {
482
- lvalues.push(lvalue);
507
+ lvalues.push({ place: lvalue, level: MemoizationLevel.Memoized });
508
}
509
return {
510
lvalues,
511
rvalues: operands,
487
- level: MemoizationLevel.Memoized,
512
};
513
}
514
case "UnsupportedNode": {
@@ -496,6 +520,45 @@ function computeMemoizationInputs(
520
}
521
}
522
523
+function computePatternLValues(pattern: Pattern): Array<LValueMemoization> {
524
+ const lvalues: Array<LValueMemoization> = [];
525
+ switch (pattern.kind) {
526
+ case "ArrayPattern": {
527
+ for (const item of pattern.items) {
528
+ if (item.kind === "Identifier") {
529
+ lvalues.push({ place: item, level: MemoizationLevel.Conditional });
530
+ } else {
531
+ lvalues.push({ place: item.place, level: MemoizationLevel.Memoized });
532
+ }
533
+ }
534
+ break;
535
+ }
536
+ case "ObjectPattern": {
537
+ for (const property of pattern.properties) {
538
+ if (property.kind === "ObjectProperty") {
539
+ lvalues.push({
540
+ place: property.place,
541
+ level: MemoizationLevel.Conditional,
542
+ });
543
+ } else {
544
+ lvalues.push({
545
+ place: property.place,
546
+ level: MemoizationLevel.Memoized,
547
+ });
548
+ }
549
+ }
550
+ break;
551
+ }
552
+ default: {
553
+ assertExhaustive(
554
+ pattern,
555
+ `Unexpected pattern kind '${(pattern as any).kind}'`
556
+ );
557
+ }
558
+ }
559
+ return lvalues;
560
+}
561
+
562
/**
563
* Populates the input state with the set of returned identifiers and information about each
564
* identifier's and scope's dependencies.
@@ -521,7 +584,7 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor<State> {
584
}
585
586
// Add the operands as dependencies of all lvalues.
524
- for (const lvalue of aliasing.lvalues) {
587
+ for (const { place: lvalue, level } of aliasing.lvalues) {
588
const lvalueId =
589
state.definitions.get(lvalue.identifier.id) ?? lvalue.identifier.id;
590
let node = state.identifiers.get(lvalueId);
@@ -535,7 +598,7 @@ class CollectDependenciesVisitor extends ReactiveFunctionVisitor<State> {
598
};
599
state.identifiers.set(lvalueId, node);
600
}
538
- node.level = joinAliases(node.level, aliasing.level);
601
+ node.level = joinAliases(node.level, level);
602
// This looks like NxM iterations but in practice all instructions with multiple
603
// lvalues have only a single rvalue
604
for (const operand of aliasing.rvalues) {
compiler/forget/src/__tests__/fixtures/hir/destructuring.expect.md
+64
-28
@@ -28,39 +28,75 @@ function foo(a, b, c) {
28
29
```javascript
30
function foo(a, b, c) {
31
- const $ = React.unstable_useMemoCache(8);
32
- const [d, t40, ...h] = a;
31
+ const $ = React.unstable_useMemoCache(18);
32
+ const c_0 = $[0] !== a;
33
+ let t0;
34
+ let d;
35
+ let h;
36
+ if (c_0) {
37
+ [d, t0, ...h] = a;
38
+ $[0] = a;
39
+ $[1] = t0;
40
+ $[2] = d;
41
+ $[3] = h;
42
+ } else {
43
+ t0 = $[1];
44
+ d = $[2];
45
+ h = $[3];
46
+ }
47
34
- const [t43] = t40;
35
- const { e: t45, ...g } = t43;
36
- const { f } = t45;
48
+ const [t1] = t0;
49
+ const c_4 = $[4] !== t1;
50
+ let t2;
51
+ let g;
52
+ if (c_4) {
53
+ ({ e: t2, ...g } = t1);
54
+ $[4] = t1;
55
+ $[5] = t2;
56
+ $[6] = g;
57
+ } else {
58
+ t2 = $[5];
59
+ g = $[6];
60
+ }
61
+ const { f } = t2;
62
63
const { l: t51, p } = b;
39
- const { m: t54 } = t51;
40
- const [t56, ...o] = t54;
41
- const [n] = t56;
42
- const c_0 = $[0] !== d;
43
- const c_1 = $[1] !== f;
44
- const c_2 = $[2] !== g;
45
- const c_3 = $[3] !== h;
46
- const c_4 = $[4] !== n;
47
- const c_5 = $[5] !== o;
48
- const c_6 = $[6] !== p;
49
- let t0;
50
- if (c_0 || c_1 || c_2 || c_3 || c_4 || c_5 || c_6) {
51
- t0 = [d, f, g, h, n, o, p];
52
- $[0] = d;
53
- $[1] = f;
54
- $[2] = g;
55
- $[3] = h;
56
- $[4] = n;
57
- $[5] = o;
58
- $[6] = p;
59
- $[7] = t0;
64
+ const { m: t3 } = t51;
65
+ const c_7 = $[7] !== t3;
66
+ let t4;
67
+ let o;
68
+ if (c_7) {
69
+ [t4, ...o] = t3;
70
+ $[7] = t3;
71
+ $[8] = t4;
72
+ $[9] = o;
73
+ } else {
74
+ t4 = $[8];
75
+ o = $[9];
76
+ }
77
+ const [n] = t4;
78
+ const c_10 = $[10] !== d;
79
+ const c_11 = $[11] !== f;
80
+ const c_12 = $[12] !== g;
81
+ const c_13 = $[13] !== h;
82
+ const c_14 = $[14] !== n;
83
+ const c_15 = $[15] !== o;
84
+ const c_16 = $[16] !== p;
85
+ let t5;
86
+ if (c_10 || c_11 || c_12 || c_13 || c_14 || c_15 || c_16) {
87
+ t5 = [d, f, g, h, n, o, p];
88
+ $[10] = d;
89
+ $[11] = f;
90
+ $[12] = g;
91
+ $[13] = h;
92
+ $[14] = n;
93
+ $[15] = o;
94
+ $[16] = p;
95
+ $[17] = t5;
96
} else {
61
- t0 = $[7];
97
+ t5 = $[17];
98
}
63
- return t0;
99
+ return t5;
100
}
101
102
```
compiler/forget/src/__tests__/fixtures/hir/escape-analysis-destructured-rest-element.expect.md
+19
-3
@@ -16,9 +16,25 @@ function Component(props) {
16
17
```javascript
18
function Component(props) {
19
- const { a, ...b } = props.a;
20
-
21
- const [c, ...d] = props.c;
19
+ const $ = React.unstable_useMemoCache(4);
20
+ const c_0 = $[0] !== props.a;
21
+ let b;
22
+ if (c_0) {
23
+ ({ a, ...b } = props.a);
24
+ $[0] = props.a;
25
+ $[1] = b;
26
+ } else {
27
+ b = $[1];
28
+ }
29
+ const c_2 = $[2] !== props.c;
30
+ let d;
31
+ if (c_2) {
32
+ [c, ...d] = props.c;
33
+ $[2] = props.c;
34
+ $[3] = d;
35
+ } else {
36
+ d = $[3];
37
+ }
38
return <div b={b} d={d}></div>;
39
}
40
compiler/forget/src/__tests__/fixtures/hir/unused-object-element-with-rest.expect.md
+10
-1
@@ -14,7 +14,16 @@ function Foo(props) {
14
15
```javascript
16
function Foo(props) {
17
- const { unused, ...rest } = props.a;
17
+ const $ = React.unstable_useMemoCache(2);
18
+ const c_0 = $[0] !== props.a;
19
+ let rest;
20
+ if (c_0) {
21
+ ({ unused, ...rest } = props.a);
22
+ $[0] = props.a;
23
+ $[1] = rest;
24
+ } else {
25
+ rest = $[1];
26
+ }
27
return rest;
28
}
29