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 Issue #33

Closed
narychen opened this issue Jul 29, 2017 · 14 comments
Closed

Performance Issue #33

narychen opened this issue Jul 29, 2017 · 14 comments

Comments

@narychen
Copy link

I read the code and found the row of ListView is just used as masonry column.
If there are three column on your masonry there will be only three rows in the ListView.
The ListView or ScrollView will remove the views of images out of vision and keep the memory low.
But here it has only limited rows of columns.
As the image number grows your column view will become bigger and bigger and at last it will crash the whole app.

@brh55
Copy link
Owner

brh55 commented Jul 29, 2017

Hey @narychen great timing on this issue as I'm working on v0.3.0. So I see what your saying and I definitely agree you. I've been thinking about how to improve upon this for v0.3.0 with the use of FlatList and it would be nice to get your thoughts a new proposed implementation, and if you have any other opinions on the implementation.

My current proposed implementation is the use of multiple <FlatList>s to represent the columns, the primary downside is a blank screen because of the async offscreen rendering.

@narychen
Copy link
Author

narychen commented Jul 30, 2017

Multiple FlatList each will have its own scrollView. If you have three columns you will be able to scroll on three columns. How will you solve this?

@srameshr
Copy link
Contributor

If you have three columns you will be able to scroll on three columns Definitely don't want that.

@narychen
Copy link
Author

As from the react-native document 'removeClippedSubView is not belong only to ListView or ScrollView but the basic View.
Here is the document from the View

removeClippedSubviews bool 

This is a special performance property exposed by RCTView and is useful for scrolling content when there are many subviews, most of which are offscreen. For this property to be effective, it must be applied to a view that contains many subviews that extend outside its bound. The subviews must also have overflow: hidden, as should the containing view (or one of its superviews).

So we may build the View contains the subviews with overflow:hidden and removeClippedSubviews set to true. It may be work for the performance issue.
I don't have time to experiment this. Will @brh55 the creator of this project be so kind to have a try ?

@brh55
Copy link
Owner

brh55 commented Aug 16, 2017

@narychen
Per suggestion with removeClippedSubviews + overflow: hidden, it was still loading all of the bricks on initial loading. This is due to removeClippedSubviews lazily loading rows, but as the problem with react-native-masonry, "rows" are intentionally designed as columns, this allows different dimensions.
In Example

ROW 1,   ROW 2
| brick |  brick  |
| brick |  brick  |

So I'm now that I'm using FlatList oppose to a typical View. I'm able to get only a subset of the rows rendered. In my demo run, I was loading over +1000 images, and the top number indicates only 50 components were loaded on screen (including additional misc components). -- Also RAM usage dropped from ~225 -> ~75
image.

Fortunately there isn't any scrolling issues because ListView render rows into a parent ScrollView by default. But with further experimentation, I can probably remove ListView entirely.

Please test and try out the implementation on v0.3.0 branch, and let me know of any feedback.
@srameshr @narychen

@srameshr
Copy link
Contributor

@brh55 I will give it a try. And also infinite loading and maintaining a source order, as listed here:
#30
#32

@kmcgill88
Copy link
Collaborator

@brh55 How is progress on this?

@brh55
Copy link
Owner

brh55 commented Sep 5, 2017

@kmcgill88 needs to some additional user testing, but implemented on v0.3.0 branch

@kmcgill88
Copy link
Collaborator

@brh55 I can give it a go and let ya know.

@kmcgill88
Copy link
Collaborator

kmcgill88 commented Sep 6, 2017

I think you should open a PR for this branch. Might be easier to discuss.
screen shot 2017-09-05 at 10 52 26 pm

I think you are missing this on the flat list, https://facebook.github.io/react-native/docs/flatlist.html#keyextractor

EDIT: Looks like you do have it,... hmmm

@brh55
Copy link
Owner

brh55 commented Sep 6, 2017

@kmcgill88 Yeah I realize this as I was going through the code just now. Might have gotten carried away working on resolving local asset support.

@srameshr
Copy link
Contributor

@brh55 Will the FlatList merge solve this #41?

@brh55 brh55 mentioned this issue Sep 15, 2017
@brh55
Copy link
Owner

brh55 commented Sep 15, 2017

Performance updates now merged into the latest release (^0.4.0) of react-native-masonry 👍, thanks @narychen !

@brh55 brh55 closed this as completed Sep 15, 2017
@narychen
Copy link
Author

@brh55 You are welcome.

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

No branches or pull requests

4 participants