From 8be45e7d4ae7b583439fea69a0d1531430e7fac6 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 6 Mar 2023 16:33:08 -0500 Subject: [PATCH] [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); } ``` --- .../DeriveMinimalDependencies.ts | 107 ++++++++++++++++-- .../PropagateScopeDependencies.ts | 49 ++++---- ...eps-join-uncond-scopes-cond-deps.expect.md | 94 +++++++++++++++ ...ctive-deps-join-uncond-scopes-cond-deps.js | 26 +++++ 4 files changed, 247 insertions(+), 29 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.js diff --git a/compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts b/compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts index fb075943f4..d62ef4e168 100644 --- a/compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/DeriveMinimalDependencies.ts @@ -32,18 +32,23 @@ export type ReactiveScopeDependencyInfo = ReactiveScopeDependency & { export class ReactiveScopeDependencyTree { #roots: Map = new Map(); - add(dep: ReactiveScopeDependencyInfo) { - let root = this.#roots.get(dep.identifier); - const path = dep.path ?? []; - if (root == null) { - // roots can always be accessed unconditionally in JS - root = { + #getOrCreateRoot(identifier: Identifier): DependencyNode { + // roots can always be accessed unconditionally in JS + let rootNode = this.#roots.get(identifier); + + if (rootNode === undefined) { + rootNode = { properties: new Map(), accessType: PropertyAccessType.UnconditionalAccess, }; - this.#roots.set(dep.identifier, root); + this.#roots.set(identifier, rootNode); } - let currNode: DependencyNode = root; + return rootNode; + } + + add(dep: ReactiveScopeDependencyInfo) { + const path = dep.path ?? []; + let currNode = this.#getOrCreateRoot(dep.identifier); const accessType = dep.cond ? PropertyAccessType.ConditionalAccess @@ -93,6 +98,25 @@ export class ReactiveScopeDependencyTree { return results; } + addDepsFromInnerScope( + depsFromInnerScope: ReactiveScopeDependencyTree, + innerScopeInConditionalWithinParent: boolean, + checkValidDepIdFn: (id: Identifier) => boolean + ) { + for (const [id, otherRoot] of depsFromInnerScope.#roots) { + if (!checkValidDepIdFn(id)) { + continue; + } + let currRoot = this.#getOrCreateRoot(id); + addSubtree(currRoot, otherRoot, innerScopeInConditionalWithinParent); + if (!isUnconditional(currRoot.accessType)) { + currRoot.accessType = isDependency(currRoot.accessType) + ? PropertyAccessType.UnconditionalDependency + : PropertyAccessType.UnconditionalAccess; + } + } + } + promoteDepsFromExhaustiveConditionals( trees: Array ) { @@ -289,6 +313,73 @@ function deriveMinimalDependenciesInSubtree( } } +/** + * Demote all unconditional accesses + dependencies in subtree to the + * conditional equivalent, mutating subtree in place. + * @param subtree unconditional node representing a subtree of dependencies + */ +function demoteSubtreeToConditional(subtree: DependencyNode) { + const stack: Array = [subtree]; + + let node; + while ((node = stack.pop()) !== undefined) { + const { accessType, properties } = node; + invariant(isUnconditional(accessType), ""); + node.accessType = isDependency(accessType) + ? PropertyAccessType.ConditionalDependency + : PropertyAccessType.ConditionalAccess; + + for (const childNode of properties.values()) { + if (isUnconditional(accessType)) { + // No conditional node can have an unconditional node as a child, so + // we only process childNode if it is unconditional + stack.push(childNode); + } + } + } +} + +/** + * Calculates currNode = union(currNode, otherNode), mutating currNode in place + * If demoteOtherNode is specified, we demote the subtree represented by + * otherNode to conditional access/deps before taking the union. + * + * This is a helper function used to join an inner scope to its parent scope. + * @param currNode (mutable) return by argument + * @param otherNode (move) {@link addSubtree} takes ownership of the subtree + * represented by otherNode, which may be mutated or moved to currNode. It is + * invalid to use otherNode after this call. + * @param demoteOtherNode + */ +function addSubtree( + currNode: DependencyNode, + otherNode: DependencyNode, + demoteOtherNode: boolean +) { + let otherType = otherNode.accessType; + if (demoteOtherNode) { + otherType = isDependency(otherType) + ? PropertyAccessType.ConditionalDependency + : PropertyAccessType.ConditionalAccess; + } + currNode.accessType = merge(currNode.accessType, otherType); + + for (const [propertyName, otherChild] of otherNode.properties) { + const currChild = currNode.properties.get(propertyName); + if (currChild) { + // recursively calculate currChild = union(currChild, otherChild) + addSubtree(currChild, otherChild, demoteOtherNode); + } else { + // if currChild doesn't exist, we can just move otherChild + // currChild = otherChild. + if (demoteOtherNode) { + demoteSubtreeToConditional(otherChild); + } + currNode.properties.set(propertyName, otherChild); + } + } +} + /** * Adds intersection(otherProperties) to currProperties, mutating * currProperties in place. i.e. diff --git a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts index 07bf316cf9..0a2d34cc51 100644 --- a/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts +++ b/compiler/forget/src/ReactiveScopes/PropagateScopeDependencies.ts @@ -98,18 +98,20 @@ class Context { this.#dependencies = previousDependencies; this.#inConditionalWithinScope = prevInConditional; - const minScopeDependencies = scopedDependencies.deriveMinimalDependencies(); + // Derive minimal dependencies now, since next line may mutate scopedDependencies + const minInnerScopeDependencies = + scopedDependencies.deriveMinimalDependencies(); + // propagate dependencies upward using the same rules as normal dependency // collection. child scopes may have dependencies on values created within // the outer scope, which necessarily cannot be dependencies of the outer // scope - // TODO(@mofeiZ): instead of merging derived minimal dependencies here, we - // can instead merge the scoped dependency tree. This would let us retain - // info about unconditional accesses. - for (const dep of minScopeDependencies) { - this.visitDependency({ ...dep, cond: this.#inConditionalWithinScope }); - } - return minScopeDependencies; + this.#dependencies.addDepsFromInnerScope( + scopedDependencies, + this.#inConditionalWithinScope, + this.#checkValidDependencyId.bind(this) + ); + return minInnerScopeDependencies; } /** @@ -202,6 +204,23 @@ class Context { this.#properties.set(lvalue.identifier, nextDependency); } + // Checks if identifier is a valid dependency in the current scope + #checkValidDependencyId(identifier: Identifier) { + // If this operand is used in a scope, has a dynamic value, and was defined + // before this scope, then its a dependency of the scope. + const currentDeclaration = + this.#reassignments.get(identifier) ?? + this.#declarations.get(identifier.id); + const currentScope = this.currentScope; + return ( + currentScope != null && + currentDeclaration !== undefined && + currentDeclaration.id < currentScope.range.start && + (currentDeclaration.scope == null || + currentDeclaration.scope !== currentScope) + ); + } + #isScopeActive(scope: ReactiveScope): boolean { return this.#scopes.indexOf(scope) !== -1; } @@ -277,19 +296,7 @@ class Context { ); } - // If this operand is used in a scope, has a dynamic value, and was defined - // before this scope, then its a dependency of the scope. - const currentDeclaration = - this.#reassignments.get(maybeDependency.identifier) ?? - this.#declarations.get(maybeDependency.identifier.id); - const currentScope = this.currentScope; - if ( - currentScope != null && - currentDeclaration !== undefined && - currentDeclaration.id < currentScope.range.start && - (currentDeclaration.scope == null || - currentDeclaration.scope !== currentScope) - ) { + if (this.#checkValidDependencyId(maybeDependency.identifier)) { this.#depsInCurrentConditional.add({ ...maybeDependency, cond: true, diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md new file mode 100644 index 0000000000..0c1f931056 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.expect.md @@ -0,0 +1,94 @@ + +## Input + +```javascript +// This tests an optimization, NOT a correctness property. +// When propagating reactive dependencies of an inner scope up to its parent, +// we prefer to retain granularity. +// +// In this test, we check that Forget propagates the inner scope's conditional +// dependencies (e.g. props.a.b) instead of only its derived minimal +// unconditional dependencies (e.g. props). +// ```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); +// } + +function TestJoinCondDepsInUncondScopes(props) { + let y = {}; + let x = {}; + if (foo) { + mutate1(x, props.a.b); + } + mutate2(y, props.a.b); + return [x, y]; +} + +``` + +## Code + +```javascript +// This tests an optimization, NOT a correctness property. +// When propagating reactive dependencies of an inner scope up to its parent, +// we prefer to retain granularity. +// +// In this test, we check that Forget propagates the inner scope's conditional +// dependencies (e.g. props.a.b) instead of only its derived minimal +// unconditional dependencies (e.g. props). +// ```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); +// } + +function TestJoinCondDepsInUncondScopes(props) { + const $ = React.unstable_useMemoCache(7); + const c_0 = $[0] !== props.a.b; + let y; + if (c_0) { + y = {}; + const c_2 = $[2] !== props; + let x; + if (c_2) { + x = {}; + if (foo) { + mutate1(x, props.a.b); + } + $[2] = props; + $[3] = x; + } else { + x = $[3]; + } + + mutate2(y, props.a.b); + $[0] = props.a.b; + $[1] = y; + } else { + y = $[1]; + } + const c_4 = $[4] !== x; + const c_5 = $[5] !== y; + let t0; + if (c_4 || c_5) { + t0 = [x, y]; + $[4] = x; + $[5] = y; + $[6] = t0; + } else { + t0 = $[6]; + } + return t0; +} + +``` + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.js b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.js new file mode 100644 index 0000000000..96917558e9 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/hir/reduce-reactive-deps-join-uncond-scopes-cond-deps.js @@ -0,0 +1,26 @@ +// This tests an optimization, NOT a correctness property. +// When propagating reactive dependencies of an inner scope up to its parent, +// we prefer to retain granularity. +// +// In this test, we check that Forget propagates the inner scope's conditional +// dependencies (e.g. props.a.b) instead of only its derived minimal +// unconditional dependencies (e.g. props). +// ```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); +// } + +function TestJoinCondDepsInUncondScopes(props) { + let y = {}; + let x = {}; + if (foo) { + mutate1(x, props.a.b); + } + mutate2(y, props.a.b); + return [x, y]; +}