facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.15k stars 24.32k forks source link

[ListView] renders all rows? #499

Closed luics closed 8 years ago

luics commented 9 years ago

There're 8 visible rows in screen after refreshing, but there're 15 rows found in chrome react tab.

Will you implement ListView with UITableView in future version?

Thank you.

colinramsay commented 9 years ago

I wonder if this is related to the default value of scrollRenderAheadDistance:

http://facebook.github.io/react-native/docs/listview.html#scrollrenderaheaddistance

It defaults to 2000 pixels unless you override it and with 15 rows it might not go above that and so will render everything when you scroll? This is just a guess but it might be worth tweaking the props for this component to see if you can affect this behaviour.

vjeux commented 9 years ago

React Native is using a different optimization strategy than iOS. Here's a summary

Load balancing

In UITableView, when an element comes on screen, you have to synchronously render it. This means that you've got less than 16ms to do it. If you don't, then you drop one or multiple frames. If you are rendering complex elements like newsfeed stories, it's basically impossible to meet this schedule so you're doomed to drop frames.

With ListView, when you reach the end of the current screen, you can prepare in advance more rows to be rendered. Those rows will be rendered in a different thread so won't freeze the UI thread while processing. The reason why it is working is that the load is not evenly spread. You don't need to render a new story on every single frame, most frames are just scrolling and don't need new stories to appear.

ListView will also render one element at a time, so if you are interacting with some element while rendering more rows, it won't block until all the rows have been pre-rendered, it will only block for one row.

Memory management

UITableView is very conservative memory-wise, it aggressively reuses cells. This decision was made back in the iPhone 1 where memory was extremely scarce. The problem with this is that reusing cell is extremely error prone for the developer. You are given a dirty object, from which you have no idea what mutations happened, and you need to reconfigure it to look like what you want. In our iOS app, this caused SOOO many bugs.

The problem of reusing cell is that some cells have internal state (video player running, text input, horizontal scroll position...) When you reuse them, you need to be able to serialize that state and put it back. This is not always possible nor easy, so you usually either loose this state or it propagates on the new row and causes bugs.

What we found out on React Native is that it is fast enough on iphone 4s to create new cells for every single row. So, we don't need to impose this very hard constraint on ourself. In your screenshot, you noticed that we don't remove rows after you scrolled for a while. That's not entirely correct, we don't remove the virtual dom representation on the React side (what you see in the chrome dev tools), but we do remove those elements from the "dom" and keep their reference.

When they are visible again, we put them back on the dom. In case we have low memory or the list is too big, we may destroy those and recreate them from scratch (loosing the state as mentioned above) in the future. We haven't done this performance optimization yet, but the user code wouldn't be impacted.

We tried to delete the iOS views aggressively but we found out that doing so was actually very expensive. It was better to leave them hanging than to remove them.

Change Detection

In ListView, we have a DataSource object that favors immutability. If you have a list of 1000 elements to render, you want to make those 1000 elements immutable, meaning that you can check the previous one === the next one and instantly know if something changed. This way, when anything change, the only thing you've got to do is to traverse those two lists and do those very fast equality checks and know what rows changed. And then update only those.

Layout

In UITableView, you've got to specify the layout of every single row even when they are not being displayed on screen. So, in cases where it's not a fixed size, you've got to basically render the element to know its size, and pay that high cost up front. It's also very annoying to do so manually.

In ListView, since React Native owns the layout system, you don't need to do all that painstaking manual computation yourself. When a row is rendered, it'll update the size. The only downside is that the scrollbar is a little funky, but I'm sure we'll be able to come up with heuristics to smooth it out in the future.

luics commented 9 years ago

@vjeux thx, it's really clear and useful.

drkibitz commented 9 years ago

Something doesn't seem to be working as intended just yet. After scrolling a very long list view with images, and 4 rows visible in vertical ipad air 2, everything seems to be working as @vjeux describes, except the memory just keeps climbing and climbing. And the most alarming thing is that scrolling (after scrolling to the bottom), can cause main thread to easily eat over 100-130% of cpu, while the JSCore thread stays quiet. This is very different to the 1-3% taken by just JSCore when scrolling from the top after the initial render. It is very apparent that the app is unstable while the listview's rows all remain in memory, and the main thread is blocked for about a second give or take while a listview in this state is unmounted, and can even result in a crash. Just a reminder, this is all on an iPad Air 2.

I also tried the "experimental feature" of removeClippedSubviews for ListView. This initially did seem to help with memory, but it was causing a crash. I assumed this was the "experimental" part, and haven't dug in there just yet ;)

What I'm wondering is if I am just missing something? Do I have the capability to remove images myself using onChangeVisibleRows, should I consider this? Would it be worth more getting removeClippedSubviews stable, or should I start contemplating a new version of list view with recycling?

ide commented 9 years ago

Does Instruments provide any insight into what's leaking? (specifically wondering if the problem is in JS or Obj-C)

drkibitz commented 9 years ago

@ide I'm a noob at instruments profiler... So here's brief summary from me taking a look at it.

cpu profile looks like most of the time is spent here (recursing through subviews), in RCTView.m :

- (void)react_updateClippedSubviewsWithClipRect:(CGRect)clipRect relativeToView:(UIView *)clipView

In memory profile major causes of persisted memory (645 MB total) are:

Unmount the ListView component, and total persisted memory drops to 74 MB total:

I have to think I'm doing something wrong, or maybe the ListView just isn't prepared for what I'm throwing at it.

drkibitz commented 9 years ago

Those profiles were from attaching to iPad Air simulator.

vjeux commented 9 years ago

cc @bryceredd who's been investigating React Native performance

vjeux commented 9 years ago

Closing this since it's not very actionable.

drkibitz commented 9 years ago

@vjeux What would make this actionable? I asked a few questions in this thread that was answered by closing the issue? I've also still been digging through the innards of the ListView implementation, as well as the crash caused by removeClippedSubviews: true. The title "ListView renders all rows?" seems appropriate based on memory consumption no?

vjeux commented 9 years ago

Sorry, trying to clean up the hundred plus issues we had, I was a bit too quick on this one.

drkibitz commented 9 years ago

@vjeux No worries, and thanks ;)

What I've found so far regarding the removeClippedSubviews: true crash

I'm seeing it happen on line 217 of RCTScrollView:

  UIView *nextHeader = nextDockedIndex >= 0 ? contentView.subviews[nextDockedIndex] : nil;

When I change the data source to contain only one section (same amount of rows), this crash does not occur, and I can see that the views are being clipped.

In this case, the memory is lowered to about 1/2-2/3 what is was before, but still very high, and cpu is not spiking as it did on minimal scroll, also about 1/2-2/3 of what is was before.

Kureev commented 9 years ago

It's totally insane: my iPhone 5c crashes after 700 list items. If I'm going to write a chat - it's blocking for me.

Also I got a lot of "Cannot find single active touch"

sahrens commented 9 years ago

removeClippedSubviews is our current stopgap that makes the groups app work - can you guys try scrolling in that app and see what you think of the perf/stability?

We definitely need to fix crashes when you have removeClippedSubviews - I don't think we tested thoroughly with sticky headers (they aren't used in groups), so that might be the issue.

Note on OP - the react tab just shows the react elements in JS. Just because they show up there doesn't mean they are in the actual UIView hierarchy (and you should find they are not all in the UIView hierarchy if you enable removeClippedSubviews).

samfriend commented 9 years ago

my < ListView pagingEnabled={true} onEndReached={this.loadAnotherFiftyArticles} >

50 rows/pages of < Image / > < Title/ > < Description/ >

3rd Load append (total 150)

Received memory warning

Received memory warning

Received memory warning

Crash Physical iPhone 6 Plus

Also I got a lot of "Cannot find single active touch" time to time

leecade commented 9 years ago

:+1:

Iragne commented 9 years ago

Hi all i create the PR https://github.com/facebook/react-native/pull/1406 It's solve for me the issue if you have a better idea to solve it feel free

Thanks

ghost commented 9 years ago

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

sghiassy commented 9 years ago

I took a stab this.

In my perf testing I was seeing constant memory allocations using ListView as the user scrolls down long lists. This is obviously problematic for reasons that don't need to be explained.

In response, I created an NPM package react-native-sglistview: https://github.com/sghiassy/react-native-sglistview.

The package isn't where I want it yet - but its showing some process comparision

josebalius commented 9 years ago

@sghiassy This looks interesting, will def check it out.

alejomendoza commented 9 years ago

Can't find a way to make the ListView component work well pulling data from Firebase javascript client.

Here's what I'm trying to create:

Here's how I'm doing it:

So far I was able to display some of the rows, in the first call the promise returns 50 items but only 25 rows are displayed. I can't reach the bottom! If someone wants to help me it would be great because I've pulling my hair out to make this work. Thank you!

https://vid.me/SEYQ

Here's my code:

https://github.com/skyhitz/skyhitz-react-native-ios

paramaggarwal commented 9 years ago

I can vouch that @sghiassy's https://github.com/sghiassy/react-native-sglistview is a great solution at the moment. I've been using pretty much the same logic (in production) but never found time to pull it out into a separate library. Great work by Shaheen.

It completely destroys the views when they are out of screen and hence removed from memory. removeClippedSubviews will only help scrolling smoothness (GPU optimisation) - it is not a memory optimisation. Objects/views/images are still in memory and hence the memory will still climb.

paramaggarwal commented 9 years ago

@alejomendoza Try disabling onEndReached. You are basically fetching the same data again on reaching the end, so it overwrites all the row's it has rendered till now. So effectively when it reaches the end, it starts over.

You can add a console.log in renderRow and render to debug that it indeed starts rendering but then starts over on reaching near the end. The third parameter to renderRow is the rowID, which you expect to be able to climb upto 50.

alejomendoza commented 9 years ago

thanks @paramaggarwal ! I debugged it and I get all the items but only 9 are rendered, saw this through adding console.log in renderRow, when I scroll to the bottom it renders the 50 items but it wouldn't let me scroll further down. Here is the video: https://vid.me/tJum https://github.com/skyhitz/skyhitz-react-native-ios

josebalius commented 9 years ago

@alejomendoza I have seen your issue before it has to do with the container views holding your views not having flex: 1 I'm not home right now but later I can provide you with an example so you see what I mean. In the mean time try adding flex: 1 to all views wrapping your listview

alejomendoza commented 9 years ago

thanks @josebalius ! I tried adding flex:1 to all container views and it worked!

vjeux commented 9 years ago

Here's a version that unmounts rows when they are out of the screen and more context from @sahrens

listview

jaygarcia commented 9 years ago

This type of "buffered" pattern can only mean very good things for the community! Absolutely awesome job, @sahrens!!

paramaggarwal commented 9 years ago

Thank you so much @vjeux. That's a really nice diff and great comments from @sahrens.

While reading I realised, that we as a community think about a solution by building something on top of ListView itself. I never considered modifying the ListView implementation itself like you do above. Nice!

@sahrens The removeClippedSubviews implementation you talk about is different from the one in the OSS RN repo right? The one in the OSS repo simply removes from view hierarchy and still will continue to eat up memory. The optimisation by @nicklockwood is where it goes further and removed images and text from the memory too, right? (ref: #332) Looking forward to it in the OSS repo. Will be of much needed help.

Thanks you guys!

nicklockwood commented 9 years ago

@paramaggarwal the removeClippedSubviews feature is the same internally and in OSS, but the implementations are different. @a2 recently added the feature of unloading offscreen images and rendered text to OSS, which should hopefully help with memory.

michaelcontento commented 9 years ago

:+1: Would be nice to see those changes in OSS RN :)

nicklockwood commented 9 years ago

@michaelcontento they are included in v0.10

paramaggarwal commented 9 years ago

@nicklockwood Will check it out. @a2 that's a really smart solution to the problem!

Thank you so much! :)

All, here are the relevant commits:

  1. Image: https://github.com/facebook/react-native/commit/bb883bfb615cf6f36ee79eff07542d4538bfbf77
  2. Text: https://github.com/facebook/react-native/commit/3e21f39a77b926f02f197b6f1104d4f4c024aa88

I guess we can close this issue now.

ssssssssssss commented 9 years ago

Our app has two tabs of quite long list view, switching between tabs is getting much slower compared to previous versions. We haven't enable the "removeClippedSubviews+overflow:hidden" optimizations yet though.

admmasters commented 9 years ago

Having problems with an iTouch 5th gen and ListView gradually filling up with memory. Over 1000 rows, followed this thread but nothing is making a huge difference at the moment. Tried working with a native table view implementation and the app took ages to load the data. Seems more sensible to group the data in chunks and drill down at this stage - although I know that is basically just a workaround. When I've got more time - I'll have a look at this as well.

bakso commented 9 years ago

How would android-platform like?

mihirsoni commented 9 years ago

Hi @paramaggarwal @sahrens ,

As mentioned in above comments , I have implemented Listview , My application is kind of Offline Dictionary which has .sqllite DB with lage number of words.

I render 50 words each time with the offset on every scroll ends , even after setting up removeClippedSubviews to true , my RN application is eating up so much memory.

The below shows that it has not more than ~500 records in listview it is eating up huge memory.

screen shot 2015-10-22 at 11 37 34 pm

I also tried @sghiassy but it seems doesn't solve my problem.

Could you please provide some some help ?

kinhunt commented 8 years ago

tried all tricks mentioned, 300 rows crashes every time.

paramaggarwal commented 8 years ago

Does each of your rows have overflow: "hidden" which is needed to work along with removeClippedSubviews.

kinhunt commented 8 years ago

@paramaggarwal thanks and yes. It's added to each row. shouldComponentUpdate implemented. Using rowHasChanged as the doc suggested. Infinite scroll implemented.

Memory usage boost up to 500MB and crashes at about 800 rows.

PS. Each row contains a feed item with some text and images, similar to the fb groups app.

paramaggarwal commented 8 years ago

So removeClippedSubviews will help only if your rows contain images and text. If there is any other kind of view, it will not be automatically removed from memory. (All views in the row would be removed from the view hierarchy though.)

kinhunt commented 8 years ago

@paramaggarwal thank you. My row item contains only text and images. As you mentioned before, 'removeClippedSubviews will only help scrolling smoothness (GPU optimisation) - it is not a memory optimisation', I assume that the ListView will eventually crashes the APP as number of grows, with or without removeClippedSubviews. Is that right?

paramaggarwal commented 8 years ago

Since RN 0.10, removeClippedSubviews will help with memory also as images and text are actually remove from memory when they are taken out of the view hierarchy - which is what it does. Hence this particular memory issue for large number of rows is actually fixed since then for the majority of use cases.

kinhunt commented 8 years ago

@paramaggarwal thank you. I will do some more tests to see what's wrong with my code. Good day.

kinhunt commented 8 years ago

Hi @paramaggarwal , I am coming back with some test results with RN 0.14.2 :)

On my iPhone 6, the app will crash after the memory usage is over 500mb. The following code will crash the app if the number of rows grows to 1800 with removeClippedSubviews off, and 5200 with removeClippedSubviews on.

In another test with some images in the row, it crashes at 300 rows with removeClippedSubviews of, and 600 rows with removeClippedSubviews on.

Can I assume that this is the best ListView can do at this moment?

let React = require("react-native")

let {
    View,
    Text,
    ListView
    } = React

let itemsPerPage = 20

let ListViewTester = React.createClass({

    getInitialState: function () {
        this.data = []
        return {
            dataSource: new ListView.DataSource({
                rowHasChanged: (row1, row2) => row1 !== row2
            })
        };
    },

    componentDidMount: function () {
        for (let i = 0; i < itemsPerPage; i++) {
            this.data.push({
                index: this.data.length
            })
        }
        this.setState({
            dataSource: this.state.dataSource.cloneWithRows(this.data)
        })
    },

    _onEndReached: function () {
        for (let i = 0; i < itemsPerPage; i++) {
            this.data.push({
                index: this.data.length
            })
        }
        this.setState({
            dataSource: this.state.dataSource.cloneWithRows(this.data)
        })
    },
    _renderRow: function (row) {
        return (
            <View style={{height:44,overflow:'hidden'}}><Text>
                Item {row.index}
            </Text></View>
        )
    },

    render: function () {
        return (
            <View style={{flex:1}}>
                <ListView
                    removeClippedSubviews={true}
                    dataSource={this.state.dataSource}
                    onEndReachedThreshold={0}
                    onEndReached={this._onEndReached}
                    pageSize={10}
                    renderRow={this._renderRow}/>
            </View>
        )
    }
})

module.exports = ListViewTester
stefalda commented 8 years ago

Hi, I agree with @kinhunt: the current implementation is unable to deal with big amount of data, memory use increases and then crashes the app...

I've made the same test with NativeScript's ListView (using the same data) and memory was always around 100Mb and the app didn't crash...

Stefano

NickJorgensen commented 8 years ago

Hello

Just an observation...

When running @kinhunt 's sample I am noticing a large difference between the way memory is managed on actual hardware vs the simulator. I guess this is to be expected because the simulation is just that, a simulation, however this seems to be really drastic.

In this test case I am using @kinhunt 's example code, and am scrolling approximately 1000 rows each for each test. Memory values are from xcode's debug navigator panel.

Should this much of a difference be expected? I can also confirm this same behavior on another app I am developing.

Nick

paramaggarwal commented 8 years ago

Please report on https://productpains.com/product/react-native/

sahrens commented 8 years ago

I've been working on an experimental WindowedListView that should help with this problem. I'll try to get it out soon.

On Tuesday, December 1, 2015, Param Aggarwal notifications@github.com wrote:

Please report on https://productpains.com/product/react-native/

— Reply to this email directly or view it on GitHub https://github.com/facebook/react-native/issues/499#issuecomment-160994126 .

kinhunt commented 8 years ago

@sahrens expecting. how is it going?