Conditional assignment of unmemoized value doesnt invalidate later scopes
The previous implementation used IdentifierId, but since this pass operates after LeaveSSA the identifier ids are no longer distinct for different SSA instances. Instead we use the Identifier instance, which preserves SSA information (even ever LeaveSSA) and allows distinguishing between variables whose value always changes vs variables that may be reassigned such that they don't always invalidate. In the future when we use HIR everywhere, this pass should use the HIR CFG to understand that phi nodes whose operands all will always invalidate can also be treated as always invalidating. ## Test Plan Synced to www, 91 files have output changes (https://fburl.com/everpaste/3e3hjpjs). I spot checked these and confirmed that they are all from cases where there was already missing memoization of earlier values, where we now can prune later reactive scopes that depend on the un-memoized values.
Joe Savona committed
Mar 6, 2024 at 11:07 UTC
d1d6310f25cd74ce9f79ab22ec947fec67eecec5
3 files changed
+108
-19
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/PruneAlwaysInvalidatingScopes.ts
+19
-19
@@ -11,7 +11,7 @@ import {
11
visitReactiveFunction,
12
} from ".";
13
import {
14
- IdentifierId,
14
+ Identifier,
15
ReactiveFunction,
16
ReactiveInstruction,
17
ReactiveScopeBlock,
@@ -33,8 +33,8 @@ export function pruneAlwaysInvalidatingScopes(fn: ReactiveFunction): void {
33
}
34
35
class Transform extends ReactiveFunctionTransform<boolean> {
36
- alwaysInvalidatingValues: Set<IdentifierId> = new Set();
37
- unmemoizedValues: Set<IdentifierId> = new Set();
36
+ alwaysInvalidatingValues: Set<Identifier> = new Set();
37
+ unmemoizedValues: Set<Identifier> = new Set();
38
39
override transformInstruction(
40
instruction: ReactiveInstruction,
@@ -50,34 +50,34 @@ class Transform extends ReactiveFunctionTransform<boolean> {
50
case "JsxFragment":
51
case "NewExpression": {
52
if (lvalue !== null) {
53
- this.alwaysInvalidatingValues.add(lvalue.identifier.id);
53
+ this.alwaysInvalidatingValues.add(lvalue.identifier);
54
if (!withinScope) {
55
- this.unmemoizedValues.add(lvalue.identifier.id);
55
+ this.unmemoizedValues.add(lvalue.identifier);
56
}
57
}
58
break;
59
}
60
case "StoreLocal": {
61
- if (this.alwaysInvalidatingValues.has(value.value.identifier.id)) {
62
- this.alwaysInvalidatingValues.add(value.lvalue.place.identifier.id);
61
+ if (this.alwaysInvalidatingValues.has(value.value.identifier)) {
62
+ this.alwaysInvalidatingValues.add(value.lvalue.place.identifier);
63
}
64
- if (this.unmemoizedValues.has(value.value.identifier.id)) {
65
- this.unmemoizedValues.add(value.lvalue.place.identifier.id);
64
+ if (this.unmemoizedValues.has(value.value.identifier)) {
65
+ this.unmemoizedValues.add(value.lvalue.place.identifier);
66
}
67
break;
68
}
69
case "LoadLocal": {
70
if (
71
lvalue !== null &&
72
- this.alwaysInvalidatingValues.has(value.place.identifier.id)
72
+ this.alwaysInvalidatingValues.has(value.place.identifier)
73
) {
74
- this.alwaysInvalidatingValues.add(lvalue.identifier.id);
74
+ this.alwaysInvalidatingValues.add(lvalue.identifier);
75
}
76
if (
77
lvalue !== null &&
78
- this.unmemoizedValues.has(value.place.identifier.id)
78
+ this.unmemoizedValues.has(value.place.identifier)
79
) {
80
- this.unmemoizedValues.add(lvalue.identifier.id);
80
+ this.unmemoizedValues.add(lvalue.identifier);
81
}
82
break;
83
}
@@ -92,19 +92,19 @@ class Transform extends ReactiveFunctionTransform<boolean> {
92
this.visitScope(scopeBlock, true);
93
94
for (const dep of scopeBlock.scope.dependencies) {
95
- if (this.unmemoizedValues.has(dep.identifier.id)) {
95
+ if (this.unmemoizedValues.has(dep.identifier)) {
96
/*
97
* This scope depends on an always-invalidating value so the scope will always invalidate:
98
* prune it to avoid wasted comparisons
99
*/
100
- for (const [id, _decl] of scopeBlock.scope.declarations) {
101
- if (this.alwaysInvalidatingValues.has(id)) {
102
- this.unmemoizedValues.add(id);
100
+ for (const [_, decl] of scopeBlock.scope.declarations) {
101
+ if (this.alwaysInvalidatingValues.has(decl.identifier)) {
102
+ this.unmemoizedValues.add(decl.identifier);
103
}
104
}
105
for (const identifier of scopeBlock.scope.reassignments) {
106
- if (this.alwaysInvalidatingValues.has(identifier.id)) {
107
- this.unmemoizedValues.add(identifier.id);
106
+ if (this.alwaysInvalidatingValues.has(identifier)) {
107
+ this.unmemoizedValues.add(identifier);
108
}
109
}
110
return { kind: "replace-many", value: scopeBlock.instructions };
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.expect.md
new
+70
@@ -0,0 +1,70 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+import { useHook, identity } from "shared-runtime";
6
+
7
+function Component(props) {
8
+ let x = 42;
9
+ if (props.cond) {
10
+ x = [];
11
+ }
12
+ useHook(); // intersperse a hook call to prevent memoization of x
13
+ identity(x);
14
+
15
+ const y = [x];
16
+
17
+ return [y];
18
+}
19
+
20
+export const FIXTURE_ENTRYPOINT = {
21
+ fn: Component,
22
+ params: [{ value: "sathya" }],
23
+};
24
+
25
+```
26
+
27
+## Code
28
+
29
+```javascript
30
+import { unstable_useMemoCache as useMemoCache } from "react";
31
+import { useHook, identity } from "shared-runtime";
32
+
33
+function Component(props) {
34
+ const $ = useMemoCache(4);
35
+ let x = 42;
36
+ if (props.cond) {
37
+ x = [];
38
+ }
39
+
40
+ useHook();
41
+ identity(x);
42
+ let t0;
43
+ if ($[0] !== x) {
44
+ t0 = [x];
45
+ $[0] = x;
46
+ $[1] = t0;
47
+ } else {
48
+ t0 = $[1];
49
+ }
50
+ const y = t0;
51
+ let t1;
52
+ if ($[2] !== y) {
53
+ t1 = [y];
54
+ $[2] = y;
55
+ $[3] = t1;
56
+ } else {
57
+ t1 = $[3];
58
+ }
59
+ return t1;
60
+}
61
+
62
+export const FIXTURE_ENTRYPOINT = {
63
+ fn: Component,
64
+ params: [{ value: "sathya" }],
65
+};
66
+
67
+```
68
+
69
+### Eval output
70
+(kind: ok) [[42]]
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/prune-scopes-whose-deps-may-invalidate-array.js
new
+19
@@ -0,0 +1,19 @@
1
+import { useHook, identity } from "shared-runtime";
2
+
3
+function Component(props) {
4
+ let x = 42;
5
+ if (props.cond) {
6
+ x = [];
7
+ }
8
+ useHook(); // intersperse a hook call to prevent memoization of x
9
+ identity(x);
10
+
11
+ const y = [x];
12
+
13
+ return [y];
14
+}
15
+
16
+export const FIXTURE_ENTRYPOINT = {
17
+ fn: Component,
18
+ params: [{ value: "sathya" }],
19
+};