Optimization idea

[igorbt]

@bvaughn, I did an optimization in my project that probably is best to be done directly in the library. I'm not sure how to properly implement it, but probably you would know better.

The basic idea is simple, a virtualized grid may not re-render itself at every scroll, because it has (pretty big) buffers above and below visible content. So, unless a specific treshold is hit, it can just ignore scroll events.

Here is how I did in my app (that 1000 is a kind of a dummy threshold that was good for me):

        <WindowScroller>
            {({ height, scrollTop }) => {
                let scrollT = scrollTop;
                if (this.lastScrollTop !== null && Math.abs(this.lastScrollTop - scrollT) < 1000) {
                    scrollT = this.lastScrollTop;
                } else {
                    this.lastScrollTop = scrollT;
                }
                return (
                    <VirtualScroll
                       {...props}
                        scrollTop={scrollT}
                    />
                );
            }}
        </WindowScroller>

One possibility would be to do a HOC for this "lazy scroll update" behavior, but I don't see any problem also to be implemented directly in Grid, just by carefully updating state.scrollTop based on props.scrollTop and taking into consideration the laziness described above. In Grid, threshold could be reliable calculated based on the buffers, that would be an advantage.

What do you think?

[bvaughn]

Hm. This is interesting.

I've tried a similar thing at a lower level a couple of times in the past. The basic idea was for Grid to track its most recently rendered range of cells and only re-render if the list changed. I didn't notice any perf boosts from that though, and it was fairly complicated (since a lot of things can change and invalidate the most recently-rendered cells.)

You're suggesting to try taking it one step further I guess, and only try and only re-render if we run out of overscanned cells too. Hm. It's an interesting suggestion.

It may improve perf but I wonder how much it would complicate things...

[igorbt]

I'm working on a hybrid app. In my case, this optimization made a pretty big difference on a low/mid-end mobile device. I know that things can go complicated, but I see performance as a feature. I implemented at work a custom virtual scrolling for Angular and it was really hard to make is smooth given the slow ng-repeat, if you know what I'm taking about. With React we are tempted to let it do all the optimization and that's fine for a custom app. However, I think a library should be as performant as it could be, because you never know in what scenarios it will be used.

[bvaughn]

Sure. Performance is a feature. No disagreement here. (I've put a lot of effort into making this lib performant too.) I was just thinking out loud about how complicated it might be to try implementing this in Grid because complication often leads to bugs (or at least increased maintenance load) etc. :)

I don't have any extra bandwidth for the next week. Let's leave this issue open though and keep chatting about it. I like the idea. I want to benchmark it. :)

[bvaughn]

It may seem counter intuitive, but this idea doesn't seem to improve performance (at least not base on my local testing). I've tried a couple of ways of implementing it (using min/max x/y of most recently rendered rows, using min/max row/col indices, etc). It actually causes jumps at the boundaries between "chunks" of rows which seems to hit the framerate pretty hard.

[bvaughn]

Hey @igorbt,

I'm going to close this issue for now as initial tests show no improvement for me. If you can provide a test project / benchmark that I can run though that you see improvements on - I'll be happy to take another look! I'll keep my branch around.

Closing the issue in the meanwhile because I don't have any immediate plans for action based on initial tests (and it helps me when I keep the issues list less cluttered).

[igorbt]

@bvaughn thanks for trying it at least. I'm not sure I can provide a test / project / benchmark to prove the idea. It is working for me probably because i use a pretty heavy grid, with each item containing images wrapped in components that take care of progressive loading, SVG's etc. More than that, it may be more visible on low-end computers or on phones. And, even if it delivers same or similar FPS, not-optimized version eats a lot of processor (I can hear my fans working hard) even if I scroll up and down in the already rendered area. And again, IMHO a library should be as optimized as it can get and live the resources free for the application that uses it.

[bvaughn]

And again, IMHO a library should be as optimized as it can get and live the resources free for the application that uses it.

Hey @igorbt,

You've said this phrase (or similar) a couple of times times. I almost think you don't believe I agree with it? Or that I'm somehow not acting in accordance to it?

Of course I agree with you that performance is an important feature of a library. That's the entire reason this library exists. And if you look through the issues and commit log you'll see that I spend a lot of time iterating and tuning performance. The very fact that I tested this suggestion a couple of ways in a branch indicates that I care about performance. (Why else would I spend time implementing and trying it?) :smile:

My findings were not consistent with yours. It didn't make things faster in my benchmarks- and it did introduce a noticeable visual lag when the chunks or rows were updated during fast scrolling. Performance testing a library like this is hard because the components are very flexible (eg you can used fixed row heights, varying row heights, just-in-time measure row heights, etc.). This variety is probably why your results don't match my results. That's why I asked a couple of times for your benchmark. If I can see what you're seeing then I can dig deeper into why it's happening. But without that all I can do is look at how it impacts my own benchmarks (and look at reports from others overy time).

FWIW, even if a specific benchmark is improved by a certain change- that doesn't always mean it's the right change. It may add additional complexity (which can have maintanence costs down the road) or it may improve one test but worsen another (I've seen this before). So it's not always an obvious decision. I have to try to make a judgement call.

I appreciate your time reporting this issue and suggesting changes. I always welcome suggestions to improve performance- even if I don't always implement them. Sometimes I re-open old issues as new information comes in and so I'd still love to see a benchmark if you're able to share one in the future. It's possible that new info could change my mind on this particular change. :)

Cheers~

[igorbt]

@bvaughn, in my head I was a bit surprised that you closed the issue so fast, this is why I was repeating myself on that phrase. I totally understand your point and it is very valid and it make sense. On the other hand, closed issues is something I rarely look at (and github thinks the same since the default filter is to not show closed ones). Usually, when there is already an open issue on the subject I'm interested in, I can contribute there with my ideas, opinions and even with code. I think this is how open source usually works. No offence, but I find this style of fast closing unresolved issues not the best one.

[bvaughn]

@igorbt Totally understandable.

FWIW I always try to leave clear messages about why I'm closing an issue and that I might re-open it if I find new information. (I've done that before actually!)

You may not be aware of this, but I'm the only maintainer of this library and it gets a decent amount of traffic (counting GH issues, Gitter messages, Stack Overflow questions, etc.) It's also a hobby project- so I can only work on it during my free time (nights and weekends). Because of that I have to basically process issues really fast. (Every morning when I wake up, I check Gitter+Github+Stack Overflow and try to process things, then every day when I get home from work I do the same. etc.) I have to do this because if I didn't- and I fell behind- I'm afraid I'd never catch up. I've seen other OSS maintainers do this and it leads to a feeling of hopelessness and burnout. So I try to combat that by keeping the issues list clean.

If it's actionable- I fix it. If it's something I don't currently plan to implement- I close it with a message explaining why (so as to hopefully not offend anyone).

People do browse closed issues FWIW. (I get comments on old issues somewhat regularly.) But I agree that it's not as visible as an open issue and it may result in duplicates. That's kind of a tradeoff that I make in order to reduce my own stress level when I look at the project every day I guess.

Hope you understand! Cheers.

[igorbt]

@bvaughn I know you are the creator and maintainer of this library and you do a great job and I'm actually very impressed about that. Please don't get me wrong, I was just giving you my friendly opinion about handling some issues like this one. But it's your decision how you want to work and I respect that.

[bvaughn]

No worries! I appreciate the kind words. :)

On Aug 24, 2016 6:02 PM, "igorbt" notifications@github.com wrote:

@bvaughn https://github.com/bvaughn I know you are the creator and maintainer of this library and you do a great job and I'm actually very impressed about that. Please don't get me wrong, I was just giving you my friendly opinion about handling some issues like this one. But it's your decision how you want to work and I respect that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bvaughn/react-virtualized/issues/350#issuecomment-242253482, or mute the thread https://github.com/notifications/unsubscribe-auth/AABznQj6B_M2qLbbLAbnk8g6c-0LAPPiks5qjOm2gaJpZM4JjuEZ .