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] Remove redundant InferMutableContextVariables #32097

Merged
merged 1 commit into from
Feb 18, 2025
Merged

[compiler] Remove redundant InferMutableContextVariables #32097

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
27 changes: 4 additions & 23 deletions compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,35 +10,16 @@ import {
Effect,
HIRFunction,
Identifier,
IdentifierId,
LoweredFunction,
isRefOrRefValue,
makeInstructionId,
} from '../HIR';
import {deadCodeElimination} from '../Optimization';
import {inferReactiveScopeVariables} from '../ReactiveScopes';
import {rewriteInstructionKindsBasedOnReassignment} from '../SSA';
import {inferMutableContextVariables} from './InferMutableContextVariables';
import {inferMutableRanges} from './InferMutableRanges';
import inferReferenceEffects from './InferReferenceEffects';

// Helper class to track indirections such as LoadLocal and PropertyLoad.
export class IdentifierState {
properties: Map<IdentifierId, Identifier> = new Map();

resolve(identifier: Identifier): Identifier {
const resolved = this.properties.get(identifier.id);
if (resolved !== undefined) {
return resolved;
}
return identifier;
}

alias(lvalue: Identifier, value: Identifier): void {
this.properties.set(lvalue.id, this.properties.get(value.id) ?? value);
}
}

export default function analyseFunctions(func: HIRFunction): void {
for (const [_, block] of func.body.blocks) {
for (const instr of block.instructions) {
Expand Down Expand Up @@ -78,7 +59,6 @@ function lower(func: HIRFunction): void {
}

function infer(loweredFunc: LoweredFunction): void {
const knownMutated = inferMutableContextVariables(loweredFunc.func);
for (const operand of loweredFunc.func.context) {
const identifier = operand.identifier;
CompilerError.invariant(operand.effect === Effect.Unknown, {
Expand All @@ -95,10 +75,11 @@ function infer(loweredFunc: LoweredFunction): void {
* render
*/
operand.effect = Effect.Capture;
} else if (knownMutated.has(operand)) {
operand.effect = Effect.Mutate;
} else if (isMutatedOrReassigned(identifier)) {
// Note that this also reflects if identifier is ConditionallyMutated
/**
* Reflects direct reassignments, PropertyStores, and ConditionallyMutate
* (directly or through maybe-aliases)
*/
operand.effect = Effect.Capture;
} else {
operand.effect = Effect.Read;
Expand Down
105 changes: 0 additions & 105 deletions compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutableContextVariables.ts
View file
Open in desktop

This file was deleted.

66 changes: 32 additions & 34 deletions ...rc/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@ function Component() {

// capture into a separate variable that is not a context variable.
const y = x;
/**
* Note that this fixture currently produces a stale effect closure if `y = x
* = someGlobal` changes between renders. Under current compiler assumptions,
* that would be a rule of react violation.
*/
useEffect(() => {
y.value = 'hello';
}, []);
});

useEffect(() => {
setState(someGlobal.value);
Expand All @@ -46,57 +51,50 @@ import { useEffect, useState } from "react";
let someGlobal = { value: null };

function Component() {
const $ = _c(7);
const $ = _c(5);
const [state, setState] = useState(someGlobal);

let x = someGlobal;
while (x == null) {
x = someGlobal;
}

const y = x;
let t0;
let t1;
let t2;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
let x = someGlobal;
while (x == null) {
x = someGlobal;
}

const y = x;
t0 = useEffect;
t1 = () => {
t0 = () => {
y.value = "hello";
};
t2 = [];
$[0] = t0;
} else {
t0 = $[0];
}
useEffect(t0);
let t1;
let t2;
if ($[1] === Symbol.for("react.memo_cache_sentinel")) {
t1 = () => {
setState(someGlobal.value);
};
t2 = [someGlobal];
$[1] = t1;
$[2] = t2;
} else {
t0 = $[0];
t1 = $[1];
t2 = $[2];
}
t0(t1, t2);
let t3;
useEffect(t1, t2);

const t3 = String(state);
let t4;
if ($[3] === Symbol.for("react.memo_cache_sentinel")) {
t3 = () => {
setState(someGlobal.value);
};
t4 = [someGlobal];
if ($[3] !== t3) {
t4 = <div>{t3}</div>;
$[3] = t3;
$[4] = t4;
} else {
t3 = $[3];
t4 = $[4];
}
useEffect(t3, t4);

const t5 = String(state);
let t6;
if ($[5] !== t5) {
t6 = <div>{t5}</div>;
$[5] = t5;
$[6] = t6;
} else {
t6 = $[6];
}
return t6;
return t4;
}

export const FIXTURE_ENTRYPOINT = {
Expand Down
7 changes: 6 additions & 1 deletion ...-react-compiler/src/__tests__/fixtures/compiler/allow-mutate-global-in-effect-fixpoint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,14 @@ function Component() {

// capture into a separate variable that is not a context variable.
const y = x;
/**
* Note that this fixture currently produces a stale effect closure if `y = x
* = someGlobal` changes between renders. Under current compiler assumptions,
* that would be a rule of react violation.
*/
useEffect(() => {
y.value = 'hello';
}, []);
});

useEffect(() => {
setState(someGlobal.value);
Expand Down
23 changes: 12 additions & 11 deletions ...mpiler/src/__tests__/fixtures/compiler/reanimated-shared-value-writes.expect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,22 @@ import { useSharedValue } from "react-native-reanimated";
* of render
*/
function SomeComponent() {
const $ = _c(3);
const $ = _c(2);
const sharedVal = useSharedValue(0);

const T0 = Button;
const t0 = () => (sharedVal.value = Math.random());
let t1;
if ($[0] !== T0 || $[1] !== t0) {
t1 = <T0 onPress={t0} title="Randomize" />;
$[0] = T0;
let t0;
if ($[0] !== sharedVal) {
t0 = (
<Button
onPress={() => (sharedVal.value = Math.random())}
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like the check wasn't completely redundant since we are getting different output. why does this output change exactly?

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 is a similar case to the other test fixture (which also had changed output). From my opinion, we should be able to replace onPress={() => (sharedVal.value = Math.random())} with onPress={() => mutate(sharedVal)}, without meaningful changes to mutability inference.

This isn't the case today. InferMutableContextVariables annotates sharedVal has having a (known) "write", only because it sees a PropertyStore instruction. Without this, we infer sharedVal as only being captured by the function. This distinction doesn't really make sense with enableTransitivelyFreezeFunctionExpressions mode

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh makes sense. Cool

title="Randomize"
/>
);
$[0] = sharedVal;
$[1] = t0;
$[2] = t1;
} else {
t1 = $[2];
t0 = $[1];
}
return t1;
return t0;
}

```
Expand Down
Loading