overscanRowCount with a cell cache

[arbesfeld]

Hi! Thanks for your work on this great library.

I am trying to use a cell cache along with overscan row count, and seeing extra renders from some children components. Here is an example: https://plnkr.co/edit/ebIVXCgOjM4XAddErRGi?p=preview

If you scroll very small amounts, notice that rerendering is logged 30 times on each change. This is far less than the number of times that rowRenderer is called.

What I would expect is that the child component here is only re-rendered if necessary.

If I set overscaneRowCount to 0 I don't notice this behavior any more.

[bvaughn]

I don't really understand what you're saying here.

[arbesfeld]

Sorry about that- it's possible that I didn't save the updated example.

I am using the technique as described here: #448 to cache cells with the goal of avoiding unnecessary re-renders of rows.

However, this cache does not seem to prevent re-renders for "overscanned" rows. In the example above, the render method of MyClass is repeatedly called for overscanned rows.

[bvaughn]

and seeing extra renders from some children components

What do you consider to be an "extra render"?

If you scroll very small amounts, notice that rerendering is logged 30 times on each change. This is far less than the number of times that rowRenderer is called.

That's because you're caching, so you avoid re-rendering once a row has already been rendered, no?

What I would expect is that the child component here is only re-rendered if necessary.

What do you define as "necessary"? By default, react-virtualized does not permanently cache cells. This is because (I think) cells are often dynamic and that would be a confusing default. It does reduce the number of cells that are rendered though, and it temporarily caches during scrolling, all of which means...overall performance is good enough that the lack of a permanent-cache isn't usually a problem.

If I set overscaneRowCount to 0 I don't notice this behavior any more.

I don't think you should ever set overscanRowCount to 30 like you're doing in this case. Usually 3-10 is a sufficient amount.

[bvaughn]

However, this cache does not seem to prevent re-renders for "overscanned" rows. In the example above, the render method of MyClass is repeatedly called for overscanned rows.

The Plnkr you linked to is preventing a cell from being rendered more than once. For example, check out this fork (that actually logs the index, so you can tell what's going on): https://plnkr.co/edit/H4btaSacFA5Fa6ft76Tu?p=preview

[bvaughn]

I think what you were being confused by was due to the fact that you're overscanning by 30 in your example. This means that by default, 30 rows above and below. However when scrolling is in progress, react-virtualized shifts the overscanned rows in the direction being scrolled (so from 30 in each direction to 60 in the direction being scrolled).

I'd guess you were seeing that, not expecting/understanding what was happening, and thinking it wa over-rendering.

But yeah, 30 rows overscanned is mega overkill. :) I'd recommend 5 or 10 instead.

[bvaughn]

Check out these slides from my recent presentation about react-virtualized:
https://bvaughn.github.io/connect-tech-2016/#/22/0

Step through (right arrow key) a few and it visualizes how overscanning works. :)

I am pretty sure this issue is just a misunderstanding and so I'm going to close it. I'm happy to answer any follow-up questions you may have though!

[arbesfeld]

Thanks! I understand that explanation and the way overscanRowCount works, but I am still confused by the behavior in https://plnkr.co/edit/ebIVXCgOjM4XAddErRGi?p=preview. I would have expected that <MyClass> not be rerendered more than once after the first overscan, since those rendered cells would be stored in the cell cache.

To provide an example, if the library caches rows 0-60 initially and then I scroll to 1-61, I would only expect a single new render.

[arbesfeld]

To further clarify, if a user scroll from index 0 to index 1 and stops, I would expect that rows -15 to 31 should be cached. Then, if they scroll to index 2, we should only need to call render on row 32. I might just be very confused here...

[bvaughn]

Ah, I understand.

react-virtualized caches the creation of the cell (eg when your rowRenderer function creates and returns a React element, react-virtualized caches that returned element and later uses the same one). But React still manages the decision of when your element gets rendered (if you're returning a custom component at least).

Put another way, react-virtualized isn't converting your component to an HTML string and caching that- that wouldn't be very React-like. :)

So react-virtualized's caching doesn't prevent your component from re-rendering, but it does prevent unnecessarily re-instantiating your component many times.

[arbesfeld]

Ah I understand - thanks for explaining this :)

Is there a suggested approach for caching the entire React tree of a row? In my case, the full render of a row can be expensive and I'd be willing to trade off memory for avoiding the extra computation, especially since overscanRow will call the render() method overscanRow/2 times after the user stops scrolling.

[bvaughn]

You should definitely be using shallowCompare or extending React.PureComponent to cut down on unnecessary, expensive renders. :)

[arbesfeld]

Definitely :) The strange thing is that even with pure components, these children are still getting rerendered. I added a shouldComponentUpdate() { return false; } to the above example (https://plnkr.co/edit/ebIVXCgOjM4XAddErRGi?p=preview) and it seems like MyClass is getting reconstructed for every new row.

If I understand this correctly: https://facebook.github.io/react/docs/reconciliation.html these components should not be re-created at all, right?

[bvaughn]

Ah, I see what you mean.

Looking at the stack, _constructComponentWithoutOwner is instantiating MyComponent when a previously-rendered row gets re-added to the list (when a row is unmounted, before it gets re-mounted, React instantiates a new instance.) I assume this is being done so that any single instance is only mounted (componentDidMount) and unmounted (componentWillUnmount) once.

Maybe this (recreating vs reusing) is to avoid confusion or potential sources of errors, or maybe it is because of some technical constraint. I need to make time to read through the underlying React source. Admittedly, this is not an area I know much about.

Anyway, this suggests that it's probably not a good idea to try to permanently cache rows like this after all. Maybe a better approach would be to use the isScrolling flag to render a lightweight representation of your row when the user is scrolling (since you say they're expensive to render). That will prevent a lot of heavy work being done while a scroll is in progress.

[bvaughn]

Relevant docs. Useful walkthrough here.

Only components declared as classes have instances, and you never create them directly: React does that for you. While mechanisms for a parent component instance to access a child component instance exist, they are only used for imperative actions (such as setting focus on a field), and should generally be avoided.

React takes care of creating an instance for every class component, so you can write components in an object-oriented way with methods and local state, but other than that, instances are not very important in the React’s programming model and are managed by React itself.

Returning JSX from a render method is a way of creating a React element. RV caches those (the elements) to avoid re-running potentially-expensive render functions. RV doesn't cache the component instance though. During the reconciliation phase, React creates the component instances. (There's no way for me to cache them; they're one level removed. I suppose I could use the dangerously-set-html method but that would break lots of things.)

[arbesfeld]

I see - that makes a lot of sense.

Unless I'm misunderstanding, it seems like a potential fix here could be to change the behavior of overscanRowCount? If it didn't change the selection of overscanned rows upon scroll stop, then we wouldn't be seeing the extra overscanRowCount/2 creation of component instances upon scroll end?

[bvaughn]

Interesting suggestion. So you are suggesting that once rows shift ahead (or behind), they would stay that way unless the user started scrolling in the opposite direction? I wonder, is it really that expensive to instantiate your components (given that RV is only creating a few at a time)?

[arbesfeld]

Yep exactly- basically any behavior that doesn't suddenly change the active set of rendered rows would work for this use case. Though I appreciate the elegance of the balancing approach, it would be more suitable for my use case if RV always rendered overscanRowCount rows above and below the visible set.

In my use case, a single row render takes ~5ms - this means that we block the main thread rendering loop for overscanRowCount/2 * 5ms when the user does any scroll stop behavior.

[bvaughn]

it would be more suitable for my use case if RV always rendered overscanRowCount rows above and below the visible set.

What if it only ever did the predictive shift. Let's say overscan is 5. By default, RV renders 5 rows below (0 above) because users are most likely to start by scrolling down. If/when a user starts scrolling up, it shifts to render 5 above (0 below).

In other words, it never renders above and below, and it never shifts/changes after scrolling stops.

Thoughts?

[bvaughn]

See PR #478 for discussion purposes.

Would you like a build of this to test? I'd love to hear if it resolves your performance concerns.

[bvaughn]

8.5.3 has been released. Hopefully this will address your overscan concerns.

[arbesfeld]

This is great!! Thanks again for your work on the library.