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.

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] Empty dep arrays for globals/module-scoped values/imports #31666

Merged
merged 2 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export function inferEffectDependencies(fn: HIRFunction): void {
{pruned: boolean; deps: ReactiveScopeDependencies; hasSingleInstr: boolean}
>();

const loadGlobals = new Set<IdentifierId>();

/**
* When inserting LoadLocals, we need to retain the reactivity of the base
* identifier, as later passes e.g. PruneNonReactiveDeps take the reactivity of
Expand Down Expand Up @@ -87,26 +89,23 @@ export function inferEffectDependencies(fn: HIRFunction): void {
lvalue.identifier.id,
instr as TInstruction<FunctionExpression>,
);
} else if (
value.kind === 'LoadGlobal' &&
value.binding.kind === 'ImportSpecifier'
) {
const moduleTargets = autodepFnConfigs.get(value.binding.module);
if (moduleTargets != null) {
const numRequiredArgs = moduleTargets.get(value.binding.imported);
if (numRequiredArgs != null) {
autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs);
}
}
} else if (
value.kind === 'LoadGlobal' &&
value.binding.kind === 'ImportDefault'
) {
const moduleTargets = autodepFnConfigs.get(value.binding.module);
if (moduleTargets != null) {
const numRequiredArgs = moduleTargets.get(DEFAULT_EXPORT);
if (numRequiredArgs != null) {
autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs);
} else if (value.kind === 'LoadGlobal') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consolidated all of the loadglobal cases here and merged the autodepFnConfig lookups

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Could we also update the PR title and description (e.g. no deps array for all global / module / imports, not just outlined fns)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep!

loadGlobals.add(lvalue.identifier.id);

if (
value.binding.kind === 'ImportSpecifier' ||
value.binding.kind === 'ImportDefault'
) {
const moduleTargets = autodepFnConfigs.get(value.binding.module);
if (moduleTargets != null) {
const importSpecifierName =
value.binding.kind === 'ImportSpecifier'
? value.binding.imported
: DEFAULT_EXPORT;
const numRequiredArgs = moduleTargets.get(importSpecifierName);
if (numRequiredArgs != null) {
autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs);
}
}
}
} else if (
Expand All @@ -117,8 +116,19 @@ export function inferEffectDependencies(fn: HIRFunction): void {
autodepFnLoads.get(value.callee.identifier.id) === value.args.length &&
value.args[0].kind === 'Identifier'
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we check value.args.length or moduleTargets in a followup? Either bailing out or skipping seems reasonable, but this looks like it might crash compilation on useInferEffect() calls with no args.

(I might be wrong though, could have missed a Environment validation fn!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add an environment validation for it. That number should always be greater than 0, otherwise there is no lambda to analyze

) {
const effectDeps: Array<Place> = [];
const newInstructions: Array<Instruction> = [];
const deps: ArrayExpression = {
kind: 'ArrayExpression',
elements: effectDeps,
loc: GeneratedSource,
};
const depsPlace = createTemporaryPlace(fn.env, GeneratedSource);
depsPlace.effect = Effect.Read;

const fnExpr = fnExpressions.get(value.args[0].identifier.id);
if (fnExpr != null) {
// We have a function expression, so we can infer its dependencies
const scopeInfo =
fnExpr.lvalue.identifier.scope != null
? scopeInfos.get(fnExpr.lvalue.identifier.scope.id)
Expand All @@ -140,14 +150,12 @@ export function inferEffectDependencies(fn: HIRFunction): void {
}

/**
* Step 1: write new instructions to insert a dependency array
* Step 1: push dependencies to the effect deps array
*
* Note that it's invalid to prune non-reactive deps in this pass, see
* the `infer-effect-deps/pruned-nonreactive-obj` fixture for an
* explanation.
*/
const effectDeps: Array<Place> = [];
const newInstructions: Array<Instruction> = [];
for (const dep of scopeInfo.deps) {
const {place, instructions} = writeDependencyToInstructions(
dep,
Expand All @@ -158,14 +166,6 @@ export function inferEffectDependencies(fn: HIRFunction): void {
newInstructions.push(...instructions);
effectDeps.push(place);
}
const deps: ArrayExpression = {
kind: 'ArrayExpression',
elements: effectDeps,
loc: GeneratedSource,
};

const depsPlace = createTemporaryPlace(fn.env, GeneratedSource);
depsPlace.effect = Effect.Read;

newInstructions.push({
id: makeInstructionId(0),
Expand All @@ -177,6 +177,16 @@ export function inferEffectDependencies(fn: HIRFunction): void {
// Step 2: push the inferred deps array as an argument of the useEffect
value.args.push({...depsPlace, effect: Effect.Freeze});
rewriteInstrs.set(instr.id, newInstructions);
} else if (loadGlobals.has(value.args[0].identifier.id)) {
// Global functions have no reactive dependencies, so we can insert an empty array
newInstructions.push({
id: makeInstructionId(0),
loc: GeneratedSource,
lvalue: {...depsPlace, effect: Effect.Mutate},
value: deps,
});
value.args.push({...depsPlace, effect: Effect.Freeze});
rewriteInstrs.set(instr.id, newInstructions);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import { print } from "shared-runtime";
* before OutlineFunctions
*/
function OutlinedFunctionInEffect() {
useEffect(_temp);
useEffect(_temp, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉!!

}
function _temp() {
return print("hello world!");
Expand Down
Loading