@samitouri / QOS-React-2 / commits / 8be56418d3

InferReactivePlaces accounts for mutable aliasing

Fixes T175227223. When inferring reactivity, mutation of a value with a reactive input marks the mutable value as reactive. However, we also need to account for aliases: ```javascript const x = []; const y = x; y.push(props.value); ``` Previously we would have only considered `y` reactive here, but `x` also becomes reactive. The implementation extracts out a helper from InferReactiveScopeVariables that builds a `DisjointSet<Identifier>` of disjoint sets of mutably aliased values. InferReactivePlaces then treats all instances of each mutable alias group as equivalent for reactivity purposes.

Joe Savona committed Jan 19, 2024 at 16:03 UTC 8be56418d31ba6ff0113c3ccb91ce3f3d11ee4fd
8 files changed +369 -112
compiler/packages/babel-plugin-react-forget/src/Inference/InferReactivePlaces.ts
+29 -5
@@ -24,7 +24,11 @@ import {
24 eachTerminalOperand,
25 } from "../HIR/visitors";
26 import { hasBackEdge } from "../Optimization/DeadCodeElimination";
27 -import { isMutable } from "../ReactiveScopes/InferReactiveScopeVariables";
27 +import {
28 + findDisjointMutableValues,
29 + isMutable,
30 +} from "../ReactiveScopes/InferReactiveScopeVariables";
31 +import DisjointSet from "../Utils/DisjointSet";
32 import { assertExhaustive } from "../Utils/utils";
33
34 /*
@@ -87,7 +91,7 @@ import { assertExhaustive } from "../Utils/utils";
91 * there are no changes after a given pass over the CFG.
92 */
93 export function inferReactivePlaces(fn: HIRFunction): void {
90 - const reactiveIdentifiers = new ReactivityMap();
94 + const reactiveIdentifiers = new ReactivityMap(findDisjointMutableValues(fn));
95 for (const param of fn.params) {
96 const place = param.kind === "Identifier" ? param : param.place;
97 reactiveIdentifiers.markReactive(place);
@@ -328,15 +332,33 @@ class ReactivityMap {
332 hasChanges: boolean = false;
333 reactive: Set<IdentifierId> = new Set();
334
335 + /**
336 + * Sets of mutably aliased identifiers — these are the same foundation for determining
337 + * reactive scopes a few passes later. The actual InferReactiveScopeVariables pass runs
338 + * after LeaveSSA, which artificially merges mutable ranges in cases such as declarations
339 + * that are later reassigned. Here we use only the underlying sets of mutably aliased values.
340 + *
341 + * Any identifier that has a mapping in this disjoint set will be treated as a stand in for
342 + * its canonical identifier in all cases, so that any reactivity flowing into one identifier of
343 + * an alias group will effectively make the whole alias group (all its identifiers) reactive.
344 + */
345 + aliasedIdentifiers: DisjointSet<Identifier>;
346 +
347 + constructor(aliasedIdentifiers: DisjointSet<Identifier>) {
348 + this.aliasedIdentifiers = aliasedIdentifiers;
349 + }
350 +
351 isReactive(place: Place): boolean {
332 - const reactive = this.reactive.has(place.identifier.id);
352 + const reactive = this.isReactiveIdentifier(place.identifier);
353 if (reactive) {
354 place.reactive = true;
355 }
356 return reactive;
357 }
358
339 - isReactiveIdentifier(identifier: Identifier): boolean {
359 + isReactiveIdentifier(inputIdentifier: Identifier): boolean {
360 + const identifier =
361 + this.aliasedIdentifiers.find(inputIdentifier) ?? inputIdentifier;
362 return this.reactive.has(identifier.id);
363 }
364
@@ -345,7 +367,9 @@ class ReactivityMap {
367 this.markReactiveIdentifier(place.identifier);
368 }
369
348 - markReactiveIdentifier(identifier: Identifier): void {
370 + markReactiveIdentifier(inputIdentifier: Identifier): void {
371 + const identifier =
372 + this.aliasedIdentifiers.find(inputIdentifier) ?? inputIdentifier;
373 if (!this.reactive.has(identifier.id)) {
374 this.hasChanges = true;
375 this.reactive.add(identifier.id);
compiler/packages/babel-plugin-react-forget/src/ReactiveScopes/InferReactiveScopeVariables.ts
+100 -94
@@ -84,100 +84,7 @@ export function inferReactiveScopeVariables(fn: HIRFunction): void {
84 * Represents the set of reactive scopes as disjoint sets of identifiers
85 * that mutate together.
86 */
87 - const scopeIdentifiers = new DisjointSet<Identifier>();
88 - for (const [_, block] of fn.body.blocks) {
89 - /*
90 - * If a phi is mutated after creation, then we need to alias all of its operands such that they
91 - * are assigned to the same scope.
92 - */
93 - for (const phi of block.phis) {
94 - if (
95 - // The phi was reset because it was not mutated after creation
96 - phi.id.mutableRange.start + 1 !== phi.id.mutableRange.end &&
97 - phi.id.mutableRange.end >
98 - (block.instructions.at(0)?.id ?? block.terminal.id)
99 - ) {
100 - for (const [, phiId] of phi.operands) {
101 - scopeIdentifiers.union([phi.id, phiId]);
102 - }
103 - }
104 - }
105 - block.phis.clear();
106 -
107 - for (const instr of block.instructions) {
108 - const operands: Array<Identifier> = [];
109 - const range = instr.lvalue.identifier.mutableRange;
110 - if (range.end > range.start + 1 || mayAllocate(fn.env, instr)) {
111 - operands.push(instr.lvalue!.identifier);
112 - }
113 - if (
114 - instr.value.kind === "StoreLocal" ||
115 - instr.value.kind === "StoreContext"
116 - ) {
117 - if (
118 - instr.value.lvalue.place.identifier.mutableRange.end >
119 - instr.value.lvalue.place.identifier.mutableRange.start + 1
120 - ) {
121 - operands.push(instr.value.lvalue.place.identifier);
122 - }
123 - if (
124 - isMutable(instr, instr.value.value) &&
125 - instr.value.value.identifier.mutableRange.start > 0
126 - ) {
127 - operands.push(instr.value.value.identifier);
128 - }
129 - } else if (instr.value.kind === "Destructure") {
130 - for (const place of eachPatternOperand(instr.value.lvalue.pattern)) {
131 - if (
132 - place.identifier.mutableRange.end >
133 - place.identifier.mutableRange.start + 1
134 - ) {
135 - operands.push(place.identifier);
136 - }
137 - }
138 - if (
139 - isMutable(instr, instr.value.value) &&
140 - instr.value.value.identifier.mutableRange.start > 0
141 - ) {
142 - operands.push(instr.value.value.identifier);
143 - }
144 - } else if (instr.value.kind === "MethodCall") {
145 - for (const operand of eachInstructionOperand(instr)) {
146 - if (
147 - isMutable(instr, operand) &&
148 - /*
149 - * exclude global variables from being added to scopes, we can't recreate them!
150 - * TODO: improve handling of module-scoped variables and globals
151 - */
152 - operand.identifier.mutableRange.start > 0
153 - ) {
154 - operands.push(operand.identifier);
155 - }
156 - }
157 - /*
158 - * Ensure that the ComputedLoad to resolve the method is in the same scope as the
159 - * call itself
160 - */
161 - operands.push(instr.value.property.identifier);
162 - } else {
163 - for (const operand of eachInstructionOperand(instr)) {
164 - if (
165 - isMutable(instr, operand) &&
166 - /*
167 - * exclude global variables from being added to scopes, we can't recreate them!
168 - * TODO: improve handling of module-scoped variables and globals
169 - */
170 - operand.identifier.mutableRange.start > 0
171 - ) {
172 - operands.push(operand.identifier);
173 - }
174 - }
175 - }
176 - if (operands.length !== 0) {
177 - scopeIdentifiers.union(operands);
178 - }
179 - }
180 - }
87 + const scopeIdentifiers = findDisjointMutableValues(fn);
88
89 // Maps each scope (by its identifying member) to a ScopeId value
90 const scopes: Map<Identifier, ReactiveScope> = new Map();
@@ -278,3 +185,102 @@ function mayAllocate(env: Environment, instruction: Instruction): boolean {
185 }
186 }
187 }
188 +
189 +export function findDisjointMutableValues(
190 + fn: HIRFunction
191 +): DisjointSet<Identifier> {
192 + const scopeIdentifiers = new DisjointSet<Identifier>();
193 + for (const [_, block] of fn.body.blocks) {
194 + /*
195 + * If a phi is mutated after creation, then we need to alias all of its operands such that they
196 + * are assigned to the same scope.
197 + */
198 + for (const phi of block.phis) {
199 + if (
200 + // The phi was reset because it was not mutated after creation
201 + phi.id.mutableRange.start + 1 !== phi.id.mutableRange.end &&
202 + phi.id.mutableRange.end >
203 + (block.instructions.at(0)?.id ?? block.terminal.id)
204 + ) {
205 + for (const [, phiId] of phi.operands) {
206 + scopeIdentifiers.union([phi.id, phiId]);
207 + }
208 + }
209 + }
210 +
211 + for (const instr of block.instructions) {
212 + const operands: Array<Identifier> = [];
213 + const range = instr.lvalue.identifier.mutableRange;
214 + if (range.end > range.start + 1 || mayAllocate(fn.env, instr)) {
215 + operands.push(instr.lvalue!.identifier);
216 + }
217 + if (
218 + instr.value.kind === "StoreLocal" ||
219 + instr.value.kind === "StoreContext"
220 + ) {
221 + if (
222 + instr.value.lvalue.place.identifier.mutableRange.end >
223 + instr.value.lvalue.place.identifier.mutableRange.start + 1
224 + ) {
225 + operands.push(instr.value.lvalue.place.identifier);
226 + }
227 + if (
228 + isMutable(instr, instr.value.value) &&
229 + instr.value.value.identifier.mutableRange.start > 0
230 + ) {
231 + operands.push(instr.value.value.identifier);
232 + }
233 + } else if (instr.value.kind === "Destructure") {
234 + for (const place of eachPatternOperand(instr.value.lvalue.pattern)) {
235 + if (
236 + place.identifier.mutableRange.end >
237 + place.identifier.mutableRange.start + 1
238 + ) {
239 + operands.push(place.identifier);
240 + }
241 + }
242 + if (
243 + isMutable(instr, instr.value.value) &&
244 + instr.value.value.identifier.mutableRange.start > 0
245 + ) {
246 + operands.push(instr.value.value.identifier);
247 + }
248 + } else if (instr.value.kind === "MethodCall") {
249 + for (const operand of eachInstructionOperand(instr)) {
250 + if (
251 + isMutable(instr, operand) &&
252 + /*
253 + * exclude global variables from being added to scopes, we can't recreate them!
254 + * TODO: improve handling of module-scoped variables and globals
255 + */
256 + operand.identifier.mutableRange.start > 0
257 + ) {
258 + operands.push(operand.identifier);
259 + }
260 + }
261 + /*
262 + * Ensure that the ComputedLoad to resolve the method is in the same scope as the
263 + * call itself
264 + */
265 + operands.push(instr.value.property.identifier);
266 + } else {
267 + for (const operand of eachInstructionOperand(instr)) {
268 + if (
269 + isMutable(instr, operand) &&
270 + /*
271 + * exclude global variables from being added to scopes, we can't recreate them!
272 + * TODO: improve handling of module-scoped variables and globals
273 + */
274 + operand.identifier.mutableRange.start > 0
275 + ) {
276 + operands.push(operand.identifier);
277 + }
278 + }
279 + }
280 + if (operands.length !== 0) {
281 + scopeIdentifiers.union(operands);
282 + }
283 + }
284 + }
285 + return scopeIdentifiers;
286 +}
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.expect.md new
+84
@@ -0,0 +1,84 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + const x = [];
7 + const y = x;
8 + y.push(props.input);
9 +
10 + return [x[0]];
11 +}
12 +
13 +export const FIXTURE_ENTRYPOINT = {
14 + fn: Component,
15 + params: [],
16 + sequentialRenders: [
17 + { input: 42 },
18 + { input: 42 },
19 + { input: "sathya" },
20 + { input: "sathya" },
21 + { input: 42 },
22 + { input: "sathya" },
23 + { input: 42 },
24 + { input: "sathya" },
25 + ],
26 +};
27 +
28 +```
29 +
30 +## Code
31 +
32 +```javascript
33 +import { unstable_useMemoCache as useMemoCache } from "react";
34 +function Component(props) {
35 + const $ = useMemoCache(4);
36 + let x;
37 + if ($[0] !== props.input) {
38 + x = [];
39 + const y = x;
40 + y.push(props.input);
41 + $[0] = props.input;
42 + $[1] = x;
43 + } else {
44 + x = $[1];
45 + }
46 +
47 + const t0 = x[0];
48 + let t1;
49 + if ($[2] !== t0) {
50 + t1 = [t0];
51 + $[2] = t0;
52 + $[3] = t1;
53 + } else {
54 + t1 = $[3];
55 + }
56 + return t1;
57 +}
58 +
59 +export const FIXTURE_ENTRYPOINT = {
60 + fn: Component,
61 + params: [],
62 + sequentialRenders: [
63 + { input: 42 },
64 + { input: 42 },
65 + { input: "sathya" },
66 + { input: "sathya" },
67 + { input: 42 },
68 + { input: "sathya" },
69 + { input: 42 },
70 + { input: "sathya" },
71 + ],
72 +};
73 +
74 +```
75 +
76 +### Eval output
77 +(kind: ok) [42]
78 +[42]
79 +["sathya"]
80 +["sathya"]
81 +[42]
82 +["sathya"]
83 +[42]
84 +["sathya"]
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-array.js new
+22
@@ -0,0 +1,22 @@
1 +function Component(props) {
2 + const x = [];
3 + const y = x;
4 + y.push(props.input);
5 +
6 + return [x[0]];
7 +}
8 +
9 +export const FIXTURE_ENTRYPOINT = {
10 + fn: Component,
11 + params: [],
12 + sequentialRenders: [
13 + { input: 42 },
14 + { input: 42 },
15 + { input: "sathya" },
16 + { input: "sathya" },
17 + { input: 42 },
18 + { input: "sathya" },
19 + { input: 42 },
20 + { input: "sathya" },
21 + ],
22 +};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.expect.md new
+91
@@ -0,0 +1,91 @@
1 +
2 +## Input
3 +
4 +```javascript
5 +function Component(props) {
6 + const x = [];
7 + const f = (arg) => {
8 + const y = x;
9 + y.push(arg);
10 + };
11 + f(props.input);
12 +
13 + return [x[0]];
14 +}
15 +
16 +export const FIXTURE_ENTRYPOINT = {
17 + fn: Component,
18 + params: [],
19 + sequentialRenders: [
20 + { input: 42 },
21 + { input: 42 },
22 + { input: "sathya" },
23 + { input: "sathya" },
24 + { input: 42 },
25 + { input: "sathya" },
26 + { input: 42 },
27 + { input: "sathya" },
28 + ],
29 +};
30 +
31 +```
32 +
33 +## Code
34 +
35 +```javascript
36 +import { unstable_useMemoCache as useMemoCache } from "react";
37 +function Component(props) {
38 + const $ = useMemoCache(4);
39 + let x;
40 + if ($[0] !== props.input) {
41 + x = [];
42 + const f = (arg) => {
43 + const y = x;
44 + y.push(arg);
45 + };
46 +
47 + f(props.input);
48 + $[0] = props.input;
49 + $[1] = x;
50 + } else {
51 + x = $[1];
52 + }
53 +
54 + const t0 = x[0];
55 + let t1;
56 + if ($[2] !== t0) {
57 + t1 = [t0];
58 + $[2] = t0;
59 + $[3] = t1;
60 + } else {
61 + t1 = $[3];
62 + }
63 + return t1;
64 +}
65 +
66 +export const FIXTURE_ENTRYPOINT = {
67 + fn: Component,
68 + params: [],
69 + sequentialRenders: [
70 + { input: 42 },
71 + { input: 42 },
72 + { input: "sathya" },
73 + { input: "sathya" },
74 + { input: 42 },
75 + { input: "sathya" },
76 + { input: 42 },
77 + { input: "sathya" },
78 + ],
79 +};
80 +
81 +```
82 +
83 +### Eval output
84 +(kind: ok) [42]
85 +[42]
86 +["sathya"]
87 +["sathya"]
88 +[42]
89 +["sathya"]
90 +[42]
91 +["sathya"]
\ No newline at end of file
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/reactivity-via-aliased-mutation-lambda.js new
+25
@@ -0,0 +1,25 @@
1 +function Component(props) {
2 + const x = [];
3 + const f = (arg) => {
4 + const y = x;
5 + y.push(arg);
6 + };
7 + f(props.input);
8 +
9 + return [x[0]];
10 +}
11 +
12 +export const FIXTURE_ENTRYPOINT = {
13 + fn: Component,
14 + params: [],
15 + sequentialRenders: [
16 + { input: 42 },
17 + { input: 42 },
18 + { input: "sathya" },
19 + { input: "sathya" },
20 + { input: 42 },
21 + { input: "sathya" },
22 + { input: 42 },
23 + { input: "sathya" },
24 + ],
25 +};
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-dont-memoize-array-with-capturing-map-after-hook.expect.md
+9 -7
@@ -42,7 +42,7 @@ import {
42 import { mutate } from "shared-runtime";
43
44 function Component(props) {
45 - const $ = useMemoCache(5);
45 + const $ = useMemoCache(6);
46 const x = [{ ...props.value }];
47 let t0;
48 let t1;
@@ -66,21 +66,23 @@ function Component(props) {
66 y = item;
67 return <span key={item.id}>{item.text}</span>;
68 });
69 - let t3;
70 - if ($[2] !== onClick || $[3] !== t2) {
71 - t3 = (
69 + const t3 = mutate(y);
70 + let t4;
71 + if ($[2] !== onClick || $[3] !== t2 || $[4] !== t3) {
72 + t4 = (
73 <div onClick={onClick}>
74 {t2}
74 - {mutate(y)}
75 + {t3}
76 </div>
77 );
78 $[2] = onClick;
79 $[3] = t2;
80 $[4] = t3;
81 + $[5] = t4;
82 } else {
81 - t3 = $[4];
83 + t4 = $[5];
84 }
83 - return t3;
85 + return t4;
86 }
87
88 export const FIXTURE_ENTRYPOINT = {
compiler/packages/babel-plugin-react-forget/src/__tests__/fixtures/compiler/repro-scope-missing-mutable-range.expect.md
+9 -6
@@ -20,7 +20,7 @@ function HomeDiscoStoreItemTileRating(props) {
20 ```javascript
21 import { unstable_useMemoCache as useMemoCache } from "react";
22 function HomeDiscoStoreItemTileRating(props) {
23 - const $ = useMemoCache(3);
23 + const $ = useMemoCache(4);
24 const item = useFragment();
25 let count;
26 if ($[0] !== item) {
@@ -34,14 +34,17 @@ function HomeDiscoStoreItemTileRating(props) {
34 } else {
35 count = $[1];
36 }
37 - let t0;
38 - if ($[2] === Symbol.for("react.memo_cache_sentinel")) {
39 - t0 = <Text>{count}</Text>;
37 +
38 + const t0 = count;
39 + let t1;
40 + if ($[2] !== t0) {
41 + t1 = <Text>{t0}</Text>;
42 $[2] = t0;
43 + $[3] = t1;
44 } else {
42 - t0 = $[2];
45 + t1 = $[3];
46 }
44 - return t0;
47 + return t1;
48 }
49
50 ```