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

RN 0.45 introduced a 2-3s delay on iOS FlatLists with header components #15115

Closed
gregblass opened this issue Jul 20, 2017 · 13 comments
Closed
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.

Comments

@gregblass
Copy link

gregblass commented Jul 20, 2017

I'm experiencing a major delay with my FlatList component with a header when I introduced RN 0.45, and I think it may be a bug or performance regression. I have about ~110 items in the list. I created an expo snack to easily show what is happening:

https://snack.expo.io/rJwjIi6Sb

Line 53 is commented out.

If you load the app in iOS simulator or on an iPhone using Expo's QR code, you'll be able to immediately press on a list item and get an alert.

If you uncomment line 53 to add the search bar back to the header, and you should experience a pretty significant delay on first load (I count about 2-3 seconds on my iPhone 6) before the alert will fire off.

It seems that you can put literally anything in the header to cause this to happen. This doesn't happen in my 0.44 RN app (Expo SDK v17) using the same code.

If anyone has trouble reproducing this or you aren't getting the same experience described above, let me know. And if for some reason I'm doing something wrong that may be causing this, I'm all ears.

@gregblass gregblass changed the title RN 0.45 introduced a 2-3s delay oniOS FlatLists with header components RN 0.45 introduced a 2-3s delay on iOS FlatLists with header components Jul 20, 2017
@brentvatne
Copy link
Collaborator

This seems related to #14348

@xilonxiao
Copy link

@gregblass I'm not having exactly the same problem, but i got a huge performance drop on my flatlists after upgrading from 0.44.3 to 0.45.1. What I mean is that maybe your problem isn't exaclty on header component, but with flatlist like me. I haven't found a solution other than downgrading back to 0.44.3, but I'm seeing lots of similar issues around here, with people experiencing major performance drops just by upgrading RN, specially from 0.44.x to 0.45.x.

I think they should provide a better documentation about performance, specially on flatlists with better examples and explanation on how to use PureComponent, that could help fix the issue.

@TGPSKI
Copy link

TGPSKI commented Aug 23, 2017

I was running into two problems - extremely slow framerate (sometimes not even loading) when navigating to a large FlatList component, and the same navigating away from the FlatList page.

In addition to the normal steps (use PureComponent, use initialNumToRender), I got significant improvements by making the following changes to VirtualizedList.js:

componentWillUnmount - add this.setState && this._updateCellsToRenderBatcher.schedule();

  componentWillUnmount() {
    this.setState({
      first: this.props.initialScrollIndex || 0,
      last: Math.min(
        this.props.getItemCount(this.props.data),
        (this.props.initialScrollIndex || 0) + this.props.initialNumToRender,
      ) - 1});
    this._updateCellsToRenderBatcher.schedule();
    this._updateViewableItems(null);
    this._updateCellsToRenderBatcher.dispose();
    this._viewabilityHelper.dispose();
    this._fillRateHelper.deactivateAndFlush();
    clearTimeout(this._initialScrollIndexTimeout);
  }

componentDidUpdate - use this._updateCellsToRenderBatcher.schedule();

componentDidUpdate() {
    this._updateCellsToRenderBatcher.schedule();
  }

in _scheduleCellsToRenderUpdate(), replace this._updateCellsToRender() with this._updateCellsToRenderBatcher.schedule(); in the if (hiPri) statement

if (hiPri) {
      // Don't worry about interactions when scrolling quickly; focus on filling content as fast
      // as possible.
      this._updateCellsToRenderBatcher.dispose({abort: true});
      this._updateCellsToRenderBatcher.schedule();

@pull-bot
Copy link

@facebook-github-bot no-template

1 similar comment
@pull-bot
Copy link

@facebook-github-bot no-template

@facebook-github-bot
Copy link
Contributor

Hey, thanks for reporting this issue! It looks like your description is missing some necessary information, or the list of reproduction steps is not complete. Can you please add all the details specified in the Issue Template? This is necessary for people to be able to understand and reproduce the issue being reported. I am going to close this, but feel free to open a new issue with the additional information provided. Thanks! See "What to Expect from Maintainers" to learn more.

@facebook-github-bot facebook-github-bot added the Ran Commands One of our bots successfully processed a command. label Oct 10, 2017
@facebook-github-bot
Copy link
Contributor

Hey, thanks for reporting this issue! It looks like your description is missing some necessary information, or the list of reproduction steps is not complete. Can you please add all the details specified in the Issue Template? This is necessary for people to be able to understand and reproduce the issue being reported. I am going to close this, but feel free to open a new issue with the additional information provided. Thanks! See "What to Expect from Maintainers" to learn more.

@sahrens
Copy link
Contributor

sahrens commented Dec 8, 2017

@TGPSKI: wanna put up a PR for that and put me as a reviewer?

@sahrens
Copy link
Contributor

sahrens commented Dec 8, 2017

Also note that in your snack example, PureComponent won't help because you're creating a new onPress instance on every render with this line:

onPressItem={() => this.onPressItem(id, title)}

You need to pre-bind functions or implement a custom shouldComponentUpdate.

@mikelambert
Copy link
Contributor

Hey @sahrens I think this is has the same root cause as my issue here #17091 . I have a longer explanation for the "looping" that appears to cause the multiple-second delays, from my exploring the issue.

I believe @TGPSKI 's change should also fix my issue...as it basically disables all hiPriority updates. I wasn't sure what they were for exactly, so wasn't comfortable disabling them...but if we are removing them, we can simplify the resulting code more than @TGPSKI 's change. (Though Tyler you don't need the componentDidUpdate change, do you?)

I assumed the problem was due to variable-sized items, but perhaps I was mistaken there. I personally noticed the issue with a refresh component (which is positioned at negative position). But I suppose the use of a header causes the distTop to be negative too, and presumably triggers the exact same hiPriority loop behavior I describe in my issue.

@TGPSKI
Copy link

TGPSKI commented Dec 8, 2017

@mikelambert I'm working on a demo app + PR - I will take a look at a few of these optimizations.

In my case, I had a complicated header component in addition to the high item count FlatList. It has been a while since I dug into this problem, but I am looking at it again now.

@ChrisLahaye
Copy link

@sahrens wanted to confirm that @TGPSKI's patch improves user interaction performance significantly for me.

Also, regarding onPressItem={() => this.onPressItem(id, title)}, as I understand this is an inline-function and hence its reference keeps changing. Suppose this statement is a property of some component in some renderItem function that receives an item as argument, of which id and title are fields. How can we bind this callback without having its reference change each time if we need to pass data that is unknown by the component (since its only passed to renderItem).

@sahrens
Copy link
Contributor

sahrens commented Jan 20, 2018

There are a few options to optimize the onPressItem use case.

  1. pass this.onPressItem and id/title as separate props and have the item component call one with the other.
  2. use PureComponent/shouldComponentUpdate at the next level of components and keep the outer component cheap to re-render
  3. write a custom shouldComponentUpdate function that ignores changes in onItemPressed

@facebook facebook locked as resolved and limited conversation to collaborators Oct 10, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Oct 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

10 participants