-
Notifications
You must be signed in to change notification settings - Fork 72
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
Use explicit interval tree query for visibility test #10
Conversation
6b217e9
to
875d17e
Compare
Interval tree... very intriguing. I'll check out the PR tomorrow and get back to you.... thx for the PR! |
A fun weekend project, but I'm comparing this against the |
I originally tried using That was back in React Native |
Okay, this is not a very scientific test (should have profiled allocations), but I scrolled through an image feed for three minutes and here's what I have: ListView without removedClippedSubviewsMemory increase linearly. Scrolling is very choppy. ListView with removeClippedSubviewsUses SGListView with removeClippedSubviewsMemory increases but much less than without SGListView. I should have profiled allocations to see if it's the same as @sghiassy's test in the README. There's some flickering and stuttering because it's rerendering views but pretty smooth for realistic usage. SGListView with interval tree and removeClippedSubviewsBasically the same as with @sghiassy's implementation except it maybe solves the issue where cells are accidentally removed even when they're onscreen. |
I guess I conclude from this that the memory grown isn't ideal, but maybe it was the lack of |
Larger render-ahead distance indeed prevents seeing unrendered rows, but there'll always be swapping going on, so if your constraint is smoothness and memory growth is permissible, then ListView is the smoothest. |
While walking home I've realized that I've totally failed to handle re-layout correctly. All of the intervals need an update when any one changes. I can fix that. (Edit: need to check. But if things get an onLayout when they move, then this should be handled automagically. I'll confirm either way.) |
@rreusser in your tests did you ensure you added Using react-native 0.11.0 I don't see as explosive of memory growth doing that as that ensures Image/Text nodes are destroyed. |
@robertjpayne thanks for the suggestion/critique! I'll check this tomorrow. I added it to the rows inside the SGListViewCells but not to the SGListViewCells where it may need to go. The memory management and rendering is moderately opaque to me so I appreciate any suggestions! |
Interesting. The discrepancy might only be in the profiling tool. |
@rreusser - Thanks again for the PR. I really like the interval-tree approach. It's the right data-structure for the job and am committed to bringing it into the repo if you are, but the current PR breaks things. I didn't do a deep dive on what's breaking, but here's a debugger screenshot showing some early |
// yt1 yt2 | ||
// | ||
var scrollProperties = this.refs.nativeListView.scrollProperties | ||
var y1 = scrollProperties.offset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scrollProperties
doesn't have property offset
Perhaps something changed between 0.8.0
and current?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. That may indeed be the case. I'm using react-native 0.11.2.
It'll take a few minutes to create a new project with react native 0.8.0 and figure out what's breaking there, but I've committed code style and logic cleanup that should address the other issues. Yeah, strictly speaking since the cells are adjacent and non-overlapping, this could all be accomplished more 'easily' with an ordered list of cell locations and a binary search, but since the interval tree is already implemented, this approach has the advantage of simplifying the difficult part to like two or three lines of code. 😄 I should be able to give this a once-over for correctness and determine the 0.8 issue in the next day or two. |
Sorry I haven't made progress on this! We've been in the process of getting our thing out the door and |
So what's your thoughts on the PR? Want to close it out? Or is there a path forward? |
Does it fix the problem for #21 ? |
This is a pretty major change so I don't expect a simple merge, but I thought I'd share my approach. I don't know if the swapping approach is the best since it'd be great to handle this on the objective c/gpu/whatnot level, but until then, I was trying to improve the robustness of the visibility check since it's been a challenge to optimize both robustness and performance (i.e. a small preemptiveLoading number results in cells being hidden accidentally; a large number slows it down a bit).
It wasn't quite clear to me what the built-in check was doing, so I thought I'd try maintaining an interval tree explicitly. When a cell is laid out, it registers or updates its bounds in an interval tree. On each scroll event, the union of the previous visible range and current visible range is queried for intersecting cells in order to determine any visibility changes. I didn't like using onScroll, but a quick check indicates the check comes back under 1ms, so it shouldn't be a bottleneck at least.
It accepts a
preemptiveLoadDistance
that defines the distance in pixels outside the visible range at which cells become visible.Final question: I had a hard time following the subtleties of the discussion at facebook/react-native#499 . Is there an approach yet that makes all of this obsolete?