[Compiler Bug]: Performance - `useEffect` without dependencies should be left alone

xsduan commented 1 month ago

What kind of issue is this?

[X] 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/#N4Igzg9grgTgxgUxALhAMygOzgFwJYSYAEUYCAwgBYCGmA5gmAKIBuCMAngCp4C2CACgCURYAB1iROITA4iAbRbUANgBoiZHADUVAXSIBeEmQDKOajkHDDAPiIBZC5QB0MWgBMIvYUIDcEiSJjBCY0NARcAWsDO3FJIM0dZQFHHBc3TE9vIT9AogBfXMw8mAQcWGIlZX9MfIDMDGx8QmCAdTcABySoBABBMAAlBDRrOKDpTFkiKsNgqloGZjZOHn5hGvGZOVK0WdIEIZGqorz90PDI6Ni8oJ3nOFhSzDkjKo2Ck8lS8phiHZq6pgQPkgA

Repro steps

I was playing around with a small repo I had and noticed something similar to #30782. (Essentially, I was tracking a variable so that the callback identity could be stable to avoid rapid-fire rerenders of the whole page)

Wrapping it in a useEffect works and probably more correct, but it generates redundant memoization:

function useWrapValueAsRef() {
  const $ = _c(2);
  const val = useChangesEveryTime();
  const ref = useRef(val);
  let t0;
  if ($[0] !== val) {
    t0 = () => {
      ref.current = val;
    };
    $[0] = val;
    $[1] = t0;
  } else {
    t0 = $[1];
  }
  useEffect(t0);
  return ref;
}

where I would expect it to just leave everything alone.

Ignoring the contrived source of the variable (in the real app I'm testing with, it probably changes a couple times on page load), it does the check every time, which is redundant because the useEffect will always run the latest closure anyways. I'm not sure if this is because of some sort of optimization in useEffect but I don't see why that would be the case.

I don't imagine this to be a huge issue, but it's technically less performant as the code size is bigger and it uses 2 array slots.

How often does this bug happen?

Every time

What version of React are you using?

React Playground (19-RC)

josephsavona commented 1 month ago

Hmm, your PR description and the playground link don't quite match up (or at least, how I understand/interpret them), so let me check.

In the playground, you have a hook that reads an impure value (Math.random), and you use a ref to try to prevent that value from changing again. But then you also have a useEffect with no dependencies, which will update the ref to reflect the new value of that impure source (in this case, calling Math.random again).

But by using refs, React doesn't know that the value is changing, and can't re-render accordingly. Instead, it seems like you would generally want one of two behaviors:

The external data source/value (that you're simulating with Math.random) can change over time and you want this to be reflected in the UI: this should use useSyncExtenralStore, or manually manage the value with useState and a subscription in useEffect.
The external data source can change over time, but you don't want to observe changes, you want it to stay the same value. In this case, using a ref works, but you don't need any useEffects to update the value again.

Can you clarify which use-case it is?

xsduan commented 1 month ago

Not exactly the same as the second one but similar, something like this hook. I think that describes like 90% of the times I use this pattern. I'm aware that it's technically not the best way to do it but we've run into several performance issues that were addressed with that hook.

E: I guess my bigger point is that in this case, if I'm using this pattern correctly, it's not worth it to check since callback identity has never been a problem with Effects, and if it's technically invalid, it's probably best to bail anyways.

josephsavona commented 1 month ago

Gotcha. That pattern is questionable since there’s no guarantee the effect will run before the callback is called, but it can work in some cases.

In terms of the memoization, the idea is that when the effect deps don’t change, we don’t have to reallocate a closure unnecessarily.

xsduan commented 1 month ago

I do wonder if it's a bit of a premature optimization in this case. Because a closure might be a few bytes of memory every render but the additional code size would probably also be an issue.

With memos the tradeoff in code size is more obvious due to potentially skipping large chunks of render but with effects we are getting into V8 territory here since the only observable difference is stuff like cache locality, G1 pressure, etc etc. I think in the cases where this does make a lot of difference the developer could create an indirection, so the compiler probably shouldn't propagate the bail out, but the default pattern of use[Layout]Effect(() => { ... }, []?) could be ignored.

Might be missing something about the internals though, which is fine.

facebook / react