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 Bug]: Values used as indexes are not memoized #29172

Open
1 of 4 tasks
guillaumebrunerie opened this issue May 20, 2024 · 4 comments
Open
1 of 4 tasks

[Compiler Bug]: Values used as indexes are not memoized #29172

guillaumebrunerie opened this issue May 20, 2024 · 4 comments
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug

Comments

@guillaumebrunerie
Copy link

guillaumebrunerie commented May 20, 2024

What kind of issue is this?

  • React Compiler core (the JS output is incorrect, or your app works incorrectly after optimization)
  • babel-plugin-react-compiler (build issue installing or using the Babel plugin)
  • eslint-plugin-react-compiler (build issue installing or using the eslint plugin)
  • react-compiler-healthcheck (build issue installing or using the healthcheck script)

Link to repro

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAMygOzgFwJYSYAEAwhALYAOhCmOAFMEQG4CGANlAkQL4CURYAB1iROITA4ieTABMEADyIBeIosq0weZggBiWXAUz02nBHxFEi7BFOg5KUKatNcwAbRnyFAXQDclkQA9EH2js7ScooBojC2sMQAPLLaAHzAYU48iUEpzKkxPDEgPEA

Repro steps

In the code below, the call to expensiveFunction(value) does not appear to be memoized (it is preserved as is at the top level in the JS output).

This seems to be due to it being used as an index in let output = values[index], because when setting output to be index itself instead, it gets memoized properly.

Code:

function Component({ value }) {
  const index = expensiveFunction(value)
  let output = values[index];
  //output = index;
  return <div>{output}</div>;
};

Edit:
Additionally, if index was manually memoized with useMemo, the compiler will remove the manual memoization, resulting in potentially much slower code than the original code.

How often does this bug happen?

Every time

What version of React are you using?

19

@guillaumebrunerie guillaumebrunerie added Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug labels May 20, 2024
@josephsavona
Copy link
Contributor

Thanks for posting. What’s happening here is that the compiler infers that expensiveFunction() returns a primitive value, which can be cheaply compared for changes. The compiler tries hard to only memorize what is strictly necessary to avoid cascading updates (parent re-renders, child re-renders, etc) and avoid memoization overhead in other cases.

In your case, is the function actually expensive enough to be a problem, or were you just surprised that the call didn’t get memoized?

@guillaumebrunerie
Copy link
Author

I see, interesting.

In your case, is the function actually expensive enough to be a problem, or were you just surprised that the call didn’t get memoized?

I'm mostly surprised that it didn't get memoized. In the part of my code where I found this behavior, it isn't actually expensive to compute.

But I still find this behavior quite surprising. Just because a function returns a primitive value doesn't mean it is cheap to run, for instance we could have something like:

const expensiveFunction = (value) => {
  return someLargeArray.findIndex(v => JSON.stringify(v) === JSON.stringify(value));
};

which feels pretty reasonable to write but should definitely be memoized. But maybe it happens rarely in practice.

What I find the most surprising is that an explicit useMemo gets removed. I thought the compiler is designed to try to preserve existing memoization, or did I misunderstand something?

@josephsavona
Copy link
Contributor

The compiler preserves existing memoization except where we can prove that the value being memoized is a primitive.

@kaaboaye
Copy link

kaaboaye commented May 25, 2024

I'm coming from #29580

Whether an object is cheap to compare or not is not a sufficient for deciding to skip memoization.

  • Are two Numbers always cheap to compare? Yes.
  • Are two Numbers always cheap to compute? No.

One of those numbers can be obtained via some very computationally expensive algorithm. If an app using correctly using useMemo to deduplicate those computations moves to React Compiler it may experience significant slowdown.

It is even worse for string template literals because if my understanding is correct each time we call foo() at least a couple allocations will be performed.

const foo() => `/foo/${1}`;
  1. Allocate '/foo/' string
  2. Call String(1) which allocates '1' string.
  3. Concatenate '/foo/' and '1' which is a third allocation.

So in my understanding of how string templates work they will always be more expensive to compute then dereference from cache them and check whether the reference has changed.

In conclusion. Unless you are absolutely sure that given value is a constant requiring no additional allocations and expensive computations it should always be memoized. Especially since react compiler strips explicit memoization.

Additionally I'm not sure whether it makes sense to distinguish between primitives and objects. Javascript compares values only by references. {a: 1} === {a: 1} evaluates to false but 'very long string' === 'very long string' evaluates to true even though the second one can be much more expensive to compare due to potentially requiring many dereferences. But I think it is a topic for another discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug
Projects
None yet
Development

No branches or pull requests

3 participants