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 on renderItem #139

Closed
edo1493 opened this issue Sep 6, 2017 · 17 comments
Closed

Performance on renderItem #139

edo1493 opened this issue Sep 6, 2017 · 17 comments

Comments

@edo1493
Copy link

edo1493 commented Sep 6, 2017

I am noticing renderItem gets called every time position changes in my case. This happens on the whole set, so performance goes really down as I swipe through my list.

I was wondering if you guys were seeing this, which can probably be related to facebook/react-native#13597.

@bd-arc
Copy link
Contributor

bd-arc commented Sep 6, 2017

@edo1493 Wow, this is a real matter! Thanks for reporting in.

I'm afraid that this is out of my hands (if it really has to do with FlatList implementation), but I will run some tests to see if there is anything I can do about it.

@edo1493
Copy link
Author

edo1493 commented Sep 6, 2017

Yes, I was looking at your shallow comparison, which is missing my array of objects and always returning true in shouldUpdateComponent. However, that's not the main issue. I have re-written that method and I still see performance problems, but it seems they are from RN core.

I am concatenating new items at the end of the flat list, while I swipe through, so when I get to a couple of dozens, it's not smooth anymore.

@bd-arc
Copy link
Contributor

bd-arc commented Sep 6, 2017

I'm interested in your rewriting of shouldComponentUpdate()... ;-)

Does the issue remains if your convert the <Carousel /> to PureComponent?

@edo1493
Copy link
Author

edo1493 commented Sep 6, 2017

Well, I am just doing a comparison on this.props.data.length, when it changes, I return true, otherwise false. This, in theory, reduces the number of renderings. However, even in this case, I still have the FlatList issue.

I have tried to move move the animation on the native thread, setting a windowSize, but I don't see any improvements. Nothing changes with PureComponent, I also tried to wrap Carousel around CustomCarousel and setting it to Pure.

I have opened an issue here just to ask you guys if you have seen something like this. I have noticed this same exact behaviour on other FlatLists I am currently using.

@bd-arc
Copy link
Contributor

bd-arc commented Sep 6, 2017

I'll run a few tests as soon as I can and I'll keep you posted. Unfortunately, this looks like a React Native issue...

@edo1493
Copy link
Author

edo1493 commented Sep 6, 2017

They say it's okay renderItem gets called every time, not sure how this impacts this component. It's getting really slow and bumping my ram. This is my configuration:

<Carousel
          ref={(carousel) => { this._carousel = carousel; }}
          data={data}
          renderItem={this.renderTotals}
          sliderWidth={width}
          itemWidth={width / 2}
          inverted={true}
          decelerationRate="fast"
          windowSize={5}
          firstItem={firstItem}
          onSnapToItem={this.onPositionChange}
          onEndReachedThreshold={0.8}
          scrollEndDragDebounceValue={0}
          onEndReached={this.onEndReached}
          useNativeOnScroll={true}
          keyExtractor={(item) => item.from} /> 

@edo1493
Copy link
Author

edo1493 commented Sep 6, 2017

As I keep scrolling down and getting more rows, the ram goes up like crazy. :(

@bd-arc
Copy link
Contributor

bd-arc commented Sep 6, 2017

Do you have the same issue with your other FlatList components?

@edo1493
Copy link
Author

edo1493 commented Sep 6, 2017

Not this bad. I have moved the animation to the native thread and set inactiveSlideScale={1} and inactiveSlideOpacity={1}. It looks better, but with large lists doesn't look good.

It gets to the point it misses onPress events on the items.

@bd-arc
Copy link
Contributor

bd-arc commented Sep 6, 2017

Have you tested on both Android and iOS? Have you tried to build a production version?

@edo1493
Copy link
Author

edo1493 commented Sep 6, 2017

I am on the iOS simulator. I have tried on production too. What I see here is that with 30 elements, I start getting: "VirtualizedList: You have a large list that is slow to update - make sure your renderItem function renders components that follow React performance best practices like PureComponent, shouldComponentUpdate, etc. ", which is quite ugly.

I have removed the scale and opacity because I was getting delay on those animations too. It gets to the point where the element has a delay in order to align itself and so onSnapToItem returns after a while.

The other thing I have noticed is that if I swipe fast and skip some elements, the carousel has a hard time figuring out where to stop and sometimes goes back.

@bd-arc
Copy link
Contributor

bd-arc commented Sep 6, 2017

That's really annoying since we've recently migrated to FlatList instead of ScrollView in order to be able to handle many items without impairing performance...

Still, can you try the latest commit on master and see if it changes anything? It features a new animation mechanism as well as a few optimizations. Also, setting removeClippedSubviews to either true or false might help.

Regarding the fact that the carousel goes back, you can thank the many ScrollView limitations. We are doing our best to circumvent them and regularly improve the carousel, but some edge cases are really difficult to handle (at least half the plugin code is just pure hack for what should have been implemented by React Native)...

@edo1493
Copy link
Author

edo1493 commented Sep 7, 2017

I didn't see much difference. I am just trying to load a set of balances horizontally as you can see from the pic. Each balance has an onPress which is supposed to call snapTo. After a couple of dozens of elements, I can't even click on the element, it gets sort of stuck.

screen shot 2017-09-07 at 09 13 10

RemoveSubclippedSubViews seems to help a bit, but I still get the warning in the console about the poor performance of the VirtualizedList.

Can you reproduce? What I haven't tried is to just pass a fixed list of 50 elements.

@edo1493
Copy link
Author

edo1493 commented Sep 7, 2017

Another thing I have noticed is when I set deceleration "fast", the component has even more issues; because it stops in the middle of two elements and has a much bigger delay in focusing.

@edo1493
Copy link
Author

edo1493 commented Sep 8, 2017

Btw, I am playing with your component and also tried to work out a better shouldComponentUpdate for my scenario with deepEqual, instead of shallow. However, what I am seeing is an actual delay on scrollToIndex from the FlatList component.

This happens with 20 elements, your method gets triggered, but then the list takes a while before moving.

@edo1493
Copy link
Author

edo1493 commented Sep 8, 2017

I am closing this. After digging, I have realised redux-undo was creating performance issues with this component transitions. This was a really weird discovery, since no one of the props involved here were anything to do with one of my reducers that was implementing undo.

@edo1493 edo1493 closed this as completed Sep 8, 2017
@bd-arc
Copy link
Contributor

bd-arc commented Sep 8, 2017

@edo1493 Ok, thanks the detailed feedback. This has been very useful anyway ;-)

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

2 participants