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

Idea: batch Subscription listener notifications #1511

Closed
OliverJAsh opened this issue Jan 28, 2020 · 9 comments
Closed

Idea: batch Subscription listener notifications #1511

OliverJAsh opened this issue Jan 28, 2020 · 9 comments

Comments

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Jan 28, 2020

Whilst investigating some performance issues on Unsplash, I discovered that when dispatching multiple actions in a row, React Redux will re-evaluate all selectors once each time an action is dispatched. It would be better for performance if it only re-evaluated selectors once after all actions had been dispatched, especially in an app with lots of selectors. I explain the problem in more detail here: https://medium.com/unsplash/react-redux-performance-considerations-when-dispatching-multiple-actions-5162047bf8a6#6af5

I understand that the batch function provided by React Redux is just an alias for React's own batch function. However, I wonder if React Redux could extend the batch function to avoid the problem I mentioned above.

My thinking is that Subscription listeners would only be notified after all actions have been dispatched (i.e. when the batch callback completes), rather than once after each dispatched action—just like how batch works to prevent multiple React re-renders, it could also work to prevent multiple re-evaluations of selectors. This way, selectors are only re-evaluated once, and it wouldn't be necessary for users to do something like I did with redux-batched-subscribe, as long as the batch function is used.

Perhaps there has been some prior discussion on this subject matter, but from a quick search I couldn't seem to find any. My idea might be flawed but I'm curious to know if there is any other way we can provide a solution to this problem out of the box.

@markerikson
Copy link
Contributor

markerikson commented Jan 28, 2020

As I mentioned in the other issue, we do already use batching within the individual Subscription instances, so that each nested level of subscribers get handled within a single render pass:

notify() {
const listeners = (current = next)
batch(() => {
for (let i = 0; i < listeners.length; i++) {
listeners[i]()
}
})
},

What you're describing is a different sort of behavior: trying to batch across multiple dispatches. That falls into how the Redux store notifies subscribers, which is a related but different topic.

I just wrote a blog post on the various techniques that can be used for batching:

A Comparison of Redux Batching Techniques

When a given action is dispatched, React-Redux has no way of knowing whether more actions might get dispatched in the near future. All it knows is that an action has been dispatched, the store has (presumably) been updated, and that components need to re-run mapState / selectors and queue re-renders if the results have changed.

What you're getting at is effectively some form of debouncing or throttling applied to subscriber notifications, which can be done, but only by modifying the behavior of the store itself. The downside there is that this affects all store subscribers. In React-Redux v7, only the <Provider> component subscribes to the store directly, and then nested components subscribe to their nearest connected ancestor:

const contextValue = useMemo(() => {
const subscription = new Subscription(store)
subscription.onStateChange = subscription.notifyNestedSubs
return {
store,
subscription
}
}, [store])

However, any other subscribers (such as a plain store.subscribe() call outside of React) would also be affected by the debouncing / throttling behavior as well because it's being applied to the store itself, which may likely not be the desired behavior.

Also see the Redux FAQ entry on "how can I reduce the number of store update events?" as well.

Note that React's "Concurrent Mode" does actually batch queued renders across event ticks. However, given how the Redux store works, there's a whole lot of uncertainty about how Redux and React-Redux will actually end up behaving in a React CM scenario. See #1351 for the latest discussion on that topic.

@timdorr
Copy link
Member

timdorr commented Jan 28, 2020

Yeah, this is just not how Redux works. We already do batching from the React side of the things, but the store itself will synchronously fire listeners when dispatching. There's nothing to batch without a store enhancer, which is out of scope for this project.

@timdorr timdorr closed this as completed Jan 28, 2020
@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented Jan 29, 2020

Thanks for your detailed replies—it's really appreciated. Learning a lot here!

which can be done, but only by modifying the behavior of the store itself.

Couldn't React Redux debounce its own store listener?

this.store.subscribe(debounce(this.handleChangeWrapper))

I tried making this change locally and it seemed to work. Selectors are only re-evaluated once after all dispatches, rather than once for each dispatch.

Before

image

After

image

@markerikson
Copy link
Contributor

That's not the default behavior we want, though. It may be something you might want, in rare specific cases, but as a default we need to re-render as quickly as possible.

@OliverJAsh
Copy link
Contributor Author

Given debouncing improves performance (as per above), what are the reasons why you might not want to do this? I'm speaking about debouncing the listener notifications only inside of React Redux, not for other store listeners.

@markerikson
Copy link
Contributor

Exactly the reason I just described. If we start debouncing updates, the UI updates will be delayed.

I'm certainly interested in potential ways to improve our existing behavior, but as far as I can see, you're focusing on the wrong metrics and constraints here.

@OliverJAsh
Copy link
Contributor Author

Exactly the reason I just described. If we start debouncing updates, the UI updates will be delayed.

Just to check I follow what you're saying, are you alluding to the discussion we're having in the other issue, i.e. the need to use useLayoutEffect? Or are you saying we can't debounce because then it would take longer for the UI to actually update? Sorry if I'm being slow here.

@markerikson
Copy link
Contributor

markerikson commented Jan 29, 2020

The latter. Debouncing might make multiple sequential dispatches result in a single collectively faster re-render than handling them individually, but it would make all individual dispatches render slower overall, and that's not something we want.

If you'd like to put together some sample projects that demonstrate differences, I can take a look. You might also want to look at the React-Redux benchmarks repo at https://github.com/reduxjs/react-redux-benchmarks .

But, simply off the top of my head, debouncing is not the approach we should be using here.

@OliverJAsh
Copy link
Contributor Author

Interesting, I thought that debouncing (with no wait time) wouldn't add any delay, in the same way that in fn in setTimeout(fn) will run immediately after the current call stack has finished processing. Like you said, it's hard to know for sure without benchmarking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants