-
Notifications
You must be signed in to change notification settings - Fork 45.6k
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
Comments
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? |
I see, interesting.
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 |
The compiler preserves existing memoization except where we can prove that the value being memoized is a primitive. |
I'm coming from #29580 Whether an object is cheap to compare or not is not a sufficient for deciding to skip memoization.
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 const foo() => `/foo/${1}`;
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. |
What kind of issue is this?
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 settingoutput
to beindex
itself instead, it gets memoized properly.Code:
Edit:
Additionally, if
index
was manually memoized withuseMemo
, 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
The text was updated successfully, but these errors were encountered: