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

Performance implications with Carousel as an indirect child of ScrollView #247

Closed
0xpatrickdev opened this issue Jan 19, 2018 · 15 comments
Closed
Labels

Comments

@0xpatrickdev
Copy link

Is this a bug report or a feature request?

General question

Have you read the guidelines regarding bug report?

Yes

Have you read the documentation in its entirety?

Yes

Have you made sure that your issue hasn't already been reported/solved?

Yes

Is the bug specific to iOS or Android? Or can it be reproduced on both platforms?

Both

Is the bug reproductible in a production environment (not a debug one)?

N/A

Have you been able to reproduce the bug in the provided example?

N/A

Environment

Environment:
React: 16.0.0
React native: 0.50
react-native-snap-carousel: 3.5.0

Target Platform:
Android (8.1)
iOS (11.2.2)

Steps to Reproduce

In the Performance Tips section of the docs there's a note that says:

  1. Make sure the carousel isn't a child of a ScrollView (this includes FlatList, VirtualizedList and many plugins). Apparently, it would render all child components, even those currently off-screen.

Does this mean that Carousel shouldn't be a direct child of ScrollView, or a child at all?

In your example implementation, I noticed that you had ScrollView --> View --> Carousel, so I'm curious if this is conforming to the performance tip, or ignoring it since it's a just an example with a few items.

As an aside, I'm really loving this component/library, so thanks for the great work ! I recently noticed some performance bottlenecks during the initial render, so I want to make sure I'm going about everything correctly. My current setup is a ScrollView with about 12 Carousel children (wrapped in View's), that each have 5-20 items. I have a feeling items off the screen are getting rendered, so I might switch the parent component from a ScrollView to a SectionList or a FlatList instead.

@bd-arc
Copy link
Contributor

bd-arc commented Jan 19, 2018

Hi @pcooney10,

Well, you are stepping into quite a nebulous world, to say the least ;-)

Everything I've learned about ScrollView/FlatList/ VirtualizedList was from experience and from peers issues. Unfortunately, there is no definitive word about anything regarding those components since they are really really buggy; we're in for some empirical research... Moreover, the regular regressions and fixes of React Native add yet another layer of complexity.

As you've understood, I can't say for sure that rendering a Carousel as a child of any of those component will trigger a performance issue, but I remember seeing a comment about that. To be honest, many use cases require providing some scrolling mechanism anyway.

You're right by the way: I've deliberately ignored my own advice in the example since it was featuring only a small set of items :-)

One final thought: I honestly don't know if switching from a ScrollView to either a SectionList or a FlatList will improve things on your side since both extends... a ScrollView. But I'll definitely be interested in your findings!

@bd-arc bd-arc closed this as completed Jan 19, 2018
@0xpatrickdev
Copy link
Author

0xpatrickdev commented Jan 24, 2018

Hey @bd-arc,

Thank you for the quick reply.

I was able to put a quick bandaid on my app and increase performance by doing some of the pretty basic stuff. Namely:

  • make sure there aren't any excessive calls to this.setState in the component that renders the Carousel's and their parents
  • utilize shouldComponentUpdate / PureComponent for my Carousel items - I'm using shouldComponentUpdate since there's only prop that I know will change
  • properly leverage the initialNumToRender and maxToRenderPerBatch props inherited from FlatList, and windowSize inherited from VirtualizedList
  • leverage onEndReached & onEndReachedThreshold so I can keep my lists short & only add new data when the user actually wants it
  • utilize InteractionManager to render my Carousels that are "below the fold"
  • avoid using functions & object literals for props declared on components - this apparently results in "new props" during a re-render
  • use babel-plugin-transform-remove-console (documented here) to remove all console.log statements in production. This is not really related to this lib, but is good to know about / use.

I still plan to test nesting the Carousel's in a FlatList, but as you said it's quite a nebulous world, so most of my efforts have been focused on researching. Learning how to properly test performance in a RN app is also an animal in itself, but this article was very helpful.

My thoughts on why FlatList and SectionList extending ScrollView might be irrelevant are based on what's said in this article. FlatList and SectionList are extensions of VirtualizedList (which I realize is still an extension of ScrollView), and the VirtualizedList component claims to:

"virtualize" elements that are outside of the render window by completely unmounting them from the component hierarchy and reclaiming the JS memory from the react components, along with the native memory from the shadow tree and the UI views.

To aid in my testing efforts, I have a question that you might be able to answer - does a component that's currently "Virtualized" fire the render() method? If so, it might be difficult to tell what's virtualized and what's not. If not, then this should be pretty easy to test.

Thanks again for the great repo !

@bd-arc bd-arc added the * label Jan 24, 2018
@bd-arc
Copy link
Contributor

bd-arc commented Jan 25, 2018

Hey @pcooney10,

Thanks for summarizing the common steps everyone can take to improve their components' performance. I've been meaning to add something like this to the doc; if you're ok with that, I think I'll use your list :-)

Regarding your question, I had the same one a while ago and tried to put it to the test. I remember testing it with two different versions of React Native. In the first one, the render() method was fired for every component which, of course, was just pure horror. In the second one, and if I recall properly, it was handled as it was supposed to. Unfortunately, I don't know the current state of the VirtualizedList.

Note that I'm currently considering migrating to this component instead - a ScrollView component with a memory management that makes sense and that is, hopefully, way less buggy. I need to run a good amount of tests before making any decision though.

@0xpatrickdev
Copy link
Author

0xpatrickdev commented Feb 2, 2018

Of course ! You notes in the README.md were a very good starting point for me so feel free.

The benefit of InteractionManager may be a little overstated in my post RE react-native-snap-carousel. I did see much better Map loading when I used this API for my Map Markers (png images), but I am not sure the benefit was as great in my ScrollView.

Outside of seeing if render() fires for elements off the screen, how do you personally test performance? I linked to an article that details how to use the Chrome Dev Tools, but my "Bottom Up" view of my components didn't give as much detail on my underlying functions as was illustrated in the article. This approach is also not a perfect solution since the app must be run in Dev Mode...

Here are some screenshots:
screen shot 2018-01-21 at 10 43 38 pm
screen shot 2018-01-21 at 10 41 45 pm

@bd-arc
Copy link
Contributor

bd-arc commented Feb 2, 2018

So far, the React doc was my reference on this matter. This means I've been using the Chrome Dev Tools, just like you.

But I've just re-read the "Performance" part of the RN doc, and my conclusion is that I should definitely read it more often. The "Profiling" section contains a lot of insights and introduces very useful tools.

I need to try this all!

@0xpatrickdev
Copy link
Author

Hopefully will have some bandwidth over the next 2 weeks to dig into this more! Using Instruments for iOS and systrace for Android doesn't seem like it'll be fun, but I guess you gotta do what you gotta do.

Also, I had implemented this suggestion (babel-plugin-transform-remove-console), and will edit my post above to include it (even though it's not really related to this lib).

@jarecsni
Copy link

I have 500 items and despite using the properties initialNumToRender and maxToRenderPerBatch (10 and 20 respectively) the carousel still renders all the items beforehand, which results in very slow performance. Not sure what I'm missing?

@bd-arc
Copy link
Contributor

bd-arc commented Feb 16, 2018

@jarecsni Have you made sure to implement every trick described here?

If the answer is "yes", can you provide a Snack example that reproduces the issue?

@jarecsni
Copy link

@bd-arc apologies for slow response I was on hols.

I had a look at the tricks but I either already comply or the trick would not solve the issue. Once again my key problem is that all 500 cards are materialised (rendered) upfront. Can you point me toward an example where the carousel is configured to render only a few items at a time? Thanks!

@bd-arc
Copy link
Contributor

bd-arc commented Mar 2, 2018

@jarecsni Unfortunately, this clearly looks like a FlatList issue. By default, the carousel uses this RN built-in component to benefit from a bunch of performance optimizations. Unless you've set prop useScrollView to true or used one of the custom layouts, the carousel will render a FlatList component (with its many flaws).

You may want to try this Snack example from #235; it shows that items are, indeed, not all rendered upfront (which is an issue in this use case).

@jarecsni
Copy link

jarecsni commented Mar 7, 2018

@bd-arc Thanks for the Snack example, I have checked it and made some interesting discoveries.

  1. The performance problems I was talking about ONLY impacting the layout='stack' mode. So when I delete that property in my project, the 500 cards load INSTANTLY

  2. There was another problem I haven't mentioned so far. I was quite puzzled as I noticed that the cards are not registering clicks all over the surface. It seemed as though particular areas of the card were ignoring clicks (and therefore dragging would not work). Again, switching to default layout solves this problem too.

Do you want me to raise an issue with these details?

@jarecsni
Copy link

jarecsni commented Mar 7, 2018

Ok just seen an issue raised about the 2nd problem. However I think it only talks about swipes, whereas clicks events in general are missed.

@bd-arc
Copy link
Contributor

bd-arc commented Mar 7, 2018

@jarecsni The root of the missing swipe/click events is the same: it has to do with the custom zIndex needed to display advanced layouts. Unfortunately, I haven't been able to understand the exact why yet...

Regarding carousel performance, and as stated in prop layout's description:

WARNING: setting this prop to either 'stack' or 'tinder' will activate useScrollView to prevent rendering bugs with FlatList. Therefore, those layouts will probably not be suited if you have a large data set.

On a second thought, it would probably be a good idea to display this warning on the main page.

@jarecsni
Copy link

jarecsni commented Mar 7, 2018

OK thanks, that clarifies it. For me this means that I will need to go with default layout as the performance with deck is not good enough. Will keep an eye on the project for any fix you may do on this in the future. Thanks!

@Norfeldt
Copy link

Norfeldt commented May 16, 2018

It took me a long time to figure out that this was my root cause for the performance issues I see in my repo https://github.com/Norfeldt/LionFood_FrontEnd

Say I drop the FlatList and use View and some navigation instead to reach every section. Then I might run into some off-screen issues with children in the View - since I'm setting a lot of styles by pixels.

Is there any magic wand trick that you can recommend to me? - I don't have that many or deep decks of cards, but it's killing my app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants