Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[compiler] Delete LoweredFunction.dependencies and hoisted instructions #32096

Merged
merged 1 commit into from
Feb 18, 2025
Merged

[compiler] Delete LoweredFunction.dependencies and hoisted instructions #32096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 12 additions & 142 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/BuildHIR.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import {NodePath, Scope} from '@babel/traverse';
import * as t from '@babel/types';
import {Expression} from '@babel/types';
import invariant from 'invariant';
import {
CompilerError,
Expand Down Expand Up @@ -75,7 +74,7 @@ export function lower(
parent: NodePath<t.Function> | null = null,
): Result<HIRFunction, CompilerError> {
const builder = new HIRBuilder(env, parent ?? func, bindings, capturedRefs);
const context: Array<Place> = [];
const context: HIRFunction['context'] = [];

for (const ref of capturedRefs ?? []) {
context.push({
Expand Down Expand Up @@ -3378,7 +3377,7 @@ function lowerFunction(
>,
): LoweredFunction | null {
const componentScope: Scope = builder.parentFunction.scope;
const captured = gatherCapturedDeps(builder, expr, componentScope);
const capturedContext = gatherCapturedContext(expr, componentScope);

/*
* TODO(gsn): In the future, we could only pass in the context identifiers
Expand All @@ -3392,7 +3391,7 @@ function lowerFunction(
expr,
builder.environment,
builder.bindings,
[...builder.context, ...captured.identifiers],
[...builder.context, ...capturedContext],
builder.parentFunction,
);
let loweredFunc: HIRFunction;
Expand All @@ -3405,7 +3404,6 @@ function lowerFunction(
loweredFunc = lowering.unwrap();
return {
func: loweredFunc,
dependencies: captured.refs,
};
}

Expand Down Expand Up @@ -4079,14 +4077,6 @@ function lowerAssignment(
}
}

function isValidDependency(path: NodePath<t.MemberExpression>): boolean {
const parent: NodePath<t.Node> = path.parentPath;
return (
!path.node.computed &&
!(parent.isCallExpression() && parent.get('callee') === path)
);
}

function captureScopes({from, to}: {from: Scope; to: Scope}): Set<Scope> {
let scopes: Set<Scope> = new Set();
while (from) {
Expand All @@ -4101,19 +4091,16 @@ function captureScopes({from, to}: {from: Scope; to: Scope}): Set<Scope> {
return scopes;
}

function gatherCapturedDeps(
builder: HIRBuilder,
function gatherCapturedContext(
fn: NodePath<
| t.FunctionExpression
| t.ArrowFunctionExpression
| t.FunctionDeclaration
| t.ObjectMethod
>,
componentScope: Scope,
): {identifiers: Array<t.Identifier>; refs: Array<Place>} {
const capturedIds: Map<t.Identifier, number> = new Map();
const capturedRefs: Set<Place> = new Set();
const seenPaths: Set<string> = new Set();
): Array<t.Identifier> {
const capturedIds = new Set<t.Identifier>();

/*
* Capture all the scopes from the parent of this function up to and including
Expand All @@ -4124,33 +4111,11 @@ function gatherCapturedDeps(
to: componentScope,
});

function addCapturedId(bindingIdentifier: t.Identifier): number {
if (!capturedIds.has(bindingIdentifier)) {
const index = capturedIds.size;
capturedIds.set(bindingIdentifier, index);
return index;
} else {
return capturedIds.get(bindingIdentifier)!;
}
}

function handleMaybeDependency(
path:
| NodePath<t.MemberExpression>
| NodePath<t.Identifier>
| NodePath<t.JSXOpeningElement>,
path: NodePath<t.Identifier> | NodePath<t.JSXOpeningElement>,
): void {
// Base context variable to depend on
let baseIdentifier: NodePath<t.Identifier> | NodePath<t.JSXIdentifier>;
/*
* Base expression to depend on, which (for now) may contain non side-effectful
* member expressions
*/
let dependency:
| NodePath<t.MemberExpression>
| NodePath<t.JSXMemberExpression>
| NodePath<t.Identifier>
| NodePath<t.JSXIdentifier>;
if (path.isJSXOpeningElement()) {
const name = path.get('name');
if (!(name.isJSXMemberExpression() || name.isJSXIdentifier())) {
Expand All @@ -4166,115 +4131,20 @@ function gatherCapturedDeps(
'Invalid logic in gatherCapturedDeps',
);
baseIdentifier = current;

/*
* Get the expression to depend on, which may involve PropertyLoads
* for member expressions
*/
let currentDep:
| NodePath<t.JSXMemberExpression>
| NodePath<t.Identifier>
| NodePath<t.JSXIdentifier> = baseIdentifier;

while (true) {
const nextDep: null | NodePath<t.Node> = currentDep.parentPath;
if (nextDep && nextDep.isJSXMemberExpression()) {
currentDep = nextDep;
} else {
break;
}
}
dependency = currentDep;
} else if (path.isMemberExpression()) {
// Calculate baseIdentifier
let currentId: NodePath<Expression> = path;
while (currentId.isMemberExpression()) {
currentId = currentId.get('object');
}
if (!currentId.isIdentifier()) {
return;
}
baseIdentifier = currentId;

/*
* Get the expression to depend on, which may involve PropertyLoads
* for member expressions
*/
let currentDep:
| NodePath<t.MemberExpression>
| NodePath<t.Identifier>
| NodePath<t.JSXIdentifier> = baseIdentifier;

while (true) {
const nextDep: null | NodePath<t.Node> = currentDep.parentPath;
if (
nextDep &&
nextDep.isMemberExpression() &&
isValidDependency(nextDep)
) {
currentDep = nextDep;
} else {
break;
}
}

dependency = currentDep;
} else {
baseIdentifier = path;
dependency = path;
}

/*
* Skip dependency path, as we already tried to recursively add it (+ all subexpressions)
* as a dependency.
*/
dependency.skip();
path.skip();

// Add the base identifier binding as a dependency.
const binding = baseIdentifier.scope.getBinding(baseIdentifier.node.name);
if (binding === undefined || !pureScopes.has(binding.scope)) {
return;
}
const idKey = String(addCapturedId(binding.identifier));

// Add the expression (potentially a memberexpr path) as a dependency.
let exprKey = idKey;
if (dependency.isMemberExpression()) {
let pathTokens = [];
let current: NodePath<Expression> = dependency;
while (current.isMemberExpression()) {
const property = current.get('property') as NodePath<t.Identifier>;
pathTokens.push(property.node.name);
current = current.get('object');
}

exprKey += '.' + pathTokens.reverse().join('.');
} else if (dependency.isJSXMemberExpression()) {
let pathTokens = [];
let current: NodePath<t.JSXMemberExpression | t.JSXIdentifier> =
dependency;
while (current.isJSXMemberExpression()) {
const property = current.get('property');
pathTokens.push(property.node.name);
current = current.get('object');
}
}

if (!seenPaths.has(exprKey)) {
let loweredDep: Place;
if (dependency.isJSXIdentifier()) {
loweredDep = lowerValueToTemporary(builder, {
kind: 'LoadLocal',
place: lowerIdentifier(builder, dependency),
loc: path.node.loc ?? GeneratedSource,
});
} else if (dependency.isJSXMemberExpression()) {
loweredDep = lowerJsxMemberExpression(builder, dependency);
} else {
loweredDep = lowerExpressionToTemporary(builder, dependency);
}
capturedRefs.add(loweredDep);
seenPaths.add(exprKey);
if (binding !== undefined && pureScopes.has(binding.scope)) {
capturedIds.add(binding.identifier);
}
}

Expand Down Expand Up @@ -4305,13 +4175,13 @@ function gatherCapturedDeps(
return;
} else if (path.isJSXElement()) {
handleMaybeDependency(path.get('openingElement'));
} else if (path.isMemberExpression() || path.isIdentifier()) {
} else if (path.isIdentifier()) {
handleMaybeDependency(path);
}
},
});

return {identifiers: [...capturedIds.keys()], refs: [...capturedRefs]};
return [...capturedIds.keys()];
}

function notNull<T>(value: T | null): value is T {
Expand Down
37 changes: 1 addition & 36 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,7 @@ function collectHoistablePropertyLoadsImpl(
fn: HIRFunction,
context: CollectHoistablePropertyLoadsContext,
): ReadonlyMap<BlockId, BlockInfo> {
const functionExpressionLoads = collectFunctionExpressionFakeLoads(fn);
const actuallyEvaluatedTemporaries = new Map(
[...context.temporaries].filter(([id]) => !functionExpressionLoads.has(id)),
);

const nodes = collectNonNullsInBlocks(fn, {
...context,
temporaries: actuallyEvaluatedTemporaries,
});
const nodes = collectNonNullsInBlocks(fn, context);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can now be cleaned up 🎉

propagateNonNull(fn, nodes, context.registry);

if (DEBUG_PRINT) {
Expand Down Expand Up @@ -598,30 +590,3 @@ function reduceMaybeOptionalChains(
}
} while (changed);
}

function collectFunctionExpressionFakeLoads(
fn: HIRFunction,
): Set<IdentifierId> {
const sources = new Map<IdentifierId, IdentifierId>();
const functionExpressionReferences = new Set<IdentifierId>();

for (const [_, block] of fn.body.blocks) {
for (const {lvalue, value} of block.instructions) {
if (
value.kind === 'FunctionExpression' ||
value.kind === 'ObjectMethod'
) {
for (const reference of value.loweredFunc.dependencies) {
let curr: IdentifierId | undefined = reference.identifier.id;
while (curr != null) {
functionExpressionReferences.add(curr);
curr = sources.get(curr);
}
}
} else if (value.kind === 'PropertyLoad') {
sources.set(lvalue.identifier.id, value.object.identifier.id);
}
}
}
return functionExpressionReferences;
}
2 changes: 0 additions & 2 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,6 @@ const EnvironmentConfigSchema = z.object({
*/
enableUseTypeAnnotations: z.boolean().default(false),

enableFunctionDependencyRewrite: z.boolean().default(true),

/**
* Enables inference of optional dependency chains. Without this flag
* a property chain such as `props?.items?.foo` will infer as a dep on
Expand Down
1 change: 0 additions & 1 deletion compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,6 @@ export type ObjectProperty = {
};

export type LoweredFunction = {
dependencies: Array<Place>;
func: HIRFunction;
};

Expand Down
17 changes: 16 additions & 1 deletion compiler/packages/babel-plugin-react-compiler/src/HIR/MergeOverlappingReactiveScopesHIR.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,14 @@ function collectScopeInfo(fn: HIRFunction): ScopeInfo {
const scope = place.identifier.scope;
if (scope != null) {
placeScopes.set(place, scope);
/**
* Record both mutating and non-mutating scopes to merge scopes with
* still-mutating values with inner scopes that alias those values
* (see `nonmutating-capture-in-unsplittable-memo-block`)
*
* Note that this isn't perfect, as it also leads to merging of mutating
* scopes with JSX single-instruction scopes (see `mutation-within-jsx`)
*/
if (scope.range.start !== scope.range.end) {
getOrInsertDefault(scopeStarts, scope.range.start, new Set()).add(
scope,
Expand Down Expand Up @@ -254,7 +262,7 @@ function visitPlace(
* of the stack to the mutated outer scope.
*/
const placeScope = getPlaceScope(id, place);
if (placeScope != null && isMutable({id} as any, place)) {
if (placeScope != null && isMutable({id}, place)) {
const placeScopeIdx = activeScopes.indexOf(placeScope);
if (placeScopeIdx !== -1 && placeScopeIdx !== activeScopes.length - 1) {
joined.union([placeScope, ...activeScopes.slice(placeScopeIdx + 1)]);
Expand All @@ -275,6 +283,13 @@ function getOverlappingReactiveScopes(
for (const instr of block.instructions) {
visitInstructionId(instr.id, context, state);
for (const place of eachInstructionOperand(instr)) {
if (
(instr.value.kind === 'FunctionExpression' ||
instr.value.kind === 'ObjectMethod') &&
place.identifier.type.kind === 'Primitive'
) {
continue;
}
Comment on lines +286 to +292
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double-check this case a bit more? Every time we have to special case something like this there's a chance that we're overlooking the larger rule. Why do we need to skip primitives, and why only for functionexpr/objectmethod?

If nothing else, this repeatedly checks properties of the instruction value for every single operand, which is inefficient, but i'm more concerned with the check itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! This change special cases function calls that capture identifiers whose type is Primitive but have a scope for other reasons. I added this to reduce the number of files changes by this refactor.

We can remove this check (which leads to more merged scopes) or generalize it in visitPlace in a later PR.

visitPlace(instr.id, place, state);
}
for (const place of eachInstructionLValue(instr)) {
Expand Down
5 changes: 1 addition & 4 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,9 +538,6 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
.split('\n')
.map(line => ` ${line}`)
.join('\n');
const deps = instrValue.loweredFunc.dependencies
.map(dep => printPlace(dep))
.join(',');
const context = instrValue.loweredFunc.func.context
.map(dep => printPlace(dep))
.join(',');
Expand All @@ -557,7 +554,7 @@ export function printInstructionValue(instrValue: ReactiveValue): string {
})
.join(', ') ?? '';
const type = printType(instrValue.loweredFunc.func.returnType).trim();
value = `${kind} ${name} @deps[${deps}] @context[${context}] @effects[${effects}]${type !== '' ? ` return${type}` : ''}:\n${fn}`;
value = `${kind} ${name} @context[${context}] @effects[${effects}]${type !== '' ? ` return${type}` : ''}:\n${fn}`;
break;
}
case 'TaggedTemplateExpression': {
Expand Down
5 changes: 2 additions & 3 deletions compiler/packages/babel-plugin-react-compiler/src/HIR/PropagateScopeDependenciesHIR.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,9 +738,8 @@ function collectDependencies(
}
for (const instr of block.instructions) {
if (
fn.env.config.enableFunctionDependencyRewrite &&
(instr.value.kind === 'FunctionExpression' ||
instr.value.kind === 'ObjectMethod')
instr.value.kind === 'FunctionExpression' ||
instr.value.kind === 'ObjectMethod'
) {
context.declare(instr.lvalue.identifier, {
id: instr.id,
Expand Down
Loading
Loading