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]; +}