Performance downgrade after update to v.6.0.0

[m1lk1way]

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

In ver.6.0.0 current state passing into context provider, it leads to update on every consumer (every connected component (on context.consumers)). Wrapped component isn't updates but React has work to be done and its influence on performance is greatly negative.
Our case when there is connected input to store and there are a lot of connected components on page and user starts typing fast (like long keypress), in this case we see lags (like input freezes and throws updates by portions).

What is the expected behavior?

As it was in v5.1.1.

Which versions of React, ReactDOM/React Native, Redux, and React Redux are you using? Which browser and OS are affected by this issue? Did this work in previous versions of React Redux?

  "react-dom": "16.7.0",
  "react-redux": "6.0.0",
  "redux": "4.0.1",

As an example, you can check this fiddle
https://x3yqx3ov1q.codesandbox.io/ - (upper component with greetings mustn't be updated when new ToDoItem added)
Source code of fiddle
https://codesandbox.io/s/x3yqx3ov1q

UPD it can be related to this issue in react repo facebook/react#13739
If so, what is the workaround (only one we came up with - downgrade of react-redux to v5.1.1)?

[timdorr]

We have a benchmark suite we have run against any major release to ensure we don't have severe performance regressions. 6.0 is slightly slower than 5.1.

If you're running into performance issues with 6.0, it's likely you were close to the edge of acceptable performance before. For instance, you absolutely should not be hooking up an input to the store. That is too high bandwidth and isn't state that affects the rest of the app. If you're doing typeahead-style search, you should instead batch dispatches with a debouncer. That's good for everyone, not just React Redux.

But in your example, that Test component you added isn't rendering when the store changes. We use much of the same methods for avoiding renders as before. There is an attempt to render, but it is blocked when there is no change detected.

[m1lk1way]

@timdorr firstly thx for answering this. Component isn't rendering, like you said, but react has a lot to be done (to build virtual dom and compare) and it takes huge part of lag time as Chrome Profiler says. And batch dispatches, you mentioned, seems like a crutch, not a fix. Anyway we have only one fix for this - downgrade (as we have no problems with previous versions, even if we multiply *5 number of connects on the page).

[PashaPash]

@timdorr we got a total performance screw on v6 upgrade.

you absolutely should not be hooking up an input to the store

That is exactly what redux-form does. They do connect every field to the store, and that causes mass updates for all connected components with new react-redux.
Live apps have forms field connected to the store. Moreover, connecting each list item to the store is a common performance optimization - that efficiently prevented updates and re-renders in v5.

I afraid that the tests are synthetic and does not show the real impact of the change. Most of them have just few connect calls.

For our prod app v6 just killed the performance. There are no re-renders, but there are mass updates on connected components and that somehow slows everything down. It seems that each extra connect slows down the entire page, so list and forms page are just slow. Extra connects were free in v5.

Do you have any ideas on how to track what exactly is slow, as there are no re-renders, just connect-level updates?

[markerikson]

@PashaPash , @m1lk1way : there's not much we can do unless you can provide projects that specifically demonstrate the performance issues. If you can provide repos that I can just clone and run, I might be able to look into things.

[markerikson]

I'm going to go ahead and reopen this, since overall perf is something I'd like to look into further.

[markerikson]

One idea I just had is to memoize the children of <Provider>. React will still have to walk the tree to find the context consumers, but at least that ought to prevent the entire tree from completely re-rendering every time the store is updated.

In practice, I'm not sure how much of a difference this would make, because it's likely there's connected components high up in the tree acting as "blockers" for the cascade of re-rendering from <Provider>'s setState call, but I don't think it can hurt.

[markerikson]

Just did a bit of experimenting, and I'm not sure the "memoize provider children" idea will actually do anything:

https://codesandbox.io/s/jln1wjor25

What I'm seeing is that when the parent calls setState(), the child component doesn't actually re-render at all, because the parent's props.children are identical to the last time. In other words, they're already "memoized" becaues React kept them around from the previous render for us, and React skips rendering when elements are the same as last time.

[Jessidhia]

Yeah, that will work as long as there is nothing else in between children and the <Provider/>

[markerikson]

Here's the current code:

react-redux/src/components/Provider.js

Lines 63 to 71 in 6e0a106

	render() {
	const Context = this.props.context || ReactReduxContext
	return (
	<Context.Provider value={this.state}>
	{this.props.children}
	</Context.Provider>
	)
	}

[PashaPash]

@markerikson thanks for reopening. not sure about prod code access, will build a sample based on it.

[PashaPash]

@markerikson @Jessidhia here is a code sample https://github.com/PashaPash/connect-sample
Sorry for a lot of garbage/unused code, copypaste from the typescript prod until I got some minimal working example.
Most of reducers and components are unrelated, can minimize it to some clear code if needed, but that will take a while.

Here what on the page:

• There is a list of 100 connected grid rows. Each row is connected directly to the state. That seems to be a common optimization pattern, followed the https://blog.isquaredsoftware.com/2017/01/practical-redux-part-6-connected-lists-forms-and-performance/#connecting-the-pilotslist-component. (@markerikson, your blog is really awesome!)
• A text box that do unrelated state changes, used as a trigger. Could be any other action trigger as well.

Measured timings on dev machine, i7-7700k,

On 5.1.1: Only text box gets updated.
Total event time: ~6-5ms:
Interactive update (<1ms Tree Reconciliation + Changes commit): ~1.5ms,

Timeline:

On 6.0.0: All connected rows gets updated (but not re-rendered)
Total event time: ~15-16ms:
Interactive update (10ms Tree Reconciliation + Changes commit): ~11ms

Timeline:

There is 6x slow down for the interactive update part. 10ms difference may not be an issue, please note that I'm measuring on dev pc, and components tree is quite simple.
Things are getting worth on average client PC. Typical consumer Core i3 is 4x times slower

Setting up 4x slowdown in chrome performance tools results in 80-100ms total event time (and 60-80 ms of Tree Reconciliation caused by connected components update).

We have a bit more components connected in the live app. So we are getting ~80-100ms lags on everything, including server-side pushes and API responses.
Some patterns, like multiple fields + save on blur are really laggy now - as user gets api response from the previous field save while typing a value in new field.
App feels to unpredictable lagging for the end user.

The root cause seems to be state passed via store provider in v.6 that leads to full tree traversal on store change in React 16

Any ideas how to fix that? Right now the only workaround is to stay on 5.1.1.

---

Another drawback of v6 behavior is mass unrelated updates highlight in React dev tools, as it breaks the common way of finding unwanted components updates, but that is not as critical as performance degradation for me.

[markerikson]

@PashaPash : Thanks for putting that together. I'm trying to juggle several different priorities atm, so I may not get to this one right away, but it's on my list.

If anyone else would like to help out by doing some analysis on this, I'd appreciate it.

[richie-south]

Hey :) We are seeing similar results :( when updating an isolated redux value all components are updated causing large slowdowns.

Isn't this what observedBits is supposed to solve?

---

We will stay on 5.1.1 until this is resolved

[timdorr]

Isn't this what observedBits is supposed to solve?

Not necessarily. That only gives us a 32 bit mask to work with, which wouldn't be enough for sufficiently large apps. We could use a bloom filter, but then you're getting near complexity levels that start to smell wrong.

Since the whole point of using state as the context value was to avoid tearing, did we ever come up with an example of tearing under 5.0 that was fixed in 6.0? I'd like to think through that problem in more detail and use a real example, rather than theory. Maybe there is some middle ground solution or something we can offer as an option? (e.g., <Provider asyncSafe store={store}>)

[markerikson]

@timdorr : a runnable example? no. conceptually, sort of.

The React team have talked about the concept several times, both in person and in various discussions online. Concurrent Mode and Suspense can both lead to situations where tearing may occur.

I'll admit I'm still a bit hazy on some of the exact details. That's part of why I've kept pestering the React team for specific examples we can poke at. I've kind of gotten pushed off with "well, go look at the conference demos we've done, you can't do that right with Redux". Then again, based on some of Sebastian's comments, it seems like they're needing to do some rethinking on how external state management interacts with React as a whole overall.

[timdorr]

Yeah, I think we need to see about making async safety an opt-in thing. My assumption is in async mode these perf issues stop being perceptible, whereas they block in sync mode and cause visible slowdowns. So, it's not that we made things faster or slower, we just moved work into the rendering path in a blocking manner.

[markerikson]

I think it's fair to say we made things slower in v6.

Specifically, in v5, connected components re-ran mapState immediately in the subscribe callbacks, and only called setState() once they knew they needed to re-render.

In v6, every store update calls setState() at the root of the component tree in <Provider>, and forces React to walk the component tree to find the consumers before they can even run mapState. That's a very meaningful difference.

Part of the issue here is that the state propagation model we settle on is pretty much an "all-in" approach. Either we propagate the state through context, which means having <Provider> subscribe and all the connected components pull from there, or we have to pull all the v5 code back in to handle subscribing in components and enforcing top-down subscriptions. I suppose it's hypothetically possible to ship both at the same time and have it controlled by an option somewhere, but the bundle size gets larger and then things get weird conceptually.

[m1lk1way]

Bundle size actually is something that can be traded for performance.

[shepmaster]

For instance, you absolutely should not be hooking up an input to the store

Is this a hard rule? For the Rust Playground, 90% of the app is an input field connected directly to the store. One of the things that I've enjoyed about Redux (and React Redux by extension) is that this was a very natural way of structuring the code. I'll be sad to learn that I need to reorganize my code to be able to upgrade without inflicting performance issues on my users.

[markerikson]

@shepmaster : it's not a hard rule, but it's something you should take into consideration.

Redux is a global state value with an event emitter. By definition, that means that if an action is dispatched that affects one tiny area of the state, all the other components in the app get notified as well, and need to check if the data they're interested in has changed.

There's a lot of back and forth discussion about what's appropriate to put into Redux. The Redux FAQ has an entry with some useful rules of thumb on what should go into the store.

Specifically for forms, the main questions are:

• Does the rest of the app need to know about this form data?
• If so, does the store need to be updated on every keystroke?

Even in v5, our general advice was "most forms probably aren't worth putting in the store". I specifically added a Redux FAQ entry discussing whether to keep form state in the store as well.

My own take is that form state that's kept in Redux should be buffered at the local component level, with a debounced action dispatching to update the store. That way, if the user types 10 keystrokes, only 1 action gets dispatched. I showed an example of this in my post Practical Redux, Part 7: Form Change Handling. There's also some related FAQ info on cutting down store notification events.

[PashaPash]

@markerikson @timdorr having a single way to propagate changes down the tree sound like a right decision for me.

The problem is that current React context API is SLOW, and they are not urging to fix it. It is not possible to get a measurable slowdown on pure React just because it is not possible to build a large app on pure React .

Most people will encounter performance issues with <Provider> API consumers instead, like react-redux, or any other state management connector that switch to the context API. And will complain in connector issue tracker, not to React team directly.

Is there any chance that facebook/react#13739 will be fixed in nearest feature? If no, that will be a blocker for migration for most of v5 users.

[Jessidhia]

The semi-official stance (from Seb) is that the context API is just the wrong primitive for a value that changes very often. There's nothing better for this specific use case than passing subscriptions through yet (the r-r@5 model).

It also seems to be his opinion that a single centralized store that's updated with everything is not itself a good model.

I think the "right" solution for this problem just hasn't been found yet. The traditional model (even if implemented with new context) will still be the most performant but will cause tearing and likely behave wrong in ConcurrentMode.

---

Maybe there is a middle ground here -- expose a subscribe and a getState API bound to the Provider through the context instead of the currentState; this makes them immutable from the point of view of React. When the store updates, save the state to a Provider-local state, triggering a local update (idk which scheduling priority to use here; Normal? is it even safe to defer unless the user already deferred?); if the Provider is a PureComponent this shouldn't propagate at all and render very quickly. Notify subscribers on the Provider componentDidUpdate, the subscribers then can all query the current state without tearing even if there are intermediate updates.

The problem is that this essentially codifies cascading updates into the API. My hunch is that this would be even worse for performance than the v6 model, unless the potential scheduler awareness gains that back. I don't know if React will know to batch any further store updates that happen during the current commit or the cascading render that might have been started by the update.

The list of subscribers doesn't have to be in state, can be an instance variable; putting that list in state would probably be even worse for performance.

It is possible to cancel deferred updates (so we can just continuously queue/unqueue a setState until the store stops changing faster than react can consume callbacks), but that depends on whether we do deferred updates or sync updates. The ability to cancel it prevents the user from being able to do dispatches that synchronously commit at all, at best they can use Immediate priority to skip ahead in the queue -- but at any rate, this would only affect the React updates to the store changes, not the store changes themselves.

((I did not check if ReactDOM.unstable_syncUpdates (what React uses to invoke event handlers) would allow users to work around this. At any rate, this still feels like a feature rather than a bug, unless you really want to put layout information inside redux.))

[Jessidhia]

Putting the above in annotated code probably makes it easier to understand:

const ctx = createContext()
const Provider = memo(function Provider({ store, children }) {
  // const subscriptions as a mutable Set
  const [subscriptions] = useState(() => new Set())
  // this goes in the context and, as subscriptions can't be reassigned, is immutable
  const subscribe = useCallback(
    cb => {
      subscriptions.add(cb)
      // do we call the cb() synchronously to replicate redux's own subscribe API?
      // do we schedule an independent update?
      // right now do nothing
      return () => subscriptions.delete(cb)
    },
    [subscriptions]
  )
  // perhaps unnecessary but explicitly empties subscription list on unmount
  useEffect(
    () => () => {
      subscriptions.clear()
    },
    []
  )

  // where we copy the store's state
  const [state, setState] = useState(() => store.getState())
  // used to make getState immutable; getState goes in the context
  const stateRef = useRef(state)
  stateRef.current = state
  const getState = useCallback(() => stateRef.current, [stateRef])

  // the currently scheduled callback
  const currentUpdate = useRef(undefined)
  useEffect(() => {
    const unsub = store.subscribe(() => {
      const state = store.getState()
      if (state !== stateRef.current) {
        // this effectively debounces updates until the store stops
        // updating faster than React
        if (currentUpdate.current !== undefined) {
          cancelCallback(currentUpdate.current)
        }
        // this inherits the priority level of the store.dispatch()
        // call that triggered the store update (Normal by default)
        currentUpdate.current = scheduleCallback(() => {
          currentUpdate.current = undefined
          setState(state)
        })
      }
    })
    return () => {
      if (currentUpdate.current !== undefined) {
        cancelCallback(currentUpdate.current)
      }
      unsub()
    }
    // everything here is either the 'store' prop or is immutable
  }, [store, stateRef, subscriptions, currentUpdate])

  // notifies subscriptions if the local state changes
  useEffect(() => {
    // maybe unneeded bailout?
    if (subscriptions.size === 0) {
      return
    }
    batchedUpdates(() => {
      for (const cb of subscriptions) {
        // the callbacks presumably come from a consumer that has
        // access to the provider; however, we could pass
        // stateRef.current as an argument here instead of requiring
        // them to access getState() from the context.
        cb()
      }
    })
  }, [state, subscriptions])

  // context should be immutable unless the 'store' prop is replaced
  const [context, replaceContext] = useState({
    subscribe,
    getState,
    store
  })
  // handle the 'store' prop update as a synchronous cascading update
  // (like getDerivedStateFromProps)
  if (context.store !== store) {
    // this is the only dangerous update in this component AFAICT,
    // and should only happen if the store instance is itself replaced,
    // but never on first mount
    stateRef.current = store.getState()
    replaceContext({
      subscribe,
      getState,
      store
    })
  }

  // everything here immutable unless props are changed!
  return useMemo(
    () => <ctx.Provider value={context}>{children}</ctx.Provider>,
    [context, children]
  )
})

---

The updates to the store will always be asynchronous (unless ReactDOM.unstable_syncUpdates can affect it -- I did not check). They will inherit the current priority the scheduler has at the time you call .dispatch on the store from any scope, inside or outside the context.

unstable_runWithPriority(UserBlocking, () => { ctx.store.dispatch(foo) }) would be a way to cause the update to have UserBlocking priority, for example. (unstable_runWithPriority is, perhaps confusingly, synchronous. It just affects the priority level that unstable_scheduleCallback calls inherit.)

[gaearon]

We had a lengthy chat with @markerikson yesterday. Our current recommendation is to revert to subscriptions, but the v4 kind — not v5. To enforce top down flow, use batchedUpdates.

To fix tearing we could add some band-aid, like Relay does. IIRC it reads a “version” and if it detects a different store version between renders, it forces a sync update.

In longer term we’ll want something different but I think this will check most boxes now and will be fast.

[Jessidhia]

If there is no negotiation at the connect / useRedux / etc level to ensure subscriptions are well-ordered, this would require a store enhancer to wrap all dispatches in batchedUpdates and hope React reorders the state updates properly. Is it reentrant / reliable?

[gaearon]

We need to be clear that there are several “levels” of compatibility with new features of React. They’re not formalized anywhere yet but a rough sketch for a library or technique X could be:

1. X breaks in sync mode
2. X works in sync mode but breaks in concurrent mode
3. X works in concurrent mode but limits its DX and UX benefits for the whole app
4. X works in concurrent mode but limits its UX benefits for the whole app
5. X works in concurrent mode but limits its DX and UX benefits for features written in X
6. X works in concurrent mode but limits its UX benefits for features written in X
7. X works in concurrent mode and lets its users take full advantage of its benefits

This is not a strict progression and there’s a spectrum of tradeoffs. (For example, maybe there is some temporary visual inconsistencies such as different like counts, but no crashes are guaranteed.)

But we need to be more precise about where React Redux is, and where it aims to be.

[gaearon]

this would require a store enhancer to wrap all dispatches in batchedUpdates

I think that’s probably the best way. And maybe opt in reexport of batched updates for more manual control.

[markerikson]

Yeah, it's gonna take me some time to go back through everything we discussed, absorb it all, and come up with a roadmap.

I know we discussed the batchedUpdates aspect, but "behave v4-style" wasn't quite what I had taken away from things. But it was also getting late, and my brain was already pretty fried :)

[shepmaster]

My own take is that form state that's kept in Redux should be buffered at the local component level, with a debounced action dispatching to update the store. That way, if the user types 10 keystrokes, only 1 action gets dispatched. I showed an example of this in my post Practical Redux, Part 7: Form Change Handling. There's also some related FAQ info on cutting down store notification events.

I have issues with this in general because I'm guessing that many people doing this are doing it in an incomplete fashion. By "many people", I mean that my team has implemented it poorly, and, luckily for me, your example also experiences the same problem. I'm happy to discuss concrete solutions to it on that issue, but I believe that it's an example of how what we think is "local" state really isn't.

My own mantra has been to place everything into Redux and to rely on tools like React and Reselect to reduce spurious updates. My usages of React's setState are only done after much hand-wringing on my part.

[quentez]

Just adding our experience here: we have a pretty big app that has a slow rate of update for our redux store. The problem is that when it gets updated, it does affect almost every single part of the app (we use reselect to limit that impact as much as possible, but still). We haven't been able to upgrade to v6 so far because every single time the user navigates from one page to another has doubled in processing time. We used to be around 100ms in production, and v6 puts us closer to ~250ms, which is a very user-noticeable difference.

[markerikson]

@quentez : if you can provide an example repo that demonstrates those particular perf issues, that would definitely help.

[markerikson]

This issue has been partially superseded by the roadmap I just laid out in #1177 .

Key point: we're going to switch connect back to using direct subscriptions internally. We're also going to work on expanding our benchmark harness, and hopefully ensure that the next meaningful release is at comparable in perf to v5.

I'll leave this open for now, but basically this will get resolved when we actually figure out what the right internal re-mplementation of subscription logic is.

[mikechabot]

Thanks @markerikson. For the sake of completeness, I entered a (now closed) issue on react-devtools since I thought it was giving false positives. But there is a (tiny) working example in the issue that manifests the problem, for whatever it's worth.

facebook/react-devtools#1290

[alexreardon]

facebook/react#13739

[markerikson]

Resolved! see https://github.com/reduxjs/react-redux/releases/tag/v7.0.1