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: Improve merging of memo scopes that invalidate together #29156

Merged
merged 7 commits into from
May 23, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,8 @@ export type ObjectShape = {
*/
export type ShapeRegistry = Map<string, ObjectShape>;
export const BuiltInArrayId = "BuiltInArray";
export const BuiltInFunctionId = "BuiltInFunction";
export const BuiltInJsxId = "BuiltInJsx";
export const BuiltInObjectId = "BuiltInObject";
export const BuiltInUseStateId = "BuiltInUseState";
export const BuiltInSetStateId = "BuiltInSetState";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,21 @@ import {
ReactiveScopeDependencies,
ReactiveScopeDependency,
ReactiveStatement,
Type,
makeInstructionId,
} from "../HIR";
import {
BuiltInArrayId,
BuiltInFunctionId,
BuiltInJsxId,
BuiltInObjectId,
} from "../HIR/ObjectShape";
import { eachInstructionLValue } from "../HIR/visitors";
import { assertExhaustive } from "../Utils/utils";
import { printReactiveScopeSummary } from "./PrintReactiveFunction";
import {
printReactiveFunction,
printReactiveScopeSummary,
} from "./PrintReactiveFunction";
import {
ReactiveFunctionTransform,
ReactiveFunctionVisitor,
Expand Down Expand Up @@ -78,6 +88,8 @@ import {
export function mergeReactiveScopesThatInvalidateTogether(
fn: ReactiveFunction
): void {
log("MergeReactiveScopesThatInvalidateTogether");
log(printReactiveFunction(fn));
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense if we want to keep this for debugging.

Maybe we should consider passing in a lambda that wraps printReactiveFunction or running a rollup transform that strips log, logger.logDiagnostic, etc argument expressions so that prod build systems don't need to call these functions.

Doesn't have to be this PR, just wanted to record for the future..

const lastUsageVisitor = new FindLastUsageVisitor();
visitReactiveFunction(fn, lastUsageVisitor, undefined);
visitReactiveFunction(fn, new Transform(lastUsageVisitor.lastUsage), null);
Expand Down Expand Up @@ -430,7 +442,13 @@ function canMergeScopes(
}))
),
next.scope.dependencies
)
) ||
(next.scope.dependencies.size !== 0 &&
[...next.scope.dependencies].every(
(dep) =>
current.scope.declarations.has(dep.identifier.id) &&
isAlwaysInvalidatingType(dep.identifier.type)
))
) {
log(` outputs of prev are input to current`);
return true;
Expand All @@ -441,6 +459,20 @@ function canMergeScopes(
return false;
}

function isAlwaysInvalidatingType(type: Type): boolean {
if (type.kind === "Object") {
switch (type.shapeId) {
case BuiltInArrayId:
case BuiltInObjectId:
case BuiltInFunctionId:
case BuiltInJsxId: {
return true;
}
}
}
return false;
}

function areEqualDependencies(
a: Set<ReactiveScopeDependency>,
b: Set<ReactiveScopeDependency>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import {
} from "../HIR/HIR";
import {
BuiltInArrayId,
BuiltInFunctionId,
BuiltInJsxId,
BuiltInObjectId,
BuiltInUseRefId,
} from "../HIR/ObjectShape";
Expand Down Expand Up @@ -313,6 +315,7 @@ function* generateInstructionTypes(

case "FunctionExpression": {
yield* generate(value.loweredFunc.func);
yield equation(left, { kind: "Object", shapeId: BuiltInFunctionId });
break;
}

Expand All @@ -327,10 +330,13 @@ function* generateInstructionTypes(
break;
}

case "JsxExpression":
case "JsxFragment": {
yield equation(left, { kind: "Object", shapeId: BuiltInJsxId });
break;
}
case "DeclareLocal":
case "NewExpression":
case "JsxExpression":
case "JsxFragment":
case "RegExpLiteral":
case "PropertyStore":
case "ComputedStore":
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ function Component(props) {
7 | return <Foo item={item} current={current} />;
8 | };
> 9 | return <Items>{props.items.map((item) => renderItem(item))}</Items>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $64[13:15] (9:9)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $64[13:15]:TObject<BuiltInFunction> (9:9)
10 | }
11 |
```
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ function Component(props) {
6 | return <Foo item={item} current={current} />;
7 | };
> 8 | return <Items>{props.items.map((item) => renderItem(item))}</Items>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $60[14:16] (8:8)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef). Cannot access ref value at mutate? $60[14:16]:TObject<BuiltInFunction> (8:8)
9 | }
10 |
```
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,44 +33,23 @@ import { c as _c } from "react/compiler-runtime";
import { useState } from "react";

function Component() {
const $ = _c(8);
const $ = _c(2);
const [count, setCount] = useState(0);
let t0;
let t1;
if ($[0] !== count) {
t0 = <button onClick={() => setCount(count - 1)}>Decrement</button>;
t0 = (
<div>
<button onClick={() => setCount(count - 1)}>Decrement</button>

t1 = () => setCount(count + 1);
<button onClick={() => setCount(count + 1)}>Increment</button>
</div>
);
$[0] = count;
$[1] = t0;
$[2] = t1;
} else {
t0 = $[1];
t1 = $[2];
}
let t2;
if ($[3] !== t1) {
t2 = <button onClick={t1}>Increment</button>;
$[3] = t1;
$[4] = t2;
} else {
t2 = $[4];
}
let t3;
if ($[5] !== t0 || $[6] !== t2) {
t3 = (
<div>
{t0}
{t2}
</div>
);
$[5] = t0;
$[6] = t2;
$[7] = t3;
} else {
t3 = $[7];
}
return t3;
return t0;
}

export const FIXTURE_ENTRYPOINT = {
Expand Down