diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts index 4f642e57a65f3..cb778c329226b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -4,6 +4,7 @@ import {Set_intersect, Set_union, getOrInsertDefault} from '../Utils/utils'; import { BasicBlock, BlockId, + DependencyPathEntry, GeneratedSource, HIRFunction, Identifier, @@ -66,7 +67,9 @@ export function collectHoistablePropertyLoads( fn: HIRFunction, temporaries: ReadonlyMap, ): ReadonlyMap { - const nodes = collectNonNullsInBlocks(fn, temporaries); + const registry = new PropertyPathRegistry(); + + const nodes = collectNonNullsInBlocks(fn, temporaries, registry); propagateNonNull(fn, nodes); const nodesKeyedByScopeId = new Map(); @@ -84,33 +87,33 @@ export function collectHoistablePropertyLoads( export type BlockInfo = { block: BasicBlock; - assumedNonNullObjects: ReadonlySet; + assumedNonNullObjects: ReadonlySet; }; /** - * Tree data structure to dedupe property loads (e.g. a.b.c) + * PropertyLoadRegistry data structure to dedupe property loads (e.g. a.b.c) * and make computing sets intersections simpler. */ type RootNode = { - properties: Map; + properties: Map; parent: null; // Recorded to make later computations simpler fullPath: ReactiveScopeDependency; root: IdentifierId; }; -type PropertyLoadNode = +type PropertyPathNode = | { - properties: Map; - parent: PropertyLoadNode; + properties: Map; + parent: PropertyPathNode; fullPath: ReactiveScopeDependency; } | RootNode; -class Tree { +class PropertyPathRegistry { roots: Map = new Map(); - getOrCreateRoot(identifier: Identifier): PropertyLoadNode { + getOrCreateIdentifier(identifier: Identifier): PropertyPathNode { /** * Reads from a statically scoped variable are always safe in JS, * with the exception of TDZ (not addressed by this pass). @@ -132,49 +135,61 @@ class Tree { return rootNode; } - static #getOrCreateProperty( - node: PropertyLoadNode, - property: string, - ): PropertyLoadNode { - let child = node.properties.get(property); + static getOrCreatePropertyEntry( + parent: PropertyPathNode, + entry: DependencyPathEntry, + ): PropertyPathNode { + if (entry.optional) { + CompilerError.throwTodo({ + reason: 'handle optional nodes', + loc: GeneratedSource, + }); + } + let child = parent.properties.get(entry.property); if (child == null) { child = { properties: new Map(), - parent: node, + parent: parent, fullPath: { - identifier: node.fullPath.identifier, - path: node.fullPath.path.concat([{property, optional: false}]), + identifier: parent.fullPath.identifier, + path: parent.fullPath.path.concat(entry), }, }; - node.properties.set(property, child); + parent.properties.set(entry.property, child); } return child; } - getPropertyLoadNode(n: ReactiveScopeDependency): PropertyLoadNode { + getOrCreateProperty(n: ReactiveScopeDependency): PropertyPathNode { /** * We add ReactiveScopeDependencies according to instruction ordering, * so all subpaths of a PropertyLoad should already exist * (e.g. a.b is added before a.b.c), */ - let currNode = this.getOrCreateRoot(n.identifier); + let currNode = this.getOrCreateIdentifier(n.identifier); if (n.path.length === 0) { return currNode; } for (let i = 0; i < n.path.length - 1; i++) { - currNode = assertNonNull(currNode.properties.get(n.path[i].property)); + currNode = PropertyPathRegistry.getOrCreatePropertyEntry( + currNode, + n.path[i], + ); } - return Tree.#getOrCreateProperty(currNode, n.path.at(-1)!.property); + return PropertyPathRegistry.getOrCreatePropertyEntry( + currNode, + n.path.at(-1)!, + ); } } -function pushPropertyLoadNode( - loadSource: Identifier, - loadSourceNode: PropertyLoadNode, +function addNonNullPropertyPath( + source: Identifier, + sourceNode: PropertyPathNode, instrId: InstructionId, knownImmutableIdentifiers: Set, - result: Set, + result: Set, ): void { /** * Since this runs *after* buildReactiveScopeTerminals, identifier mutable ranges @@ -187,26 +202,22 @@ function pushPropertyLoadNode( * See comment at top of function for why we track known immutable identifiers. */ const isMutableAtInstr = - loadSource.mutableRange.end > loadSource.mutableRange.start + 1 && - loadSource.scope != null && - inRange({id: instrId}, loadSource.scope.range); + source.mutableRange.end > source.mutableRange.start + 1 && + source.scope != null && + inRange({id: instrId}, source.scope.range); if ( !isMutableAtInstr || - knownImmutableIdentifiers.has(loadSourceNode.fullPath.identifier.id) + knownImmutableIdentifiers.has(sourceNode.fullPath.identifier.id) ) { - let curr: PropertyLoadNode | null = loadSourceNode; - while (curr != null) { - result.add(curr); - curr = curr.parent; - } + result.add(sourceNode); } } function collectNonNullsInBlocks( fn: HIRFunction, temporaries: ReadonlyMap, + registry: PropertyPathRegistry, ): ReadonlyMap { - const tree = new Tree(); /** * Due to current limitations of mutable range inference, there are edge cases in * which we infer known-immutable values (e.g. props or hook params) to have a @@ -227,18 +238,18 @@ function collectNonNullsInBlocks( * Known non-null objects such as functional component props can be safely * read from any block. */ - const knownNonNullIdentifiers = new Set(); + const knownNonNullIdentifiers = new Set(); if ( fn.fnType === 'Component' && fn.params.length > 0 && fn.params[0].kind === 'Identifier' ) { const identifier = fn.params[0].identifier; - knownNonNullIdentifiers.add(tree.getOrCreateRoot(identifier)); + knownNonNullIdentifiers.add(registry.getOrCreateIdentifier(identifier)); } const nodes = new Map(); for (const [_, block] of fn.body.blocks) { - const assumedNonNullObjects = new Set( + const assumedNonNullObjects = new Set( knownNonNullIdentifiers, ); for (const instr of block.instructions) { @@ -247,9 +258,9 @@ function collectNonNullsInBlocks( identifier: instr.value.object.identifier, path: [], }; - pushPropertyLoadNode( + addNonNullPropertyPath( instr.value.object.identifier, - tree.getPropertyLoadNode(source), + registry.getOrCreateProperty(source), instr.id, knownImmutableIdentifiers, assumedNonNullObjects, @@ -258,9 +269,9 @@ function collectNonNullsInBlocks( const source = instr.value.value.identifier.id; const sourceNode = temporaries.get(source); if (sourceNode != null) { - pushPropertyLoadNode( + addNonNullPropertyPath( instr.value.value.identifier, - tree.getPropertyLoadNode(sourceNode), + registry.getOrCreateProperty(sourceNode), instr.id, knownImmutableIdentifiers, assumedNonNullObjects, @@ -270,9 +281,9 @@ function collectNonNullsInBlocks( const source = instr.value.object.identifier.id; const sourceNode = temporaries.get(source); if (sourceNode != null) { - pushPropertyLoadNode( + addNonNullPropertyPath( instr.value.object.identifier, - tree.getPropertyLoadNode(sourceNode), + registry.getOrCreateProperty(sourceNode), instr.id, knownImmutableIdentifiers, assumedNonNullObjects, @@ -314,7 +325,6 @@ function propagateNonNull( nodeId: BlockId, direction: 'forward' | 'backward', traversalState: Map, - nonNullObjectsByBlock: Map>, ): boolean { /** * Avoid re-visiting computed or currently active nodes, which can @@ -345,7 +355,6 @@ function propagateNonNull( pred, direction, traversalState, - nonNullObjectsByBlock, ); changed ||= neighborChanged; } @@ -374,38 +383,36 @@ function propagateNonNull( const neighborAccesses = Set_intersect( Array.from(neighbors) .filter(n => traversalState.get(n) === 'done') - .map(n => assertNonNull(nonNullObjectsByBlock.get(n))), + .map(n => assertNonNull(nodes.get(n)).assumedNonNullObjects), ); - const prevObjects = assertNonNull(nonNullObjectsByBlock.get(nodeId)); - const newObjects = Set_union(prevObjects, neighborAccesses); + const prevObjects = assertNonNull(nodes.get(nodeId)).assumedNonNullObjects; + const mergedObjects = Set_union(prevObjects, neighborAccesses); - nonNullObjectsByBlock.set(nodeId, newObjects); + assertNonNull(nodes.get(nodeId)).assumedNonNullObjects = mergedObjects; traversalState.set(nodeId, 'done'); - changed ||= prevObjects.size !== newObjects.size; + changed ||= prevObjects.size !== mergedObjects.size; return changed; } - const fromEntry = new Map>(); - const fromExit = new Map>(); - for (const [blockId, blockInfo] of nodes) { - fromEntry.set(blockId, blockInfo.assumedNonNullObjects); - fromExit.set(blockId, blockInfo.assumedNonNullObjects); - } const traversalState = new Map(); const reversedBlocks = [...fn.body.blocks]; reversedBlocks.reverse(); - let i = 0; let changed; + let i = 0; do { - i++; + CompilerError.invariant(i++ < 100, { + reason: + '[CollectHoistablePropertyLoads] fixed point iteration did not terminate after 100 loops', + loc: GeneratedSource, + }); + changed = false; for (const [blockId] of fn.body.blocks) { const forwardChanged = recursivelyPropagateNonNull( blockId, 'forward', traversalState, - fromEntry, ); changed ||= forwardChanged; } @@ -415,43 +422,14 @@ function propagateNonNull( blockId, 'backward', traversalState, - fromExit, ); changed ||= backwardChanged; } traversalState.clear(); } while (changed); - - /** - * TODO: validate against meta internal code, then remove in future PR. - * Currently cannot come up with a case that requires fixed-point iteration. - */ - CompilerError.invariant(i <= 2, { - reason: 'require fixed-point iteration', - description: `#iterations = ${i}`, - loc: GeneratedSource, - }); - - CompilerError.invariant( - fromEntry.size === fromExit.size && fromEntry.size === nodes.size, - { - reason: - 'bad sizes after calculating fromEntry + fromExit ' + - `${fromEntry.size} ${fromExit.size} ${nodes.size}`, - loc: GeneratedSource, - }, - ); - - for (const [id, node] of nodes) { - const assumedNonNullObjects = Set_union( - assertNonNull(fromEntry.get(id)), - assertNonNull(fromExit.get(id)), - ); - node.assumedNonNullObjects = assumedNonNullObjects; - } } -function assertNonNull, U>( +export function assertNonNull, U>( value: T | null | undefined, source?: string, ): T { diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts index ecc1844b006aa..f2bb0b31f05c9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/DeriveMinimalDependenciesHIR.ts @@ -19,16 +19,17 @@ const ENABLE_DEBUG_INVARIANTS = true; export class ReactiveScopeDependencyTreeHIR { #roots: Map = new Map(); - #getOrCreateRoot(identifier: Identifier, isNonNull: boolean): DependencyNode { + #getOrCreateRoot( + identifier: Identifier, + accessType: PropertyAccessType, + ): DependencyNode { // roots can always be accessed unconditionally in JS let rootNode = this.#roots.get(identifier); if (rootNode === undefined) { rootNode = { properties: new Map(), - accessType: isNonNull - ? PropertyAccessType.NonNullAccess - : PropertyAccessType.Access, + accessType, }; this.#roots.set(identifier, rootNode); } @@ -37,7 +38,7 @@ export class ReactiveScopeDependencyTreeHIR { addDependency(dep: ReactiveScopePropertyDependency): void { const {path} = dep; - let currNode = this.#getOrCreateRoot(dep.identifier, false); + let currNode = this.#getOrCreateRoot(dep.identifier, MIN_ACCESS_TYPE); const accessType = PropertyAccessType.Access; @@ -45,8 +46,11 @@ export class ReactiveScopeDependencyTreeHIR { for (const property of path) { // all properties read 'on the way' to a dependency are marked as 'access' - let currChild = getOrMakeProperty(currNode, property.property); - currChild.accessType = merge(currChild.accessType, accessType); + let currChild = makeOrMergeProperty( + currNode, + property.property, + accessType, + ); currNode = currChild; } @@ -251,17 +255,20 @@ function printSubtree( return results; } -function getOrMakeProperty( +function makeOrMergeProperty( node: DependencyNode, property: string, + accessType: PropertyAccessType, ): DependencyNode { let child = node.properties.get(property); if (child == null) { child = { properties: new Map(), - accessType: MIN_ACCESS_TYPE, + accessType, }; node.properties.set(property, child); + } else { + child.accessType = merge(child.accessType, accessType); } return child; } diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts index 30408ab032b35..615ec18feb962 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts @@ -874,13 +874,7 @@ export type InstructionValue = }; loc: SourceLocation; } - | { - kind: 'StoreLocal'; - lvalue: LValue; - value: Place; - type: t.FlowType | t.TSType | null; - loc: SourceLocation; - } + | StoreLocal | { kind: 'StoreContext'; lvalue: { @@ -1123,6 +1117,13 @@ export type Primitive = { export type JSXText = {kind: 'JSXText'; value: string; loc: SourceLocation}; +export type StoreLocal = { + kind: 'StoreLocal'; + lvalue: LValue; + value: Place; + type: t.FlowType | t.TSType | null; + loc: SourceLocation; +}; export type PropertyLoad = { kind: 'PropertyLoad'; object: Place; @@ -1496,7 +1497,8 @@ export type ReactiveScopeDeclaration = { scope: ReactiveScope; // the scope in which the variable was originally declared }; -export type DependencyPath = Array<{property: string; optional: boolean}>; +export type DependencyPathEntry = {property: string; optional: boolean}; +export type DependencyPath = Array; export type ReactiveScopeDependency = { identifier: Identifier; path: DependencyPath;