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

Low performance in React Native with Redux #443

Closed
aleksey-shishkevich opened this issue Jul 25, 2016 · 35 comments
Closed

Low performance in React Native with Redux #443

aleksey-shishkevich opened this issue Jul 25, 2016 · 35 comments

Comments

@aleksey-shishkevich
Copy link

aleksey-shishkevich commented Jul 25, 2016

I have discovered some issues with performance in my react native app. It seems to be caused by react-redux bundle.

As you can see in the video

https://youtu.be/D5L-RM5EY5E

there is a significant delay between action dispatching and view rendering. On real devices it looks even worse. There are no API calls in this example. Only simple actions dispatching and state changes. On other hand Facebook Flux implementation and simple call of setState work much more faster.

Any ideas how to improve the app performance?

I am using react: v15.2.1, react-native: v0.29.2, react-redux: v4.4.5,
View

import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';

import {Map} from 'immutable';

import * as testActions from '../reducers/test/testActions';
import {Actions} from 'react-native-router-flux';

const actions = [
  testActions
];

function mapStateToProps(state) {
  return {
      ...state
  };
}

function mapDispatchToProps(dispatch) {
  const creators = Map()
          .merge(...actions)
          .filter(value => typeof value === 'function')
          .toObject();

  return {
    actions: bindActionCreators(creators, dispatch),
    dispatch
  };
}

........

<View style={styles.box}>
    {PRICE_FILTERS.map(filter =>{
        let price_cont_style = {};
        let price_text_style = {};
        if (filter.id == PRICE_FILTERS[3].id){
            price_cont_style.marginRight = 0;
        }
        if (filter.id == this.props.test.price){
            price_cont_style.backgroundColor = 'black';
            price_text_style.color = 'white';
        }
        return(
        <TouchableOpacity 
            key={filter.id} 
            style={[styles.price_cont, price_cont_style]} 
            onPress={()=>this.props.actions.setPrice(filter.id)}>
        <Text 
            style={[styles.price_text, price_text_style]}>
            {filter.label}
        </Text>
        </TouchableOpacity>
        );
    })}
</View>

......

export default connect(mapStateToProps, mapDispatchToProps)(PerformanceTest);

Actions

const {
    TEST_SET_PRICE,
} = require('../../lib/constants').default;

export function setPrice(filter) {
  return {
    type: TEST_SET_PRICE,
    payload: filter
  };
}

Reducer

const {
    TEST_SET_PRICE,
} = require('../../lib/constants').default;
const InitialState = require('./testInitialState').default;
export default function authReducer(state = InitialState, action) {
  switch (action.type) {

  case TEST_SET_PRICE:
        if (state.price!=action.payload){
            return {price:action.payload}
        } else{
            return state;
        }

    }
    return state;
}
@lourd
Copy link

lourd commented Jul 28, 2016

Your mapDispatchToProps function looks pretty hairy. Why are you using an immutable Map there? Initializing the Map, merging, filtering, and then converting back to an object is quite computationally expensive. That will run a lot, possibly every time the store's state is updated. I would bet that is your performance bottleneck.

I think you could simplify it to:

function mapDispatchToProps(dispatch) {
  return {
    setPrice(id) {
      dispatch(testActions.setPrice(id))
    },
  }
}

Your mapStateToProps function could also be optimized a little. Just return state instead of return { ...state }, there's no need to copy the tree there.

@aleksey-shishkevich
Copy link
Author

Thank you for the advice but i see same delays after applying your changes. This makes me sad because i really like redux and would like to use it for further projects

@lourd
Copy link

lourd commented Jul 28, 2016

Hm, darn. If you link to a reproduction in a separate repo someone may be able to help out more.

Rest assured, setting a single value in the store and re-rendering a component is definitely, definitely performant — React, React Native, or otherwise.

@aleksey-shishkevich
Copy link
Author

Yes, i definitely should create separate simple project for testing. still hope it's a result of libraries conflict.
I supposed i have to use only actions/reducers for containers rerendering and passing props for other components. and did not use setState at all. but maybe i got Redux wrong

@jimbolla
Copy link
Contributor

@lourd If mapDispatchToProps doesn't depend on props (function.length === 1), I'm pretty sure it is only run once per component instance.

@alsh76 Your mapStateToProps will cause your component to rerender every time any action is fired. You should have it be more picky about what parts of state it needs.

@lourd
Copy link

lourd commented Jul 28, 2016

Right, thanks for the correction. Having just read through #416 and #407, I know you know what you're talking about 😉

@aleksey-shishkevich
Copy link
Author

@jimbolla there is the only action in my example. and button pressing causes the only dispatching and the only rendering. the issue happens because of delay between dispatching and rendering. not because of multiple rerenderings.

@foiseworth
Copy link

@alsh76 Have you tried you demo without any middleware (including redux-logger)?

@aleksey-shishkevich
Copy link
Author

aleksey-shishkevich commented Jul 29, 2016

@foiseworth I tried to remove redux-logger and all console.log from the code. It did not help - same delays.

Actually it was a test page from my current project that uses redux-thunk and react-native-router-flux as well as some other libraries.

i am gonna create test project with minimal dependencies and put it on github
but honestly i need thunk and router - if the issue is caused by them i will be forced using Flux. With Flux it works good enough

@aleksey-shishkevich
Copy link
Author

@lourd, @jimbolla, @foiseworth As it turned out the cause of this issue is all components from navigation chain are staying unmounted and get rerendered behind of the visible scene
See more details here Possible navigation issue in React Native/Redux app

So it seems not a redux issue but nevertheless I would be very grateful for advices how to handle multiple unmounted components from navigation stack in the best way. I am checking in shouldComponentUpdate if current component is visible but maybe there is the better way?

@max-mykhailenko
Copy link

@alsh76 please test your example with the last react-redux@next. I have almost same problem and update doesn't fix it :(

@aleksey-shishkevich
Copy link
Author

@max-mykhailenko as i figured out my performance issues were caused by navigation system.
when the user navigate through pages like this Scene A => Scene B => Scene C all components A, B, C are staying mounted. Because all reducers are combined into one any action dispatching causes passing props to every mounted component.

For example if the user makes any changes and dispatch them in Scene C all mounted components A, B, C are receiving props and getting rendered in same order A => B => C. If components A and B are big enough (e.g. big ScrollView list) it causes visible delays between Scene C actions and Scene C rendering as in my example

a workaround for this - do not allow rendering of not visible components in shouldComponentUpdate. it resolved my performance issue

i hope it will help you

@max-mykhailenko
Copy link

@alsh76 Thanks for response. I use NavigatorExperimental and also saw lags, but with react-native-router-flux I got good performance. How do you check your active screen for shouldComponentUpdate?

@aleksey-shishkevich
Copy link
Author

@max-mykhailenko i am using react-native-router-flux. as well as RN Navigator it keeps all components from navigation stack mounted and all of them are rendered together.

with react-native-router-flux you can use your own reducer to detect start of navigation as in their example REDUX_FLUX ActionConst.BACK_ACTION for Actions.pop() and ActionConst.JUMP for tabs switching. First reducer's param contains scene key and name.

also it's important to detect end of navigation (transition between scenes). No rendering should be allowed in transition time. When navigator animation and rendering happen simultaneously it causes freezes and delays in my app. No event is fired at the end of transition. I am using
InteractionManager
to detect end of navigation.

if you are using NavigatorExperimental that's even simpler you have all callbacks out of the box https://facebook.github.io/react-native/docs/navigation.html#step-4-create-a-navigation-stack

and common rule i came to - the less renderings the better.

@aljux
Copy link

aljux commented Nov 10, 2016

@alsh76 How did you did you check if components were visible?

@mschipperheyn
Copy link

@aljux, I use NativationExperimental but I imagine the flux one is similar in the sense that a list of routes will be maintained with an index to indicate "where we are". You can easily calculate the "distance" between the index and the requested route

@alsh76 I have had a lot of problems with performance on NativationExperimental. Aside from the trick abovem, another trick that made a HUGE ( ;-) ) difference is the following:

render(){

    const { navigationState, onNavigate, onBack } = this.props;
    return (
        <NavigationCardStack
                       onNavigateBack={this.navigateBack}
                       navigationState={navigationState}
                       renderScene={this._renderScene}
               renderHeader={this._renderHeader}
           onTransitionStart={this._navigationStart}
                       onTransitionEnd={this._navigationComplete}
                />
    );

in the navigationStart and End methods I set a state boolean 'navigating'. When it's true, I force the sceneComponent to not re-render through shouldComponentUpdate.

Another issue you may or may not have is that there are many more render cycles than people often realize. If you don't have a system to "guard" against fetch requests being unnecessarily re-executed with every render cycle, you will also see a big negative performance impact and even recursive loops.

@aleksey-shishkevich
Copy link
Author

@aljux just watched for navigation events and keep full navigation history and a key of visible scene. code for react-native-router-flux looks like

   reducerCreate(params){
        const defaultReducer = Reducer(params);
        return (state, action)=>{
            switch (action.type) {
                case ActionConst.PUSH:
                    // current scene key is action.key
                    break;
                case ActionConst.BACK_ACTION:
                   // ....
                    break;
                case ActionConst.JUMP:
                    // tab switching
                    break;
                case ActionConst.FOCUS:
                    //can be useful
                    InteractionManager.runAfterInteractions(....);
                    break;
            }
            return defaultReducer(state, action);
        }
    } 

render() {
   return (
        <Router createReducer={this.reducerCreate}  hideNavBar={true}>
        <Scene key="root" >
....

maybe in recent versions you can get foreground scene directly. should check that

@aleksey-shishkevich
Copy link
Author

@mschipperheyn yes, thank you for pointing out. I forgot to mention about blocking re-rendering during scene-to-scene transition. If not using NativationExperimental then should use
InteractionManager.runAfterInteractions. This callback is run on the end of navigation animation - the only way to detect end of the animation in my case

@aljux
Copy link

aljux commented Nov 11, 2016

@alsh76, @mschipperheyn Thanks.
Im using the regular Navigator component together with redux.
But I cant figure out how to check inside the component which index that is currently on top?
Since the navigator it self does not have that kind of property?

@mschipperheyn
Copy link

Ok yeah navigation experimental does have that property. Don't know about the one u are using. I understood that fb wants to release some. Kind out of out of the box based on navigation experimental. Don't know ur time frame

@aleksey-shishkevich
Copy link
Author

aleksey-shishkevich commented Nov 11, 2016

@aljux You can track user actions (navigation buttons and hardware back button presses). To detect end of scene-to-scene transition use InteractionManager.runAfterInteractions

Have you tried onDidFocus and onWillFocus ? As far as I understand these callbacks allow to detect end and start of navigator transitions. Maybe their arguments refer to top component

@aljux
Copy link

aljux commented Nov 12, 2016

@mschipperheyn @alsh76 thanks for the tips and info, really appreciate it!
I will look into it asap.

@kuby
Copy link

kuby commented Dec 6, 2016

Actually I have the same problem you mentioned in the begining, but it my case, thats propably not a navigator problem. I use react-native-router-flux for navigation, but when I start my app, there are no other components mounted (and in most cases I use Action type: reset, so it resets the stack and the result is, that there wont be other components mounted "in the background").

So now on, I have no idea how to solve my issue. The case is, I am using Redux for changing a show/hide state of two components (the one I tap on -> i have to change the source of an Image component - the icon of the button, and the other is a little box that has to be showed/hidden, depending on that value from state). With the help of connect I get from my mapStateToProps the needed property "show" from reducer, and my two components depending on this value. It works fine, but there is a delay (just like in the video example on the top of this topic).

The render function is called with some ~ 1s delay. (if I add some console.log to the begining of my render function, it shows immediatly, but the components in return() are re-rendered just within that delay).

Anyone else noticed this? Any tips how can I fix this issue? (the console.log was there just for a test, there are no more logs, nothing that should slow this)

Update: If I use setState instead of Redux, it works well without the delay, but I dont know if I have another option in this case (I am controlling a component with another one - and they are not related to each other)

@ganmor
Copy link

ganmor commented Dec 9, 2016

For what it's worth here is what I do right now.
Store the route for one component tree in context

We created a wrapper for this
const CurrentRoute = React.createClass({ childContextTypes : { currentRoute: PropTypes.object }, getChildContext() { return { currentRoute: this.props.route } }, render() { return this.props.children; } })

And used it in render scene

<CurrentRoute route={route}><CurrentScene navigate={navigate} route={route} index={index} /></CurrentRoute>

Then you can access the route a component has been rendered into.

Store the navigation stack in a singleton
We use this code in configure scene
let routeStack = []; export const updateRouteStack = stack => routeStack = stack;

Then you can use this slightly modified react-redux connect function, it will skip updates if component is rendered in another component tree then the currently displayed one
( or a similar implementation )
master...ganmor:master

It might be possible to package this but I have'nt had the time to look into it.
Hope that helps

@davidsamacoits
Copy link

Hi guys! I'm still having the same issue, anyone find a good way to fix it?

@aleksey-shishkevich
Copy link
Author

@davidsamacoits blocking re-rendering of all currently not visible components in shouldComponentUpdate works for me. see more details above

@davidsamacoits
Copy link

@alsh76 Thank you for your fast answer, but how do I check if a component needs to be rendered or not?

@aleksey-shishkevich
Copy link
Author

@davidsamacoits it depends on which navigation system you are using.

I have to specify my issue was 100% navigation issue. Not a react/redux issue. I was using react-native-router-flux. When a user was opening new screens/scenes one after another ROOT TAB => Scene A => Scene B => Scene C all these scenes and their attached components stayed mounted (active). On every redux action all them got re-rendered despite of only the last one was visible

This issue could be resolved by tracking all navigation events. If you know which scene's component is currently visible (Scene C in my example) you can block re-rendering of other components (related to scenes ROOT TAB, Scene A, Scene B) by returning false in shouldComponentUpdate

If you are using another navigation system e.g. react-navigation then should think about using scene-to-scene transition callbacks to detect which scene is active and needs to be re-rendered. like onNavigationStateChange

@davidsamacoits
Copy link

@alsh76 thanks for your answer mate. I'm using react-native-router-flux as well. I'm just not sure about how to check what is the current screen. Is it a props property or something?

@aleksey-shishkevich
Copy link
Author

oh that was a bit tricky. I used old v3
First step was creating a custom reducer. It tracked navigation events and saved it in special redux store

   reducerCreate(params){
        const defaultReducer = Reducer(params);
        return (state, action)=>{
            switch (action.type) {
                case ActionConst.PUSH:
                    //fired on regular scene-to-scene transition
                    //you can detect active scene key by action.key
                    store.dispatch(navigationPush(action.key));
                    break;
                case ActionConst.BACK_ACTION:
                    //fired on android back action
                    store.dispatch(navigationBack());
                    break;
                case ActionConst.JUMP:
                case ActionConst.REFRESH:
                    //fired on tab switching
                    //action.key points on active tab
                    store.dispatch(changeTab(action.key));
                    break;
                case ActionConst.POP:
                case ActionConst.POP_TO: 
                    //fired on Action.pop()
                    store.dispatch(popTo(action.scene));
                    break;
                case ActionConst.FOCUS:
                    //immediately fired on any new scene opening
                    //it does not wait end of transition 
                    //....
                    break;
            }
            return defaultReducer(state, action);
        }
    }

....

  render() {
    return (
        <Provider store={store}>
        <Router createReducer={this.reducerCreate}>
        <Scene key="root">
        ....
	    </Router>
	    </Provider>

Second step - override shouldComponentUpdate in every your scene component

class SomesSceneComponent extends Component{

    shouldComponentUpdate(nextProps, nextState){
        //check navigation redux store for active tab and scene
        if (this_scene_is_not_active){
            return false;
        }
        return true;
    }
.....

@aleksey-shishkevich
Copy link
Author

aleksey-shishkevich commented Jul 13, 2017

@davidsamacoits If you are using v4 hopefully you can omit first step
https://github.com/aksonov/react-native-router-flux#v4-features
each ‘scene’ could have onEnter/onExit handlers.
Access to your app navigations state as simple as Actions.state, use Actions.currentScene to get name of current scene.
I hope that helps

@davidsamacoits
Copy link

@alsh76 thank you very much. I'm actually using v4 and I saw we can now use Actions.currentScene so I did that :

shouldComponentUpdate() { return (Actions.currentScene === 'login') ? true : false; }

Unfortunately, it still sounds to be a little bit laggy on a real device... :(

Do you tried or know any other routing libraries that does not involve this kind issue?

@aleksey-shishkevich
Copy link
Author

aleksey-shishkevich commented Jul 13, 2017

@davidsamacoits I did not try because finally got it worked fast and smooth enough
I would recommend also blocking component re-rendering during scene-to-scene transition (animation). Actually new scene becomes active once the animation just starts. So sometimes a scene gets updated during animation. The animation is not smooth, hopping

if you have similar issue shouldComponentUpdate() should return false until end of animation even for active scene. End of animation can be detected with InteractionManager callback

@aleksey-shishkevich
Copy link
Author

@davidsamacoits also take in attention production build will work faster than dev one. especially if you have debugger enabled

@orourkedd
Copy link

orourkedd commented Jul 30, 2017

FWIW I had this same "ghost latency" issue when in dev mode. When I did an actual ios build and deployed it to my iphone, the latency completely disappeared. Interestingly enough, this latency issue only happens on ios, not android. It must be something to do with the javascript runtime when in development.

@reduxjs reduxjs deleted a comment from NikiTsv Feb 1, 2018
@reduxjs reduxjs deleted a comment from ozberkctn May 2, 2018
@reduxjs reduxjs locked as resolved and limited conversation to collaborators May 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests