Skip to content

Commit 0751fac

Browse files
committed
[compiler] Optional chaining for dependencies (HIR rewrite)
Adds HIR version of `PropagateScopeDeps` to handle optional chaining. Internally, this improves memoization on ~4% of compiled files (internal links: [1](https://www.internalfb.com/intern/paste/P1610406497/)) Summarizing the changes in this PR. 1. `CollectOptionalChainDependencies` recursively traverses optional blocks down to the base. From the base, we build up a set of `baseIdentifier.propertyA?.propertyB` mappings. The tricky bit here is that optional blocks sometimes reference other optional blocks that are *not* part of the same chain e.g. a(c?.d)?.d. See code + comments in `traverseOptionalBlock` for how we avoid concatenating unrelated blocks. 2. Adding optional chains into non-null object calculation. (Note that marking `a?.b` as 'non-null' means that `a?.b.c` is safe to evaluate, *not* `(a?.b).c`. Happy to rename this / reword comments accordingly if there's a better term) This pass is split into two stages. (1) collecting non-null objects by block and (2) propagating non-null objects across blocks. The only significant change here was to (2). We add an extra reduce step `X=Reduce(Union(X, Intersect(X_neighbors)))` to merge optional and non-optional nodes (e.g. nonNulls=`{a, a?.b}` reduces to `{a, a.b}`) 3. Adding optional chains into dependency calculation. This was the trickiest. We need to take the "maximal" property chain as a dependency. Prior to this PR, we avoided taking subpaths e.g. `a.b` of `a.b.c` as dependencies by only visiting non-PropertyLoad/LoadLocal instructions. This effectively only recorded the property-path at site-of-use. Unfortunately, this *quite* doesn't work for optional chains for a few reasons: - We would need to skip relevant `StoreLocal`/`Branch terminal` instructions (but only those within optional blocks that have been successfully read). - Given an optional chain, either (1) only a subpath or (2) the entire path can be represented as a PropertyLoad. We cannot directly add the last hoistable optional-block as a dependency as MethodCalls are an edge case e.g. given a?.b.c(), we should depend on `a?.b`, not `a?.b.c` This means that we add its dependency at either the innermost unhoistable optional-block or when encountering it within its phi-join. 4. Handle optional chains in DeriveMinimalDependenciesHIR. This was also a bit tricky to formulate. Ideally, we would avoid a 2^3 case join (cond | uncond cfg, optional | not optional load, access | dependency). This PR attempts to simplify by building two trees 1. First add each hoistable path into a tree containing `Optional | NonOptional` nodes. 2. Then add each dependency into another tree containing `Optional | NonOptional`, `Access | Dependency` nodes, truncating the dependency at the earliest non-hoistable node (i.e. non-matching pair when walking the hoistable tree) ghstack-source-id: a2170f2 Pull Request resolved: #31037
1 parent 459fd41 commit 0751fac

File tree

37 files changed

+2221
-407
lines changed

37 files changed

+2221
-407
lines changed

compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts

Lines changed: 108 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
import {CompilerError} from '../CompilerError';
22
import {inRange} from '../ReactiveScopes/InferReactiveScopeVariables';
3-
import {Set_intersect, Set_union, getOrInsertDefault} from '../Utils/utils';
3+
import {
4+
Set_equal,
5+
Set_filter,
6+
Set_intersect,
7+
Set_union,
8+
getOrInsertDefault,
9+
} from '../Utils/utils';
410
import {
511
BasicBlock,
612
BlockId,
@@ -15,9 +21,9 @@ import {
1521
} from './HIR';
1622

1723
/**
18-
* Helper function for `PropagateScopeDependencies`.
19-
* Uses control flow graph analysis to determine which `Identifier`s can
20-
* be assumed to be non-null objects, on a per-block basis.
24+
* Helper function for `PropagateScopeDependencies`. Uses control flow graph
25+
* analysis to determine which `Identifier`s can be assumed to be non-null
26+
* objects, on a per-block basis.
2127
*
2228
* Here is an example:
2329
* ```js
@@ -42,15 +48,16 @@ import {
4248
* }
4349
* ```
4450
*
45-
* Note that we currently do NOT account for mutable / declaration range
46-
* when doing the CFG-based traversal, producing results that are technically
51+
* Note that we currently do NOT account for mutable / declaration range when
52+
* doing the CFG-based traversal, producing results that are technically
4753
* incorrect but filtered by PropagateScopeDeps (which only takes dependencies
4854
* on constructed value -- i.e. a scope's dependencies must have mutable ranges
4955
* ending earlier than the scope start).
5056
*
51-
* Take this example, this function will infer x.foo.bar as non-nullable for bb0,
52-
* via the intersection of bb1 & bb2 which in turn comes from bb3. This is technically
53-
* incorrect bb0 is before / during x's mutable range.
57+
* Take this example, this function will infer x.foo.bar as non-nullable for
58+
* bb0, via the intersection of bb1 & bb2 which in turn comes from bb3. This is
59+
* technically incorrect bb0 is before / during x's mutable range.
60+
* ```
5461
* bb0:
5562
* const x = ...;
5663
* if cond then bb1 else bb2
@@ -62,15 +69,30 @@ import {
6269
* goto bb3:
6370
* bb3:
6471
* x.foo.bar
72+
* ```
73+
*
74+
* @param fn
75+
* @param temporaries sidemap of identifier -> baseObject.a.b paths. Does not
76+
* contain optional chains.
77+
* @param hoistableFromOptionals sidemap of optionalBlock -> baseObject?.a
78+
* optional paths for which it's safe to evaluate non-optional loads (see
79+
* CollectOptionalChainDependencies).
80+
* @returns
6581
*/
6682
export function collectHoistablePropertyLoads(
6783
fn: HIRFunction,
6884
temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>,
85+
hoistableFromOptionals: ReadonlyMap<BlockId, ReactiveScopeDependency>,
6986
): ReadonlyMap<ScopeId, BlockInfo> {
7087
const registry = new PropertyPathRegistry();
7188

72-
const nodes = collectNonNullsInBlocks(fn, temporaries, registry);
73-
propagateNonNull(fn, nodes);
89+
const nodes = collectNonNullsInBlocks(
90+
fn,
91+
temporaries,
92+
hoistableFromOptionals,
93+
registry,
94+
);
95+
propagateNonNull(fn, nodes, registry);
7496

7597
const nodesKeyedByScopeId = new Map<ScopeId, BlockInfo>();
7698
for (const [_, block] of fn.body.blocks) {
@@ -96,17 +118,21 @@ export type BlockInfo = {
96118
*/
97119
type RootNode = {
98120
properties: Map<string, PropertyPathNode>;
121+
optionalProperties: Map<string, PropertyPathNode>;
99122
parent: null;
100123
// Recorded to make later computations simpler
101124
fullPath: ReactiveScopeDependency;
125+
hasOptional: boolean;
102126
root: IdentifierId;
103127
};
104128

105129
type PropertyPathNode =
106130
| {
107131
properties: Map<string, PropertyPathNode>;
132+
optionalProperties: Map<string, PropertyPathNode>;
108133
parent: PropertyPathNode;
109134
fullPath: ReactiveScopeDependency;
135+
hasOptional: boolean;
110136
}
111137
| RootNode;
112138

@@ -124,10 +150,12 @@ class PropertyPathRegistry {
124150
rootNode = {
125151
root: identifier.id,
126152
properties: new Map(),
153+
optionalProperties: new Map(),
127154
fullPath: {
128155
identifier,
129156
path: [],
130157
},
158+
hasOptional: false,
131159
parent: null,
132160
};
133161
this.roots.set(identifier.id, rootNode);
@@ -139,23 +167,20 @@ class PropertyPathRegistry {
139167
parent: PropertyPathNode,
140168
entry: DependencyPathEntry,
141169
): PropertyPathNode {
142-
if (entry.optional) {
143-
CompilerError.throwTodo({
144-
reason: 'handle optional nodes',
145-
loc: GeneratedSource,
146-
});
147-
}
148-
let child = parent.properties.get(entry.property);
170+
const map = entry.optional ? parent.optionalProperties : parent.properties;
171+
let child = map.get(entry.property);
149172
if (child == null) {
150173
child = {
151174
properties: new Map(),
175+
optionalProperties: new Map(),
152176
parent: parent,
153177
fullPath: {
154178
identifier: parent.fullPath.identifier,
155179
path: parent.fullPath.path.concat(entry),
156180
},
181+
hasOptional: parent.hasOptional || entry.optional,
157182
};
158-
parent.properties.set(entry.property, child);
183+
map.set(entry.property, child);
159184
}
160185
return child;
161186
}
@@ -216,6 +241,7 @@ function addNonNullPropertyPath(
216241
function collectNonNullsInBlocks(
217242
fn: HIRFunction,
218243
temporaries: ReadonlyMap<IdentifierId, ReactiveScopeDependency>,
244+
hoistableFromOptionals: ReadonlyMap<BlockId, ReactiveScopeDependency>,
219245
registry: PropertyPathRegistry,
220246
): ReadonlyMap<BlockId, BlockInfo> {
221247
/**
@@ -252,6 +278,13 @@ function collectNonNullsInBlocks(
252278
const assumedNonNullObjects = new Set<PropertyPathNode>(
253279
knownNonNullIdentifiers,
254280
);
281+
282+
const maybeOptionalChain = hoistableFromOptionals.get(block.id);
283+
if (maybeOptionalChain != null) {
284+
assumedNonNullObjects.add(
285+
registry.getOrCreateProperty(maybeOptionalChain),
286+
);
287+
}
255288
for (const instr of block.instructions) {
256289
if (instr.value.kind === 'PropertyLoad') {
257290
const source = temporaries.get(instr.value.object.identifier.id) ?? {
@@ -303,6 +336,7 @@ function collectNonNullsInBlocks(
303336
function propagateNonNull(
304337
fn: HIRFunction,
305338
nodes: ReadonlyMap<BlockId, BlockInfo>,
339+
registry: PropertyPathRegistry,
306340
): void {
307341
const blockSuccessors = new Map<BlockId, Set<BlockId>>();
308342
const terminalPreds = new Set<BlockId>();
@@ -388,10 +422,17 @@ function propagateNonNull(
388422

389423
const prevObjects = assertNonNull(nodes.get(nodeId)).assumedNonNullObjects;
390424
const mergedObjects = Set_union(prevObjects, neighborAccesses);
425+
reduceMaybeOptionalChains(mergedObjects, registry);
391426

392427
assertNonNull(nodes.get(nodeId)).assumedNonNullObjects = mergedObjects;
393428
traversalState.set(nodeId, 'done');
394-
changed ||= prevObjects.size !== mergedObjects.size;
429+
/**
430+
* Note that it's not sufficient to compare set sizes since
431+
* reduceMaybeOptionalChains may replace optional-chain loads with
432+
* unconditional loads. This could in turn change `assumedNonNullObjects` of
433+
* downstream blocks and backedges.
434+
*/
435+
changed ||= !Set_equal(prevObjects, mergedObjects);
395436
return changed;
396437
}
397438
const traversalState = new Map<BlockId, 'done' | 'active'>();
@@ -440,3 +481,50 @@ export function assertNonNull<T extends NonNullable<U>, U>(
440481
});
441482
return value;
442483
}
484+
485+
/**
486+
* Any two optional chains with different operations . vs ?. but the same set of
487+
* property strings paths de-duplicates.
488+
*
489+
* Intuitively: given <base>?.b, we know <base> to be either hoistable or not.
490+
* If unconditional reads from <base> are hoistable, we can replace all
491+
* <base>?.PROPERTY_STRING subpaths with <base>.PROPERTY_STRING
492+
*/
493+
function reduceMaybeOptionalChains(
494+
nodes: Set<PropertyPathNode>,
495+
registry: PropertyPathRegistry,
496+
): void {
497+
let optionalChainNodes = Set_filter(nodes, n => n.hasOptional);
498+
if (optionalChainNodes.size === 0) {
499+
return;
500+
}
501+
let changed: boolean;
502+
do {
503+
changed = false;
504+
505+
for (const original of optionalChainNodes) {
506+
let {identifier, path: origPath} = original.fullPath;
507+
let currNode: PropertyPathNode =
508+
registry.getOrCreateIdentifier(identifier);
509+
for (let i = 0; i < origPath.length; i++) {
510+
const entry = origPath[i];
511+
// If the base is known to be non-null, replace with a non-optional load
512+
const nextEntry: DependencyPathEntry =
513+
entry.optional && nodes.has(currNode)
514+
? {property: entry.property, optional: false}
515+
: entry;
516+
currNode = PropertyPathRegistry.getOrCreatePropertyEntry(
517+
currNode,
518+
nextEntry,
519+
);
520+
}
521+
if (currNode !== original) {
522+
changed = true;
523+
optionalChainNodes.delete(original);
524+
optionalChainNodes.add(currNode);
525+
nodes.delete(original);
526+
nodes.add(currNode);
527+
}
528+
}
529+
} while (changed);
530+
}

0 commit comments

Comments
 (0)