[rhir][optim] Preserve conditional deps when propagating reactive scopes
--- **This PR slightly changes the semantics of ReactiveScopeDependencies**. Previously, reading a ReactiveScopeDependency is guaranteed to preserve the `nullthrows` semantics of its own declarations (not that of its inner scopes). This does not affect the overall correctness properties, since we already hoist reading of conditional dependencies (and thus may throw earlier than the original source). E.g. we already do not preserve *where* the nullthrows occurs. ```javascript function Component(props) { // throws here, before print(x) const c_0 = props.a.b !== $[0]; let x; if (c_0) { x = {}; print(x); if (...) mutate1(x, props.a.b); mutate2(x, props.a.b); // ... ``` ### Summary This is an optimization, not a correctness property. When propagating reactive dependencies of an inner scope up to its parent, we want to *retain information about conditional dependencies* -- not the derived unconditional dependencies. This helps us produce more granular dependencies in the parent scope. Current implementation: ```javascript const innerScopeDeps = innerScope.depTree.deriveMinimalUnconditionalDeps(); for (const dep of innerScopeDeps) { currentScope.depTree.addDep(dep); } ``` New implementation: ```javascript // union of a tree takes union of each node currentScope.depTree = currentScope.depTree.union(innerScope.depTree); ``` ### Example In the below example: - `scope @1` has a conditional dependency of `props.a.b`, but that reduces to the unconditional dependency `props` - `scope @0` itself has a unconditional dependency of `props.a.b` - Currently, Forget joins the derived / reduced dependencies of inner scopes, which adds `props` as unconditional dependency of `scope @0` - With this change, Forget joins the property trees and retains info about conditional deps, which adds `props.a.b` as a conditional dep of `scope @0`. ```javascript // scope @0 (deps=[???] decls=[x, y]) let y = {}; // scope @1 (deps=[props] decls=[x]) let x = {}; if (foo) mutate1(x, props.a.b); mutate2(y, props.a.b); ``` ### Followup We currently keep track of properties unconditionally accessed per ReactiveBlock. Eventually we want to keep track of properties unconditionally accessed across blocks (as according to control flow). Consider the following code, in which sibling scopes 0 and 1 are sequentially executed. In this case, we can safely add props.a.b as a dependency of scope 1. ```javascript // scope@0 (deps=[props.a.b], decls=[x]) let x = { a: foo(props.a.b) }; // scope@1 (deps=[???], decls=[y]) let y = {}; if (...) { mutate(y, props.a.b); } ```
Mofei Zhang committed
Mar 6, 2023 at 16:33 UTC
8be45e7d4ae7b583439fea69a0d1531430e7fac6
4 files changed
+247
-29
compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts
+99
-8
@@ -32,18 +32,23 @@ export type ReactiveScopeDependencyInfo = ReactiveScopeDependency & {
32
export class ReactiveScopeDependencyTree {
33
#roots: Map<Identifier, DependencyNode> = new Map();
34
35
- add(dep: ReactiveScopeDependencyInfo) {
36
- let root = this.#roots.get(dep.identifier);
37
- const path = dep.path ?? [];
38
- if (root == null) {
39
- // roots can always be accessed unconditionally in JS
40
- root = {
35
+ #getOrCreateRoot(identifier: Identifier): DependencyNode {
36
+ // roots can always be accessed unconditionally in JS
37
+ let rootNode = this.#roots.get(identifier);
38
+
39
+ if (rootNode === undefined) {
40
+ rootNode = {
41
properties: new Map(),
42
accessType: PropertyAccessType.UnconditionalAccess,
43
};
44
- this.#roots.set(dep.identifier, root);
44
+ this.#roots.set(identifier, rootNode);
45
}
46
- let currNode: DependencyNode = root;
46
+ return rootNode;
47
+ }
48
+
49
+ add(dep: ReactiveScopeDependencyInfo) {
50
+ const path = dep.path ?? [];
51
+ let currNode = this.#getOrCreateRoot(dep.identifier);
52
53
const accessType = dep.cond
54
? PropertyAccessType.ConditionalAccess
@@ -93,6 +98,25 @@ export class ReactiveScopeDependencyTree {
98
return results;
99
}
100
101
+ addDepsFromInnerScope(
102
+ depsFromInnerScope: ReactiveScopeDependencyTree,
103
+ innerScopeInConditionalWithinParent: boolean,
104
+ checkValidDepIdFn: (id: Identifier) => boolean
105
+ ) {
106
+ for (const [id, otherRoot] of depsFromInnerScope.#roots) {
107
+ if (!checkValidDepIdFn(id)) {
108
+ continue;
109
+ }
110
+ let currRoot = this.#getOrCreateRoot(id);
111
+ addSubtree(currRoot, otherRoot, innerScopeInConditionalWithinParent);
112
+ if (!isUnconditional(currRoot.accessType)) {
113
+ currRoot.accessType = isDependency(currRoot.accessType)
114
+ ? PropertyAccessType.UnconditionalDependency
115
+ : PropertyAccessType.UnconditionalAccess;
116
+ }
117
+ }
118
+ }
119
+
120
promoteDepsFromExhaustiveConditionals(
121
trees: Array<ReactiveScopeDependencyTree>
122
) {
@@ -289,6 +313,73 @@ function deriveMinimalDependenciesInSubtree(
313
}
314
}
315
316
+/**
317
+ * Demote all unconditional accesses + dependencies in subtree to the
318
+ * conditional equivalent, mutating subtree in place.
319
+ * @param subtree unconditional node representing a subtree of dependencies
320
+ */
321
+function demoteSubtreeToConditional(subtree: DependencyNode) {
322
+ const stack: Array<DependencyNode> = [subtree];
323
+
324
+ let node;
325
+ while ((node = stack.pop()) !== undefined) {
326
+ const { accessType, properties } = node;
327
+ invariant(isUnconditional(accessType), "");
328
+ node.accessType = isDependency(accessType)
329
+ ? PropertyAccessType.ConditionalDependency
330
+ : PropertyAccessType.ConditionalAccess;
331
+
332
+ for (const childNode of properties.values()) {
333
+ if (isUnconditional(accessType)) {
334
+ // No conditional node can have an unconditional node as a child, so
335
+ // we only process childNode if it is unconditional
336
+ stack.push(childNode);
337
+ }
338
+ }
339
+ }
340
+}
341
+
342
+/**
343
+ * Calculates currNode = union(currNode, otherNode), mutating currNode in place
344
+ * If demoteOtherNode is specified, we demote the subtree represented by
345
+ * otherNode to conditional access/deps before taking the union.
346
+ *
347
+ * This is a helper function used to join an inner scope to its parent scope.
348
+ * @param currNode (mutable) return by argument
349
+ * @param otherNode (move) {@link addSubtree} takes ownership of the subtree
350
+ * represented by otherNode, which may be mutated or moved to currNode. It is
351
+ * invalid to use otherNode after this call.
352
+ * @param demoteOtherNode
353
+ */
354
+function addSubtree(
355
+ currNode: DependencyNode,
356
+ otherNode: DependencyNode,
357
+ demoteOtherNode: boolean
358
+) {
359
+ let otherType = otherNode.accessType;
360
+ if (demoteOtherNode) {
361
+ otherType = isDependency(otherType)
362
+ ? PropertyAccessType.ConditionalDependency
363
+ : PropertyAccessType.ConditionalAccess;
364
+ }
365
+ currNode.accessType = merge(currNode.accessType, otherType);
366
+
367
+ for (const [propertyName, otherChild] of otherNode.properties) {
368
+ const currChild = currNode.properties.get(propertyName);
369
+ if (currChild) {
370
+ // recursively calculate currChild = union(currChild, otherChild)
371
+ addSubtree(currChild, otherChild, demoteOtherNode);
372
+ } else {
373
+ // if currChild doesn't exist, we can just move otherChild
374
+ // currChild = otherChild.
375
+ if (demoteOtherNode) {
376
+ demoteSubtreeToConditional(otherChild);
377
+ }
378
+ currNode.properties.set(propertyName, otherChild);
379
+ }
380
+ }
381
+}
382
+
383
/**
384
* Adds intersection(otherProperties) to currProperties, mutating
385
* currProperties in place. i.e.
compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts
+28
-21
@@ -98,18 +98,20 @@ class Context {
98
this.#dependencies = previousDependencies;
99
this.#inConditionalWithinScope = prevInConditional;
100
101
- const minScopeDependencies = scopedDependencies.deriveMinimalDependencies();
101
+ // Derive minimal dependencies now, since next line may mutate scopedDependencies
102
+ const minInnerScopeDependencies =
103
+ scopedDependencies.deriveMinimalDependencies();
104
+
105
// propagate dependencies upward using the same rules as normal dependency
106
// collection. child scopes may have dependencies on values created within
107
// the outer scope, which necessarily cannot be dependencies of the outer
108
// scope
106
- // TODO(@mofeiZ): instead of merging derived minimal dependencies here, we
107
- // can instead merge the scoped dependency tree. This would let us retain
108
- // info about unconditional accesses.
109
- for (const dep of minScopeDependencies) {
110
- this.visitDependency({ ...dep, cond: this.#inConditionalWithinScope });
111
- }
112
- return minScopeDependencies;
109
+ this.#dependencies.addDepsFromInnerScope(
110
+ scopedDependencies,
111
+ this.#inConditionalWithinScope,
112
+ this.#checkValidDependencyId.bind(this)
113
+ );
114
+ return minInnerScopeDependencies;
115
}
116
117
/**
@@ -202,6 +204,23 @@ class Context {
204
this.#properties.set(lvalue.identifier, nextDependency);
205
}
206
207
+ // Checks if identifier is a valid dependency in the current scope
208
+ #checkValidDependencyId(identifier: Identifier) {
209
+ // If this operand is used in a scope, has a dynamic value, and was defined
210
+ // before this scope, then its a dependency of the scope.
211
+ const currentDeclaration =
212
+ this.#reassignments.get(identifier) ??
213
+ this.#declarations.get(identifier.id);
214
+ const currentScope = this.currentScope;
215
+ return (
216
+ currentScope != null &&
217
+ currentDeclaration !== undefined &&
218
+ currentDeclaration.id < currentScope.range.start &&
219
+ (currentDeclaration.scope == null ||
220
+ currentDeclaration.scope !== currentScope)
221
+ );
222
+ }
223
+
224
#isScopeActive(scope: ReactiveScope): boolean {
225
return this.#scopes.indexOf(scope) !== -1;
226
}
@@ -277,19 +296,7 @@ class Context {
296
);
297
}
298
280
- // If this operand is used in a scope, has a dynamic value, and was defined
281
- // before this scope, then its a dependency of the scope.
282
- const currentDeclaration =
283
- this.#reassignments.get(maybeDependency.identifier) ??
284
- this.#declarations.get(maybeDependency.identifier.id);
285
- const currentScope = this.currentScope;
286
- if (
287
- currentScope != null &&
288
- currentDeclaration !== undefined &&
289
- currentDeclaration.id < currentScope.range.start &&
290
- (currentDeclaration.scope == null ||
291
- currentDeclaration.scope !== currentScope)
292
- ) {
299
+ if (this.#checkValidDependencyId(maybeDependency.identifier)) {
300
this.#depsInCurrentConditional.add({
301
...maybeDependency,
302
cond: true,
compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md
new
+94
@@ -0,0 +1,94 @@
1
+
2
+## Input
3
+
4
+```javascript
5
+// This tests an optimization, NOT a correctness property.
6
+// When propagating reactive dependencies of an inner scope up to its parent,
7
+// we prefer to retain granularity.
8
+//
9
+// In this test, we check that Forget propagates the inner scope's conditional
10
+// dependencies (e.g. props.a.b) instead of only its derived minimal
11
+// unconditional dependencies (e.g. props).
12
+// ```javascript
13
+// scope @0 (deps=[???] decls=[x, y]) {
14
+// let y = {};
15
+// scope @1 (deps=[props] decls=[x]) {
16
+// let x = {};
17
+// if (foo) mutate1(x, props.a.b);
18
+// }
19
+// mutate2(y, props.a.b);
20
+// }
21
+
22
+function TestJoinCondDepsInUncondScopes(props) {
23
+ let y = {};
24
+ let x = {};
25
+ if (foo) {
26
+ mutate1(x, props.a.b);
27
+ }
28
+ mutate2(y, props.a.b);
29
+ return [x, y];
30
+}
31
+
32
+```
33
+
34
+## Code
35
+
36
+```javascript
37
+// This tests an optimization, NOT a correctness property.
38
+// When propagating reactive dependencies of an inner scope up to its parent,
39
+// we prefer to retain granularity.
40
+//
41
+// In this test, we check that Forget propagates the inner scope's conditional
42
+// dependencies (e.g. props.a.b) instead of only its derived minimal
43
+// unconditional dependencies (e.g. props).
44
+// ```javascript
45
+// scope @0 (deps=[???] decls=[x, y]) {
46
+// let y = {};
47
+// scope @1 (deps=[props] decls=[x]) {
48
+// let x = {};
49
+// if (foo) mutate1(x, props.a.b);
50
+// }
51
+// mutate2(y, props.a.b);
52
+// }
53
+
54
+function TestJoinCondDepsInUncondScopes(props) {
55
+ const $ = React.unstable_useMemoCache(7);
56
+ const c_0 = $[0] !== props.a.b;
57
+ let y;
58
+ if (c_0) {
59
+ y = {};
60
+ const c_2 = $[2] !== props;
61
+ let x;
62
+ if (c_2) {
63
+ x = {};
64
+ if (foo) {
65
+ mutate1(x, props.a.b);
66
+ }
67
+ $[2] = props;
68
+ $[3] = x;
69
+ } else {
70
+ x = $[3];
71
+ }
72
+
73
+ mutate2(y, props.a.b);
74
+ $[0] = props.a.b;
75
+ $[1] = y;
76
+ } else {
77
+ y = $[1];
78
+ }
79
+ const c_4 = $[4] !== x;
80
+ const c_5 = $[5] !== y;
81
+ let t0;
82
+ if (c_4 || c_5) {
83
+ t0 = [x, y];
84
+ $[4] = x;
85
+ $[5] = y;
86
+ $[6] = t0;
87
+ } else {
88
+ t0 = $[6];
89
+ }
90
+ return t0;
91
+}
92
+
93
+```
94
+
\ No newline at end of file
compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.js
new
+26
@@ -0,0 +1,26 @@
1
+// This tests an optimization, NOT a correctness property.
2
+// When propagating reactive dependencies of an inner scope up to its parent,
3
+// we prefer to retain granularity.
4
+//
5
+// In this test, we check that Forget propagates the inner scope's conditional
6
+// dependencies (e.g. props.a.b) instead of only its derived minimal
7
+// unconditional dependencies (e.g. props).
8
+// ```javascript
9
+// scope @0 (deps=[???] decls=[x, y]) {
10
+// let y = {};
11
+// scope @1 (deps=[props] decls=[x]) {
12
+// let x = {};
13
+// if (foo) mutate1(x, props.a.b);
14
+// }
15
+// mutate2(y, props.a.b);
16
+// }
17
+
18
+function TestJoinCondDepsInUncondScopes(props) {
19
+ let y = {};
20
+ let x = {};
21
+ if (foo) {
22
+ mutate1(x, props.a.b);
23
+ }
24
+ mutate2(y, props.a.b);
25
+ return [x, y];
26
+}