Bug: unexpected rerender when using `useMutableSource`

[csr632]

React version: experimental

Steps To Reproduce

https://codesandbox.io/s/react-redux-usemutablesource-j1vgy

(modified from Brian Vaughn's fake react-redux demo)

1. Increment Buttons1(update state.s1) for several times, only Label1 is rerendered, which is expected.
2. Increment Buttons2(update state.s2), both Label1 and Label2 is rerendered, which is unexpected.

   Also notice that the unexpected render of Label1 is not commited, even when the render return value is different(since it render Math.random()).

Since Label1 return the same state.s1 from getSnapshot, Label1 should not rerendered.

This bug may lead to worse performance, because Components are rerendered for state fields that they don't need.

---

@bvaughn may be interested in this.
@dai-shi may also be interested in this because his use-context-selector need the correct behaviour to be efficient

[bvaughn]

Note that this isn't actually an issue with the second state, specifically, so much as it is an issue with the second mutation against the shared source. (So if you reverse steps 1 and 2 in the repro, the unexpected behavior will happen for step 1.)

[bvaughn]

I think the reason for the unexpected render of the label component is due to the useState bailout mechanism. Even though the state value doesn't change, one of the label components re-renders (shallowly).

It looks like React doesn't commit any mutations, or do any further rendering for sub-components (if the label component rendered children), so the overhead of this isn't large even though it's not ideal.

Here's a reduced test case:

const { createMutableSource, useCallback, useEffect } = React;

function getStoreVersion(store) {
  return store.getState();
}

function subscribe(store, callback) {
  return store.subscribe(callback);
}

function useSelector(selector) {
  const getSnapshot = useCallback((store) => selector(store.getState()), [
    selector,
  ]);

  return useMutableSource(mutableSource, getSnapshot, subscribe);
}

function reducer(state, action) {
  switch (action.type) {
    case "INCREMENT_ONE":
      return {
        ...state,
        one: state.one + 1,
      };
    case "INCREMENT_TWO":
      return {
        ...state,
        two: state.two + 1,
      };
    default:
      return state;
  }
}

// Faux Redux store
const store = {
  _listeners: [],
  _value: {
    one: 100,
    two: 200,
  },
  getState: () => store._value,
  dispatch: (action) => {
    store._value = reducer(store._value, action);
    store.version++;
    store._listeners.forEach((listener) => listener());
  },
  subscribe: (listener) => {
    store._listeners.push(listener);
    return () => {
      const index = store._listeners.indexOf(listener);
      if (index >= 0) {
        store._listeners.splice(index, 1);
      }
    };
  },
};

const mutableSource = createMutableSource(store, getStoreVersion);

const selector1 = (state) => state.one;
const selector2 = (state) => state.two;

function Grandchild({ value }) {
  Scheduler.unstable_yieldValue(`Grandchild render "${value}"`);
  useEffect(() => {
    Scheduler.unstable_yieldValue(`Grandchild useEffect "${value}"`);
  });
  return value;
}

function Child({ label, selector }) {
  const value = useSelector(selector);
  Scheduler.unstable_yieldValue(`Child "${label}" render "${value}"`);
  useEffect(() => {
    Scheduler.unstable_yieldValue(`Child "${label}" useEffect "${value}"`);
  });
  return <Grandchild value={value} />;
}

function App() {
  Scheduler.unstable_yieldValue("App render");
  return (
    <>
      <Child label="one" selector={selector1} />
      {"|"}
      <Child label="two" selector={selector2} />
    </>
  );
}

const root = ReactNoop.createRoot();
act(() => {
  root.render(<App />);
  expect(Scheduler).toFlushAndYield([
    "App render",
    'Child "one" render "100"',
    'Grandchild render "100"',
    'Child "two" render "200"',
    'Grandchild render "200"',
    'Grandchild useEffect "100"',
    'Child "one" useEffect "100"',
    'Grandchild useEffect "200"',
    'Child "two" useEffect "200"',
  ]);
});
expect(root.getChildrenAsJSX()).toEqual("100|200");

act(() => {
  store.dispatch({ type: "INCREMENT_TWO" });

  expect(Scheduler).toFlushAndYield([
    'Child "two" render "201"',
    'Grandchild render "201"',
    'Grandchild useEffect "201"',
    'Child "two" useEffect "201"',
  ]);
});
expect(root.getChildrenAsJSX()).toEqual("100|201");

act(() => {
  store.dispatch({ type: "INCREMENT_ONE" });

  expect(Scheduler).toFlushAndYield([
    'Child "one" render "101"',
    'Grandchild render "101"',
    'Child "two" render "201"', // Unexpected render
    'Grandchild useEffect "101"',
    'Child "one" useEffect "101"',
    'Child "one" render "101"', // Unexpected render after commit
  ]);
});
expect(root.getChildrenAsJSX()).toEqual("101|201");

[bvaughn]

I think the reason for the unexpected render of the label component is due to the useState bailout mechanism. Even though the state value doesn't change, one of the label components re-renders (shallowly).

Confirmed, in that I can reproduce the same "unexpected" behavior with just the useState hook.

const setStateCallbacks = {};

function Grandchild({ value }) {
  Scheduler.unstable_yieldValue(`Grandchild render "${value}"`);
  React.useEffect(() => {
    Scheduler.unstable_yieldValue(`Grandchild useEffect "${value}"`);
  });
  return value;
}

function Child({ label }) {
  const [value, setValue] = React.useState(0);
  setStateCallbacks[label] = setValue;
  Scheduler.unstable_yieldValue(`Child "${label}" render "${value}"`);
  React.useEffect(() => {
    Scheduler.unstable_yieldValue(`Child "${label}" useEffect "${value}"`);
  });
  return <Grandchild value={value} />;
}

function App() {
  Scheduler.unstable_yieldValue("App render");
  return (
    <>
      <Child label="one" />
      {"|"}
      <Child label="two" />
    </>
  );
}

const root = ReactNoop.createRoot();
act(() => {
  root.render(<App />);
  expect(Scheduler).toFlushAndYield([
    "App render",
    'Child "one" render "0"',
    'Grandchild render "0"',
    'Child "two" render "0"',
    'Grandchild render "0"',
    'Grandchild useEffect "0"',
    'Child "one" useEffect "0"',
    'Grandchild useEffect "0"',
    'Child "two" useEffect "0"',
  ]);
});
expect(root.getChildrenAsJSX()).toEqual("0|0");

act(() => {
  setStateCallbacks.one(0);
  setStateCallbacks.two(1);

  expect(Scheduler).toFlushAndYield([
    'Child "two" render "1"',
    'Grandchild render "1"',
    'Grandchild useEffect "1"',
    'Child "two" useEffect "1"',
  ]);
});
expect(root.getChildrenAsJSX()).toEqual("0|1");

act(() => {
  setStateCallbacks.one(1);
  setStateCallbacks.two(1);

  expect(Scheduler).toFlushAndYield([
    'Child "one" render "1"',
    'Grandchild render "1"',
    'Child "two" render "1"', // Unexpected render
    'Grandchild useEffect "1"',
    'Child "one" useEffect "1"',
    'Child "one" render "1"', // Unexpected render after commit
  ]);
});
expect(root.getChildrenAsJSX()).toEqual("1|1");

[bvaughn]

React has a fast-path bailout for state updates that don't actually change the current value- but we only eagerly compute and compare values if we're sure the update queue is empty (by checking that both the current and alternate Fibers lanes === NoWork).

This bailout optimization was added back in #14569. Looking at the original implementation notes from that PR, we see (emphasis added):

In some cases, we can bail out without entering the render phase by eagerly computing the next state and comparing it to the current one. This only works if we are absolutely certain that the queue is empty at the time of the update. In concurrent mode, this is difficult to determine, because there could be multiple copies of the queue and we don't know which one is current without doing lots of extra work, which would defeat the purpose of the optimization. However, in our implementation, there are at most only two copies of the queue, and if both are empty then we know that the current queue must be.

This is relevant to this issue because, in some cases¹ we don't reset lanes to NoWork when we're done processing an update, leaving an alternate around with the previous lanes value- so if another update is scheduled, the bailout mechanism won't kick in, and we'll end up shallowly re-rendering the component before bailing out.

¹ The case this occurs for follows the pattern of:

1. Schedule an update on a fiber (which sets a non-0 lanes value).
2. Start working on update.
3. cloneChildFibers clones the fiber, copying over the lanes value.
4. beginWork resets the lanes value when it starts rendering, (but only for the current fiber- not the alternate the update was scheduled on).
5. The render commits, at which point the current fiber has lanes === NoLanes but the alternate still has whatever non-0 value it had originally.

Fortunately this doesn't seem to happen often. Unfortunately it does happen sometimes. Still digging in to see if it's a known/unavoidable thing or if it's something we can improve.

[bvaughn]

After speaking with the team, this sounds like a known limitation of the bailout mechanism because of our current Fiber architecture. One positive clarification of my previous comment: this will only happen once per sub tree so the performance impact should be further minimized.

I'm going to close this issue for now since it seems the mechanism is working as designed (with a known limitation that I was not aware of until digging in).

Thank you so much for the helpful report with repro case ❤️

One quick note: We have some possible planned optimizations for useState (which would also impact useMutableSource) that could improve this case- but for now, this is working as designed.