prescottprue / react-redux-firebase

Redux bindings for Firebase. Includes React Hooks and Higher Order Components.
https://react-redux-firebase.com
MIT License
2.55k stars 559 forks source link

Performance drop between version 2.x and latest #1014

Open PiotrSzlagura opened 3 years ago

PiotrSzlagura commented 3 years ago

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

I believe this is a bug

What is the current behavior?

After upgrading from 2.x to latest version (3.7.0) I noticed a significant drop in performance. I sometimes need to render lists with a lot of elements connecting to firebase (e.g. 40, 80, etc). I am using regular firebaseConnect HOC. I checked how often does it execute, and found that in version 2.x it was executed around 400 times for 80 elements (which is a lot, but acceptable), but in version 3.7.0 it is being executed... 6700 times! And it completely freezes whole app for the time it runs.

My use of firebaseConnect looks like this (seems rather typical):

function getFirebasePath(id, intervalType) {
  return `entries/${id}/${intervalType}`;
}

@firebaseConnect(props => {
  const id = getIdFromProps(props);
  const intervalType = getIntervalTypeFromProps(props);

  return [getFirebasePath(id, intervalType)];
})
@connect(/* Later I collect data using regular connect */)

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.

Try rendering a lot of elements which connect to different parts of real time database, and check this list's performance on version 2.5.0 and latest.

What is the expected behavior?

Latest version should not be less performant than older one...

Which versions of dependencies, and which browser and OS are affected by this issue? Did this work in previous versions or setups?

Most important dependencies:

"react": "16.9.0",
"react-native": "0.61.4" (with Expo SDK)
"redux": "4.0.1",
"react-redux": "7.2.1",
"react-redux-firebase": "3.7.0",
"firebase": "7.18.0"

This problem is noticeable on both native (Android/iOS) and web (latest Chrome/Firefox).

prescottprue commented 3 years ago

Are you able to provide a reproduction that includes the state connection logic? That will most likely drastically impact debugging any type of performance issues

PiotrSzlagura commented 3 years ago

I'm not able to share codesandbox etc as I don't have equivalent firebase data available for testing, but here's my code:

function getQuoteData(state, id, intervalType) {
  return state.firebase.data &&
    state.firebase.data.quotes &&
    state.firebase.data.quotes[id]
    ? state.firebase.data.quotes[id][intervalType]
    : null;
};

function getFirebasePath(id, intervalType) {
  return `quotes/${id}/${intervalType}`;
}

@firebaseConnect(props => {
  const id = getIdFromProps(props);
  const intervalType = getIntervalTypeFromProps(props);

  return [getFirebasePath(id, intervalType)];
})
@connect((state, props) => {
  const id = getIdFromProps(props);
  const intervalType = getIntervalTypeFromProps(props);

  return {
    quoteData: getQuoteData(state, id, intervalType),
  };
})
class WithQuoteData extends React.Component {
  render() {
    const { quoteData } = this.props;

    const emptyQuoteData = quoteData === null;

    return (
      <WrappedComponent
        {...this.props}
        quoteData={quoteData}
        emptyQuoteData={emptyQuoteData}
      />
    );
  }
}
lmick002 commented 3 years ago

@prescottprue did you resolve these issues in your project? I have a similar issue where I'm using multiple firestoreConnect HOC and it locks up the UI for 30-45+ seconds minimum at times. @prescottprue

prescottprue commented 3 years ago

@PiotrSzlagura there was a recent update in v3.8.1. which prevents duplicate listeners on the same query, would be interested to hear if that helps. When you say "it executes 6700 times" what part are you referring to executing? The render of the component? connection logic? action dispatches?

@lmick002 I don't currently have these issues in any project of mine, I was going to attempt to replicate with the provided code - a full app where the issue can be replicated would be helpful though

lmick002 commented 3 years ago

@prescottprue thanks for the quick response. I might be able to share some of the codebase with you/ or video as well. I can connect with you via twitter

PiotrSzlagura commented 3 years ago

@prescottprue What I meant was my @firebaseConnect executed that many times. I'll check latest version next week and I'll post an update here :)

PiotrSzlagura commented 3 years ago

@prescottprue Sorry it took so long, but I finally managed to check latest versions of lib and see if this helps. Unfortunately - nothing has changed. I took some time to test it further and found some very strange relationship between count of rendered elements and how many times @firebaseConnect executes.

To make it easier to visualize: I have a table, which has (adjustable) n rows and some columns. Two of these columns are using components which use firebaseConnect, so for n rows there always will be n * 2 firebaseConnects. But every row only uses one firebase endpoint, so each data fetched is used for two components.

And now for the weird part, I found that:

Rows firebaseConnect usages firebaseConnect executions
1 2 4
2 4 16
3 6 36
4 8 64
5 10 100

This indicates that number of firebaseConnect executions is always a square of firebaseConnect usages, which I can't really explain. Do you have any idea why this might be the case?

I checked if components which use firebaseConnect are mounted and unmounted, but my test indicates that this does not occur, each component is mounted only once.

prescottprue commented 3 years ago

Wondering if these changes will have any impact since it prevents attaching of duplicate listeners - being released in v3.10.0

PiotrSzlagura commented 3 years ago

@prescottprue I checked the latest version today, and I think it generally renders faster, but unfortunately I still have the same problem here.

PiotrSzlagura commented 3 years ago

I mean it still executes 2*n^2 times for each listener.

PiotrSzlagura commented 3 years ago

@prescottprue Hi, any updates here? Maybe there is some way around this issue?