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

Disconnect from redux #2088

Closed
gentlee opened this issue Nov 22, 2023 · 20 comments
Closed

Disconnect from redux #2088

gentlee opened this issue Nov 22, 2023 · 20 comments

Comments

@gentlee
Copy link

gentlee commented Nov 22, 2023

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
Copy link
Contributor

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).

@markerikson markerikson closed this as not planned Won't fix, can't repro, duplicate, stale Nov 22, 2023
@gentlee
Copy link
Author

gentlee commented Nov 22, 2023

@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
Copy link
Contributor

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
Copy link
Member

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

@gentlee
Copy link
Author

gentlee commented Nov 22, 2023

@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
Copy link
Contributor

@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
Copy link
Author

gentlee commented Nov 22, 2023

@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
Copy link
Contributor

@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
Copy link
Author

gentlee commented Nov 22, 2023

@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
Copy link
Contributor

@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
Copy link
Author

gentlee commented Nov 22, 2023

@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
Copy link
Contributor

@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
Copy link
Author

gentlee commented Nov 22, 2023

@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
Copy link
Author

gentlee commented Nov 22, 2023

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Author

gentlee commented Nov 22, 2023

@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
Copy link
Member

phryneas commented Nov 22, 2023

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
Copy link
Member

phryneas commented Nov 22, 2023

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
Copy link
Author

gentlee commented Nov 22, 2023

@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.

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