Skip to content

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

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

Closed
mofeiZ wants to merge 10 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7ac7b52
[compiler][ez] Patch hoistability for ObjectMethods
mofeiZ Nov 5, 2024
e288dea
[compiler] bugfix for hoistable deps for nested functions
mofeiZ Nov 5, 2024
13752bf
[compiler] patch: rewrite scope dep/decl in inlineJsxTransform
mofeiZ Nov 5, 2024
de4b0a8
[compiler] Delete propagateScopeDeps (non-hir)
mofeiZ Nov 5, 2024
00a53cc
[compiler] Collect temporaries and optional chains from inner functions
mofeiZ Nov 5, 2024
2b51f3b
[compiler] Stop using function `dependencies` in propagateScopeDeps
mofeiZ Nov 5, 2024
640a785
[compiler] Lower JSXMemberExpression with LoadLocal
mofeiZ Nov 5, 2024
98b0d00
[compiler][be] Patch test fixtures for evaluator
mofeiZ Nov 5, 2024
cdf5e83
[compiler][be] Clean up nested function context in DCE
mofeiZ Nov 5, 2024
cd06aee
[compiler] Delete LoweredFunction.dependencies and hoisted instructions
mofeiZ Nov 5, 2024
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
24 changes: 6 additions & 18 deletions compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Pipeline.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ import {
mergeReactiveScopesThatInvalidateTogether,
promoteUsedTemporaries,
propagateEarlyReturns,
propagateScopeDependencies,
pruneHoistedContexts,
pruneNonEscapingScopes,
pruneNonReactiveDependencies,
Expand Down Expand Up @@ -348,14 +347,12 @@ function* runWithEnvironment(
});
assertTerminalSuccessorsExist(hir);
assertTerminalPredsExist(hir);
if (env.config.enablePropagateDepsInHIR) {
propagateScopeDependenciesHIR(hir);
yield log({
kind: 'hir',
name: 'PropagateScopeDependenciesHIR',
value: hir,
});
}
propagateScopeDependenciesHIR(hir);
yield log({
kind: 'hir',
name: 'PropagateScopeDependenciesHIR',
value: hir,
});

if (env.config.inlineJsxTransform) {
inlineJsxTransform(hir, env.config.inlineJsxTransform);
Expand Down Expand Up @@ -383,15 +380,6 @@ function* runWithEnvironment(
});
assertScopeInstructionsWithinScopes(reactiveFunction);

if (!env.config.enablePropagateDepsInHIR) {
propagateScopeDependencies(reactiveFunction);
yield log({
kind: 'reactive',
name: 'PropagateScopeDependencies',
value: reactiveFunction,
});
}

pruneNonEscapingScopes(reactiveFunction);
yield log({
kind: 'reactive',
Expand Down
160 changes: 18 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 @@ -3186,7 +3185,13 @@ function lowerJsxMemberExpression(
loc: object.node.loc ?? null,
suggestions: null,
});
objectPlace = lowerIdentifier(builder, object);

const kind = getLoadKind(builder, object);
objectPlace = lowerValueToTemporary(builder, {
kind: kind,
place: lowerIdentifier(builder, object),
loc: exprPath.node.loc ?? GeneratedSource,
});
}
const property = exprPath.get('property').node.name;
return lowerValueToTemporary(builder, {
Expand Down Expand Up @@ -3359,7 +3364,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 @@ -3373,7 +3378,7 @@ function lowerFunction(
expr,
builder.environment,
builder.bindings,
[...builder.context, ...captured.identifiers],
[...builder.context, ...capturedContext],
builder.parentFunction,
);
let loweredFunc: HIRFunction;
Expand All @@ -3386,7 +3391,6 @@ function lowerFunction(
loweredFunc = lowering.unwrap();
return {
func: loweredFunc,
dependencies: captured.refs,
};
}

Expand Down Expand Up @@ -4060,14 +4064,6 @@ function lowerAssignment(
}
}

function isValidDependency(path: NodePath<t.MemberExpression>): boolean {
Copy link
Copy Markdown
Contributor Author

@mofeiZ mofeiZ Oct 26, 2024

Choose a reason for hiding this comment

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

Not needed as we don't calculate LoweredFunction.dependencies anymore

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 @@ -4082,19 +4078,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>();
Copy link
Copy Markdown
Contributor Author

@mofeiZ mofeiZ Oct 26, 2024

Choose a reason for hiding this comment

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

We now only need a set of seen context variables. Previously capturedIds: Map<t.Identifier, number> and other collections were used to find and dedupe potential dependency paths.


/*
* Capture all the scopes from the parent of this function up to and including
Expand All @@ -4105,33 +4098,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>,
Copy link
Copy Markdown
Contributor Author

@mofeiZ mofeiZ Oct 26, 2024

Choose a reason for hiding this comment

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

This becomes much simpler as we just need to extract potential context variables (i.e. ones defined in an outer scope of the current function)

): 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 @@ -4147,115 +4118,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 @@ -4286,13 +4162,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
Loading