Skip to content
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

Why calculateChangedBits = () => 0 #29

Open
dai-shi opened this issue Jun 27, 2019 · 15 comments

Comments

@dai-shi
Copy link
Owner

commented Jun 27, 2019

Background

React-Redux v6 (RR6) introduced what I'd call the state-based context value.
The idea was to let React propagate re-render instead of triggering re-render from outside of React. This makes it easy to ensure components render top-down the component tree.
Unfortunately, a way to bail out with useContext didn't come, as I understand because there's no way to implement that in an efficient way.
So, React-Redux v7 (RR7) went back to store context and subscriptions.

Problem (Hypothetical)

We are not sure what the final concurrent mode will look like, but @markerikson had a comment in the code.

// TODO We're reading the store directly in render() here. Bad idea?
// This will likely cause Bad Things (TM) to happen in Concurrent Mode.
// Note that we do this because on renders _not_ caused by store updates, we need the latest store state
// to determine what the child props should be.

My understanding is if a component reads a state from the store, it might not be consistent for all components in the tree. If React pauses rendering in the middle of the tree, Redux may update the store state. So, a component A and a component B could get different state even in a single render in the tree. I was hoping batchedUpdates solves this, but unless batchedUpdates run all renders synchronously, Redux has a chance to update the store state.
If we could only read a state from the context like RR6, this problem should be solved.

That doesn't mean all issues around concurrent mode are solved. a warning comment by @gaearon .

Solution

We specify calculateChangedBits = () => 0; so that React doesn't propagate updates.
Only a single Provider subscribes to the Redux store. All components under the Provider subscribe to the Provider. When the store state is updated, Provider re-renders which triggers listeners, subscribed components check updates (in useSelector) and forces render. When a component re-renders, it will read the state from the context, and we expect it's consistent. (No evidence, but that's how RR6 would have worked.)

Note, this still doesn't solve the stale props issue in useSelector.

Regret

If we had this solution half a year ago, we would have been able technically to base RR6 to add hooks support. (Updating peer deps might still require major version up, though.) It's too late and this doesn't provide any constructive suggestion now, but it may give a hint hopefully.

Final note

I read once again the 14110 issue, and found @sebmarkbage 's comment.

E.g. setting changed bits to zero and then manually force updating the relevant children.

It's already noted. I didn't have enough understanding back then. But, this is it.

@markerikson

This comment has been minimized.

Copy link

commented Jun 27, 2019

So lemme see if I understand what you're actually doing here:

  • You are putting the store state into context
  • However, because of the changedBits usage, no context consumers will ever be updated when that context provider value changes
  • Components subscribe to the <Provider>
  • When the <Provider> re-renders, it triggers those subscribers
  • The subscribed components do a forceUpdate()
  • That ends up as a batched-ish update across the tree
  • When React re-renders those components, they read the new store state out of context, thus having a consistent value across the tree

That sound about right?

It seems like this would still require React to traverse the entire component tree to find any potential context consumers, although none of them would match the bitmask. That's probably not as expensive as an actual render, but it's still O(n) with the size of the tree.

@dai-shi

This comment has been minimized.

Copy link
Owner Author

commented Jun 27, 2019

That sound about right?

Yes.

It seems like this would still require React to traverse the entire component tree

My guess is not. It seems like the Provider doesn't traverse the entire tree, but each component just "pull"s the context value. No evidence. How could we prove that?

What I can tell at this moment is that one benchmark (just one, which is js-framework-benchmark) shows the same or slightly better result compared to the store context (I mean comparison between RRR v4 and RRR v3).

@dai-shi

This comment has been minimized.

Copy link
Owner Author

commented Jun 27, 2019

Or, traversing the entire tree and matching the bitmask is just extremely fast.

@dai-shi

This comment has been minimized.

Copy link
Owner Author

commented Jun 28, 2019

@markerikson I'm not very familiar with React internal code, but this seems it handles changedBits=0 specially.

https://github.com/facebook/react/blob/9b0bd43550206e04bfe9ca695e5981eff0e2d03f/packages/react-reconciler/src/ReactFiberBeginWork.js#L2304

@markerikson

This comment has been minimized.

Copy link

commented Jun 28, 2019

Ah, good catch! I thought there might be some special handling there, but I hadn't dug into the code to see if that was actually the case.

I'd still expect direct subscriptions to be faster for sure when no components would need to re-render, and I'd generally expect them to still be faster anyway when some do need to re-render because fewer total components would be rendering.

Still, good to know this.

@dai-shi

This comment has been minimized.

Copy link
Owner Author

commented Jun 28, 2019

OK, glad to hear that you got what I've learned.

Personally, I'm comfortable with this new implementation. (react-tracked has to use this technique anyway, because it doesn't have any external store.)
Only the caveat is it's using the undocumented/unstable feature.

My intuition is that the performance would be comparable. Again, one benchmark showed fairly good numbers.
Anyway, I'm not suggesting any change to RR7 from the beginning.

As for benchmark, do you have any plan to update react-redux-benchmarks for hooks?
I could help with it, but I'm not sure which direction it would go, like if it's for useSelector only,
or it keeps comparing RR5, RR6, RR7, and RR7 with hooks.

because fewer total components would be rendering.

Just curious, fewer means just minus one, doesn't it?

@markerikson

This comment has been minimized.

Copy link

commented Jun 28, 2019

Imagine a scenario where a state change would actually cause, say, 10% of the components to re-render.

With direct subscriptions, 100% of subscriber callbacks run, but only 10% of the components mark themselves as needing to render. So, React can skip over rendering for large portions of the component tree.

With the () => 0 + subscriptions approach as I understand it, all wrapper components mark themselves as needing to render every time, and the "does the child need to render too?" logic ends up executing inside of rendering. So, 100% of the wrapper components would have to render, if I understand this right.

Not sure how much actual meaningful difference that gives, but it does seem like direct subscriptions would result in somewhat less work overall.

As for benchmarks... good question. @MrWolfZ said he forked our benchmarks repo and hooks-ified it during the hooks development cycle, but I haven't looked at that fork or tried to integrate any of the changes back into the main repo.

I've been mostly on vacation the last few weeks, and when I get back, my goal is to focus on Redux Starter Kit for a while. So, the benchmarks repo isn't a main priority for me atm. Would happily accept help on that, of course :)

@markerikson

This comment has been minimized.

Copy link

commented Jun 28, 2019

Huh. @dai-shi , check out this RFC for React context changes by @gnoff :

gnoff/rfcs#2

@dai-shi

This comment has been minimized.

Copy link
Owner Author

commented Jun 28, 2019

So, 100% of the wrapper components would have to render, if I understand this right.

No, I believe not. Only 10% of the components would have to render with the () => 0 + subscriptions approach, too. It's the same as direct subscriptions.

a) Because otherwise, I wouldn't have gotten the comparable benchmark result.

b) Imagine the normal context usage. Remember useForceUpdate is just a local state.

const ThemeProvider = ({ children }) => {
  const [theme, setTheme] = useState('dark');
  return (
    <ThemeContext.Provider value={[theme, setTheme]}>
      ...
    </ThemeContext.Provider>
  );
};

const MyComponent = () => {
  const [count, setCount] = useState(0);
  const [theme] = useContext(ThemeContext);
  const backgroundColor = theme === 'dark' ? 'gray' : 'white';
  return (
    <div style={{ backgroundColor }}>
      {count} <button onClick={() => setCount(c => c + 1)}>+1</button>
    </div>
  );
};

If the +1 button in MyComponent is clicked, only that component re-renders, and no other components in the Provider tree would re-render.

Hm, you mentioned "wrapper" components. Could you elaborate what you mean by that? @markerikson

@markerikson

This comment has been minimized.

Copy link

commented Jun 29, 2019

Oh, wait, I misunderstood something very important about your implementation.

I saw you were putting state into the context value, and assumed that was the only way components read the state value. But, it looks like you're handling things two ways:

  • During a standard render, components read the state value from context:
const { state, subscribe } = useContext(customContext);
const selected = selector(state);
  • But, when an action is dispatched, you're actually passing the current state value to the subscriber callbacks:
// Provider
listeners.current.forEach(listener => listener(state));

// components
const callback = (nextState) => {

That's totally different, then :)

Somehow I got the idea that the subscriber callbacks would immediately call forceUpdate() to force the component to re-render and read the latest value from context, which is clearly not what's going on here.

@dai-shi

This comment has been minimized.

Copy link
Owner Author

commented Jun 29, 2019

OK. Yeah, the callback part is not changed in that sense (compared to the store context value approach), and we still have the stale props issue in useSelector. (It would have been a big plus, otherwise.)

Now, as you get the idea, my question is how you like this approach. If I may ask, could this be taken seriously before RR7 is released?

@markerikson

This comment has been minimized.

Copy link

commented Jun 29, 2019

Yeah, it would have been something to consider. Not sure if it would have been the ultimate solution, but would definitely have been worth comparing.

@dai-shi

This comment has been minimized.

Copy link
Owner Author

commented Jul 18, 2019

I read this comment again.

State updates scheduled from componentDidMount or componentDidUpdate are processed synchronously and flushed before the user sees the UI update.

I knew this behavior, but this means that the current approach works in the concurrent mode because it de-opts to sync mode, so it doesn't get the benefit of the concurrent mode.

I hope reactjs/rfcs#119 solves this.

@dai-shi

This comment has been minimized.

Copy link
Owner Author

commented Jul 18, 2019

OK, I've updated my tool to check tearing: https://github.com/dai-shi/will-this-react-global-state-work-in-concurrent-mode

Now, it measures how long it takes for each action (which is clicking two buttons).
The result is, although my current implementation works in concurrent mode, but it de-opts to sync mode. hence, it's slow in UX.

  react-redux
    ✕ check no tearing (11ms)
    ✓ check avg delay < 300ms (1ms)
  reactive-react-redux
    ✓ check no tearing (2ms)
    ✕ check avg delay < 300ms (1ms)
  react-tracked
    ✓ check no tearing (1ms)
    ✕ check avg delay < 300ms
  constate
    ✓ check no tearing (27ms)
    ✓ check avg delay < 300ms (1ms)
  unstated-next
    ✓ check no tearing (52ms)
    ✓ check avg delay < 300ms
  zustand
    ✕ check no tearing (36ms)
    ✓ check avg delay < 300ms (1ms)
  react-sweet-state
    ✕ check no tearing (39ms)
    ✓ check avg delay < 300ms (1ms)
  storeon
    ✕ check no tearing (39ms)
    ✓ check avg delay < 300ms (1ms)
  react-hooks-global-state
    ✓ check no tearing (36ms)
    ✓ check avg delay < 300ms (1ms)
@snakeUni

This comment has been minimized.

Copy link

commented Jul 28, 2019

awesome !how to use calculateChangedBits = () => 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.