reduxjs / react-redux

Official React bindings for Redux
https://react-redux.js.org
MIT License
23.36k stars 3.38k forks source link

Disconnect from redux #2088

Closed gentlee closed 10 months ago

gentlee commented 10 months ago

What is the new or updated feature that you are suggesting?

There should be a way of disconnecting from redux, which prevents both re-calculation of selector result and re-rendering of the component.

Why should this feature be included?

The main issue of bad performance of apps that use react-navigation or its analog with keeping components mounted when you switch them (like tab navigation or stack navigation) is subscription to redux. Especially relevant for React Native apps (Any app with tab bar and/or navigation stack).

Similar issue was created for mapStateToProps once upon a time.

What docs changes are needed to explain this?

There should be a new option in useSelector, smth like:

useSelector(selector, {
  ...
  enabled: boolean // when false, selector is not re-evaluated and component is not rerendered. default is true
})

Can be used like:

const isFocused = useIsFocused() // react-navigation hook
const data = useSelector(dataSelector, { enabled: isFocused })
markerikson commented 10 months ago

This sounds like you're asking for the still WIP React <Activity> component (recently renamed from <Offscreen>), which disconnects effects while the component is effectively disabled (and thus turns off effect-based subscriptions).

gentlee commented 10 months ago

@markerikson any idea how to implement it using current api? App is very slow already, no time to wait couple more years to have this feature released and all libs updated.

markerikson commented 10 months ago

Afraid not, no. Never worked with React Native, never used react-navigation. I'm not familiar with the constraints there other than getting the general sense of "components are kept alive when a new screen gets pushed on the stack".

How many components and how many active store subscriptions are you dealing with, that this is becoming a meaningful perf issue?

phryneas commented 10 months ago

afaik react-navigation can be used with react-freeze which probably does what you want.

gentlee commented 10 months ago

@markerikson it is an issue for web apps too, but making slow web apps looks like a standard in web development right now. Do you know any web app that is as fast as, for example, nice iOS app? On a slow device with low battery. And it is possible, if web devs and libs weren't so bad and companies spent some time on profiling and optimization.

But lags and freezes on mobile apps is not something that users are used to, 60 fps is a standard, so the expectations are much higher.

As for subscriptions/selectors, I'm not sure whats the best way to get the number, but I think there are hundreds of them. There are 5 screens in tab navigation, each can have stack with other screens. Each screen is a list of components, each of them is subscribed to redux. Also, there are subscriptions in root components like App (authorization, etc).

And each action (including saga) causes to all of them recalculate. There can be dozens of actions each second.

markerikson commented 10 months ago

@gentlee this is getting a bit off the original question, but:

what are you doing in those sagas that requires dispatching "dozens of actions a second"? That sounds excessive, and that starts to get into territory where Redux isn't necessarily well suited anyway.

fwiw I do care very deeply about React-Redux perf, and I have some research avenues I'd like to investigate like #2033 / #2047 , but that'll be well down the road.

my first general suggestion would be to look at the app architecture and see if you can cut down on the number of actions being dispatched in these cases.

gentlee commented 10 months ago

@markerikson there is an opensource app built on react native called mattermost. With redux it was extremely slow (1st version which is probably still in their github). So they swtiched to watermelonDB in the 2nd version (I don't think it was a good solution).

This app could get up to 100 actions per second and even more, because it is a chat app with websocket. I've optimized it a lot in a fork once, batching updates to redux, and reduced it to around 10-20 actions per second. The app was still slow on android because each mounted component had a subscription to redux too (theme was also stored in a single redux store 🤦‍♂️), each emoji of each message had a subscription to redux.

So dozens of actions is not a big number actually.

Also, on app start there could be dozens of requests sent to the backend and it is fine - each mounted screen of tab bar can send its own, and then get response, all initializations of all services - geolocation, in-app etc.

PS. They used react-native-navigation, not react-navigation, so they have mounted only single screen. If they used the second one and kept components mounted, I think the app would just freeze for half an hour.

markerikson commented 10 months ago

@gentlee if you can record some kind of JS perf trace capture (equivalent to a Chrome DevTools perf trace JSON format), I can try to take a look and get a sense of how much time is being spent on subscriptions vs re-rendering vs other issues.

The cost of individual subscriptions is not high. Several hundred subscriptions does have a cost, as every function call has a non-zero cost, but the larger question is really about the number of actions being dispatched, how long the selectors take to run, and how many components are being forced to re-render as a result.

gentlee commented 10 months ago

@markerikson I got one. chrome-supported.json.zip

getActivityBadgeSpec there, which takes one the most cpu time, is a mapStateToProps function of each cell of several screens in tab navigator.

Could be watched with chrome://tracing/ tab.

markerikson commented 10 months ago

@gentlee Okay, loaded that into https://perfetto.dev and did a quick look for getActivityBadgeSpec .

Looking at this screenshot for the first hit:

image

Your mapStateToProps function took 79ms to run here.

Similarly, a later hit here:

image

took 91ms to run.

That's hideous :) That should never be happening. Selectors are intended to be very fast, for exactly this reason. A given selector should only take a fraction of a millisecond to run every time - it should either be doing a simple return of state => state.x.y.z, or using memoization to avoid recalculations.

Looking at those stack traces, it seems like the real issue is the date handling. That looks like a lot of generating some kind of date config and parsing ISO strings, or something along those lines.

Fix that issue first so that these selectors don't keep taking that long, and your app perf will improve dramatically :)

gentlee commented 10 months ago

@markerikson Yes I'm currently optimizing mapping functions, but there are too many calls anyway. Also, it is an old android device in debug mode, and on my macbook m1 these selectors will evaluate for several ms as you say.

Also, there are a lot of re-renders and garbage collection as well, caused by useSelector re-evaluations (that could be disabled). And yes, regular expressions (from using moment.js) are very slow, that are calculated in mapStateToProps functions.

markerikson commented 10 months ago

@gentlee : if a selector is properly memoized, it shouldn't be generating re-renders or garbage collections, because it will end up as a no-op, not return a new reference, and not force a re-render.

the regex issue is an app-level concern. the fact that these are in every one of those calls tells me the selectors aren't properly memoized in the first place, otherwise they'd be avoiding the extra calculation attempts and be skipping the regex calls.

so, yes, my advice here is that your app looks like it would first benefit from a major overhaul of the selector usage, and that would significantly improve the app perf, separate from the question of how many useSelector calls you have or whether useSelector / connect checks are still getting triggered for background screens.

gentlee commented 10 months ago

@markerikson lets say we have 300 selectors which evaluate for 50ms. It is 15 seconds freeze. I will optimize them to be 5ms on old android device, it will be 1.5 seconds of unresponsiveness, which is also terrible.

But it could be 50 selectors of 5 ms - only 0.25 seconds. So number of active selectors also matters.

Also, when you open details screen you often update data from the list screen, and this data can be used by other mounted screens. So memoization doesn't always help.

gentlee commented 10 months ago

To sum up:

Current implementation has O(n) complexity, where n is number of selectors (of mounted components), while I'm searching for a O(1) solution.

markerikson commented 10 months ago

@gentlee : I agree that fewer calls to selectors will be less expensive that more calls. No argument there.

But, React-Redux will never be an "O(1)" approach. It's O(n), by definition.

My point is based on these perf profiles, your selectors are very badly written right now. That is the immediate problem.

Fix those selectors, and the app perf will greatly improve, and it's likely that it will improve enough that the cost of components subscribed in the background won't be an issue at that point.

phryneas commented 10 months ago

If you properly memoize your selectors it will for an action be 295 that take <0.01ms each and 5 that maybe take 3ms. The real problem you have here is that those selectors do too much and are not properly memoized.

gentlee commented 10 months ago

@markerikson it could have something like O(1) if I could disable all selectors of screens except currently visible one. Then even this very bad selector would be fine enough.

Again, I'm already optimizing these selectors (this cpuprofile was done before any optimizations), but from my experience it is not the only thing that should be done. You can check actually mattermost, they have very good memoized selectors, but the app is extremely slow (version 1).

phryneas commented 10 months ago

And we're not talking fast or slow device here. A sufficiently memoized selector will be <0.1 on a potato with energy savings mode.

phryneas commented 10 months ago

Honestly: it's a problem of react-navigation that they mount all screens. This can either be fixed in every single library that might be used there (react-redux being one of them), or in the upstream library. They already seem to be including redux-freeze to some degree, so why not activate that?

See https://github.com/software-mansion/react-native-screens#experimental-support-for-react-freeze

import { enableFreeze } from 'react-native-screens';

enableFreeze(true);
gentlee commented 10 months ago

@phryneas they mount screens to switch tab bar faster. Also, you can swipe back with your finger on iOS and the screen below should be mounted not to have a lag. Native frameworks do the same, for example UITabBarController keeps all views "mounted" all the time, same as UINavigationController. You just unsubscribe them from updates when they disappear and subscribe when appear. But react-redux lack that ability.

Yep, I'll check that freezing as well, thanks for idea.