[Performance] Short-circuit set recoil state if the value is unchanged

[marty-wang]

It seems that the current implementation of setting recoil state can be further improved when the value is the same as the current value. In this case, we don't have to go through all the enchilada and more importantly don't have to call setState in the end, which is not exactly cost free. I have observed major performance hit thanks to that. I have to cache the previous value myself and do the equality check before calling set recoil state when recoil has all the knowledge to that.

[BenjaBobs]

Related: https://github.com/facebookexperimental/Recoil/issues/1416

[marty-wang]

just want to note that this issue exists for both objects and primitives, like string, number or boolean, as the center of the issue is that recoil does not short-circuit propagating downstream and more importantly calling setState when values pass the equality check.

[drarmstr]

@marty-wang -

Note that atoms already implement this optimization (https://github.com/facebookexperimental/Recoil/blob/main/packages/recoil/recoil_values/Recoil_atom.js#L544). We have to be careful trying something like that at the same level for selectors because you don't want to always go through the expense of computing the existing value, even looking up in full cache may initiate dependency evaluations. The following short-circuit in setRecoilState() in Recoil_selector.js may work when updating upstream values:

        // Skip write if writing the existing value
        // Note: Optimization doesn't cover the "reset" case
        if (state.atomValues.has(recoilState.key)) {
          const existing = nullthrows(state.atomValues.get(recoilState.key));
          if (existing.state === 'hasValue' && setValue === existing.contents) {
            return;
          }
        }

However, another issue is that these short-circuits are still after some overhead for the hooks to start performing the action and potentially updating state. To avoid all that overhead we could try an additional short-circuit at a higher level like the hook. However, we have avoid incurring additional subscriptions, so something like that might work for the useRecoilState() hook but not the useSetRecoilState() hook. That also wouldn't be trivial to support the updater form of the setter. Consider the following:

function useRecoilState<T>(
  recoilState: RecoilState<T>,
): [T, SetterOrUpdater<T>] {
  if (__DEV__) {
    validateRecoilValue(recoilState, 'useRecoilState');
  }
  const value = useRecoilValue(recoilState);
  const setRecoilState = useSetRecoilState(recoilState);

  // Optimization to skip setting if same as the exiting value
  // TODO support updater form of setter
  const setValue = useCallback(
    (newValueOrUpdater: T | (T => T)) => {
      if (value !== newValueOrUpdater) {
        setRecoilState(newValueOrUpdater);
      }
    },
    [setRecoilState, value],
  );

  return [value, setValue];
}

However, this won't pass the unit tests because it would also cause a new instance of the setter function to be created whenever the value changes which would be a breaking change.

As these checks would also add their own overhead it would be worth measuring the impact and improvement to both the short-circuit and common cases, such as with Recoil_perf-test.js. Please feel free to continue exploring if you wish.

[marty-wang]

thanks for the explanation! I did the short-circuit in the user land with ref, namely, I have a useRef to store the previous value and before set the recoil state, I check if the incoming value is equal to the previous value, if true, I don't set the incoming value. this also gives me the opportunity to do the custom equality check, such as shallow equal or even deep equal if I need. By my measurement, it saves me lots of time in my own scenarios, that is why it promoted me to open the issue, hoping that it can be done in the library to benefit the community. I don't see if there is an issue with my approach. Please do let me know if you see any. thank you.

[drarmstr]

Using a ref could avoid the issue with new setter function instances. However, I guess another issue would be that it's only comparing with the existing value as of the last render, which may or may not be the current value with other batched actions or due to React concurrent rendering. So, potential risk for incorrect suppression in those situations..

The upstream selector short-circuit should be safer in that regards. Does that one alone help performance for your case?

[marty-wang]

sorry that I somehow fail to see your concern. In my mind, the ref always stores the last source-of-truth value, as it directly gets the value when it was set, it does not matter what value React uses for rendering, the ref is only used to compare against the next value when it is set. So the ref has nothing to do with rendering, rather has everything to do with what's being set, more importantly, if the next value should be set. As to the upstream selector short-circuit, I haven't tried. but based on my observation, the performance cost comes from the eventual setState call, which is not exactly cheap, let alone when many of them compound together. As long as the short-circuit happens before setState call, I believe that the performance will be good.

[drarmstr]

Consider the case where a component renders, which updates the "current" value in the ref. Then the user performs some action which causes the atom to be updated and then calls the setter provided by that latest component render. In this case the "current" value in the ref from the latest render is now stale.

If the upstream selector short-circuit works for your case it would be a lot safer since it's actually looking at the current state, not a potentially stale cached version of it. Can you drop in that code snippet and test the performance?