facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.46k stars 46.75k forks source link

Effect memoization and immutable data structures #15154

Open roman01la opened 5 years ago

roman01la commented 5 years ago

Current design of useEffect requires dependencies to be either primitive values or references to the same object, because shallow equality check relies on Object.is which is an identity check for objects.

The above means that there's no way to perform structural comparison, which is needed for immutable data structures when identity check fails.

To maintain backwards compatibility a comparator function could be provided as the third argument to useEffect:

useEffect(fn, deps, depsComparator);

The goal here is to preserve an ease of use of the API with immutable data structures in order to provide an idiomatic usage of useEffect in ClojureScript and other environments that rely on immutability e.g. Immutable.js

cc @mhuebert @Lokeh @orestis

gaearon commented 5 years ago

Do they have some kind of a hash?

gaearon commented 5 years ago

@sebmarkbage

roman01la commented 5 years ago

Yes, they do, but ClojureScript doesn't rely on hash equality. It's reliable only as a fast fail check, but doesn't guarantee that two objects with the same hash are structurally equal.

gaearon commented 5 years ago

How deep can the structural comparison go?

roman01la commented 5 years ago

It can be full traversal when comparing literal values, but in practice a new value is often a slightly modified version. Because cljs utilizes structural sharing and comparison performs identity check on every level the operation short-circuits early.

sebmarkbage commented 5 years ago

It is not recommended to go too deep. Eg as far as deep traversal. This would be a bad default. If a data structure wanted to at most compare four levels deep, it could deconstruct eg a trie to the fourth level and pass that level to useEffect. Eg a wrapper around useEffect.

roman01la commented 5 years ago

If I understand this correctly, you are suggesting to hook into equivalence procedure (or implement a separate one) so it can be stopped at arbitrary level in data structure and pass the rest of it as deps into useEffect?

If that's correct, I don't think this is practically more useful than hacking around useEffect by tracking deps in a ref, though it seem to be unsafe operation in concurrent rendering mode.

Clojure's equality semantics are used in every ClojureScript wrapper for React, this was never a problem.

On the other hand there's a question about if it's common enough that compound objects are being used as deps in useEffect?

btw, I think the issue touches useMemo as well.

orestis commented 5 years ago

The issue with using "equality equivalence" values for fast comparison is that you still pay the price of constructing those values.

I think it also touches useState and the fact that a render is skipped if the new state is "equal" to the previous one.

From a performance point of view, the question is -- would an equality comparison be more an expensive an unnecessary re-render? In the case of Object.is, equality is faster than a render so it makes sense to rely on that.

I understand that exposing a custom equality algorithm might expose footguns when used wrongly. At the same time, the immutable data structure libraries (ClojureScript, Immutable.js) do have a very fast and optimised value equality comparison algorithm.

As it stands, relying on Object.is means that React will do more unnecessary re-renders, re-effects, and also fail to memoize pure functional components that expect immutable data structure props, so even that avenue is closed.

Thank you for taking the time to discuss this, and also thank you for coming up and documenting the Hooks API so well. It's a game changer!

mhuebert commented 5 years ago

and also fail to memoize pure functional components that expect immutable data structure props

React.Memo does accept a second areEqual argument, so that part is already compatible with cljs/immutable-js.

orestis commented 5 years ago

React.Memo does accept a second areEqual argument, so that part is already compatible with cljs/immutable-js.

Ah that's right -- I was thinking of useMemo.

lilactown commented 5 years ago

We greatly appreciate the React team’s time discussing this with us. The way I understand it, equality with ClojureScript’s immutable data basically works like:

  1. Are these two values referentially equal?
  2. Recurse

Since it uses structural sharing, this can have differing performance depending on whether the two values are derived from a common ancestor or not. The worst case is that they are completely distinct-but-equal structures, in which case it does a depth-first traversal of the whole thing.

My general take at this point is that adding a comparator to useEffect is not the best way, since it will be running this comparator every render. I think that we should instead wrap our heads around the cases where we would be passing in different references with equivalent structures, and then see how we can optimize those.

I can think of three cases right now where I see reference equality causing undesired behavior:

  1. When a user passes in a data literal to useState / useEffect deps / etc., e.g. (useEffect fx [{:data “foo”}])

  2. When some transformation returns a structurally equivalent value from some mutation, e.g. dispatching a state change

  3. Obtaining a data value from some external source, e.g. parsing JSON from a request

I think that (1) is something that we just have to discourage, or at least educate on the tradeoffs. That is the pathological case where even equality with structural sharing will have to do a deep traversal of the entire structure to be sure every render, which could be quite expensive.

With (2), we can take advantage of the fact that state changes are often derived, and also that we can do the equality check on update rather than on every render. Currently I’m thinking of exposing this as a general state hook to help with this:

function useImmutableState(initial) {
  let [v, u] = useState(initial);

  function updater(x) {
    if (!isFunction(x)) {
      // if x is not a fn, then it’s likely not derived from previous state
      // so we don’t bother checking equality
      return u(x);
    }

    // If it is a function, new state will be derived from previous
    // so we can take advantage of structural sharing to do fast
    // equality here
    function update(currentState) {
      let newState = x(currentState);
      if (immutableEqual(currentState, newState)) {
        return currentState; // should bail
      } else {
        return newState;
      }
    }
    return u(update);
  }
  return [v, updater];
}

I think that this takes care of the case where a user somehow applies a transformation to state that ends up with a structurally similar value.

For the case of doing a network request and parsing JSON to EDN or whatever (3), I’m not sure yet what the best way to handle that is; it’s going to be a deep equality no matter what (just like passing in a data literal), but at least it’s not being done each render. Perhaps a custom hook using a ref to do a comparison would guarantee equality, and be worth the cost in performance?

// If the value passed in is structurally equal to the one saved in the ref,
// it will return the one saved in the ref to preserve reference equality
function useImmutableValue(value) {
  let v = useRef(value);
  if (!immutableEquals(value, v.current)) {
    v.current = value;
  }
  return v.current;
}

Which could be used something like:

function MyComponent(props) {
  let result = useImmutableValue(useDataFromAPI(props.params));
  useEffect(function doThingBasedOnResult () { ... }, [result]);
  ...
}

In general, I think we’re looking for help / guidance in how best to leverage our immutable data structures with the latest React changes. It’d be nice to know if we’re thinking about this correctly, or if we should take a different tack.

mhuebert commented 5 years ago

When a user passes in a data literal to useState / useEffect deps / etc., e.g. (useEffect fx [{:data “foo”}])

I think typed-in literals are too small to worry about. From #14476 -

In those rare cases it's acceptable to pass [JSON.stringify(variables)] as a dependency. “Wait!” I hear you saying, “Isn’t JSON.stringify() slow?” On small inputs, like the example above, it is very fast.

Comparing small structures like {:some "map" :that "I typed in"} is always going to be fast.

DjebbZ commented 5 years ago

Note that I've been bitten in the past by such structural comparison in Clojurescript before. Comparing 2 vectors (arrays) of deeply nested maps when only the last element was different (think products list), it mean basically that the comparison checked every field and nested field of every object in the vector to finally find a difference. The performance hit just killed the UI.

orestis commented 5 years ago

For me it seems that the user of the API is best placed to know:

a) How deep/complex the compared values will be b) What is the value of skipping a render (deep tree vs shallow tree)

DjebbZ commented 5 years ago

the user of the API is best placed to know

Exactly.

DjebbZ commented 5 years ago

Which means that if I want to skip based on a custom equality check (by custom I mean not the one provided by React), I can just do it before calling setState since I have access to both the old and new state.