React-virtualized: Performance issues with Grid.js and scrollTop check

[istarkov]

Hi thank you for this library, it's amazing.

I want to ask what the need of this check?

I have a component which in some situations animate scrollTop property of VirtualScroll component, and even in production I got 30-50ms per frame (I have a lot of lines on the screen).

It's because check causes reflow.
Just removing this line highly (4+ x times) increases performance of my component without lose of any functionality. I got stable 8ms per frame on scroll animation.

I've read your comments about autoHeight coupling, and what I want is another boolean property and I know that it's bad idea.

May be you have any ideas about how to remove this check? Or move all that checks outside control?

[bvaughn]

Safari scrolling does not work without the additional assignment statement on this line. Comments on these lines provide a little more detail. Edit 1: I just removed the line locally and tested, and Safari seems to work fairly well (with the exception of issue #160 which I have no known workaround for). I will do some more testing!

Good news is that once I get a little more time to experiment with ideas being discussed on issues #350 and #353 this should hopefully become a non-issue.

Edit 2: Initial smoke testing on Mac OS with Safari+Firefox+Chrome, iOS Safari, and Windows 7 IE 9, 10, and 11 suggest that this check _can_ be removed without any broken functionality. All unit tests pass as well. _However_ my local tests don't reveal any significant improvement in performance. I'm interested to see your benchmark!

Edit 3: Production test harness isn't showing any noticeable increase in performance but local (dev-mode) testing _feels_ a bit faster. (Power of suggestion? Or real? heh)

[bvaughn]

It may be possible to also remove this check _although_ I'm not sure. I've thought about it before and didn't see a way.

I don't think it always causes reflow though, for what it's worth- only in the case where I've changed something about the style already- within the current frame- before I query the scroll offset. (If the style hasn't changed then the browser does not need to reflow anything, only return the previous value.)

Also if the style doesn't change again _after_ a forced reflow, it still won't have much of a net impact (since a style change would have caused a reflow anyway).

This type of optimization is very tricky.

[bvaughn]

Do you, by chance, have a test harness or repro case demonstrating the 4x speed increase? I'd love to play with it a bit. (I have some tests as well but it's useful to see what others use for benchmarks.)

[bvaughn]

@istarkov Try the attached build. (Extract it and then npm i /path/to/react-virtualized-7.19.4.tgz).

react-virtualized-7.19.4.tgz.zip

PS Sorry for the tgz in a zip but Github doesn't allow uploading of tgz files for some reason

[bvaughn]

I've pushed branch 357 with this change. I'll wait to hear back from you regarding any perf difference you see with the branch (aka the above build).

I'd really like to get your benchmark for this as well so I can reproduce it locally.

[istarkov]

Hi, thank you,
looks like I can't provide a working test except visible by eyes:
Source

Please use arrow keys or space, shift-space to navigate:
Example builded with current react-virtualized
Example builded with react-virtualized you provide at 357

As you could see first example has fps issues,
the problem that looks like yesterday I was wrong about scroll frame performance is based on reflow (as looks like I looked in the wrong place) BTW issue exists, and 357 branch solves it somehow.

If you uncomment in this line (it is the scrollTop property of VirtualScroll)
so line becomes looking like ? Math.round(currentStyle.style.scrollTop) both examples above will start to work with the same speed.
IIF browser zoom is set to 100%,
on other zoom percentages first example will work much worse than second.

(_May be because of elt.scrollTop on zoom 100% always returns integer results,
but double values on other zoom levels_)

Also with Math.round change, second example (357) will start to draw all lines in between Hello world 5 and Marker dispatcher steps.

Also with Math.round there will be no situations when line highlighting will be after the move has completed.

[istarkov]

Looks like the issue with non rounded scrollTop is here as this if condition are always will be true on user animated scrollTop.

As event.target.scrollTop on 100% zoom are always returns integer, this integer with high probability is not equal to scrollTop in the state. (On other zoom level even it return a double that double is not the same that you set)

IMO this comparison should be changed, and looks like it should depend on zoom level.

But even such simple change

if (Math.abs(this.state.scrollLeft - scrollLeft) >= 1 || Math.abs(this.state.scrollTop - scrollTop) >= 1)

solves all my issues on 357 branch and even on master branch.

PS:
Having zoom level (not in percentages but 0.5 0.75 0.9 1 etc.) equation above should look like:

if (Math.abs(this.state.scrollLeft - scrollLeft) >= 1 / zoom || Math.abs(this.state.scrollTop - scrollTop) >= 1 / zoom)

The problem is to find zoom level in modern browsers
1) Chrome only: Math.round(((window.outerWidth) / window.innerWidth)*100) / 100

This library is outdated and seems not work for safari and firefox

PPS:
Looks like it's hard to find zoom detection for firefox, so must be another idea of scroll comparison based on component property or some hack .

PPPS:
2) In firefox scrollTop is independent of zoom level and always integer
3) In safari scrollTop is independent of zoom.
4) Have no IE to check.

Looks like just zoom = Math.round(((window.outerWidth) / window.innerWidth)*100) / 100 will be enough for all browsers. As it will give 1 for non chrome - and having independent scrollTop this will be right value and zoom for chrome.

[bvaughn]

I'm a bit confused. This issue now lists 3 changes that solve the FPS issue you're reporting. Which ones are necessary? Could you summarize or attach a DIFF of the changes you've made?

I don't really see an FPS issue with either of the links you posted above. The 2nd one does a weird jitter thing when I first start track-pad scrolling but Timeline view shows ~60fps for both of them.

[istarkov]

Issues seen when you press spacebar, or arrow keys.
The problem that timeline FPS does not show the reality, it's seen without timeline that first example works really slow. And FPS meter etc shows near the same results for both. But my eyes see the BIG difference. I asked a few guys - all see the difference without tooling.

The solution is to change this
https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/Grid.js#L863-L866

on this

if (
  Math.abs(this.state.scrollLeft - scrollLeft) >= 1 / zoom ||
  Math.abs(this.state.scrollTop - scrollTop) >= 1 / zoom
)

where

zoom = Math.round(((window.outerWidth) / window.innerWidth)*100) / 100

It works with / without your branch and improve examples performance.

[bvaughn]

The problem that timeline FPS does not show the reality, it's seen without timeline that first example works really slow. And FPS meter etc shows near the same results for both.

If the timeline says it isn't slow, then it's not actually slow. It may be jittery/stuttery because of a rounding issue- but that's different from being "slow" FPS. I just want to make sure I understand what we're talking about here because it's easier to test and track improvements to _measurable_ things (like FPS) than it is to subjective things (like "looks slow").

I'm hesitant to add a bunch of zoom calculations into the on-scroll handler because that handler gets fired _a lot_ and calculations add up. (Granted it's pretty small in the overall scheme of things...but still) When you the code you posted above "will be enough for all browsers" - which browsers did you test? Just OSX? Mobile? IE?

[istarkov]

I just played with the code on chrome, safari and firefox
BTW this issue is easily avoided on safari + firefox as both has scrollTop independent of zoom. And I could just pass rounded values.

But on chrome every time I change scrollTop position on zoom level other than 1, that equality will always return true.

So for that 3 browsers I could be sure that this

zoom = Math.round(((window.outerWidth) / window.innerWidth)*100) / 100

formula is good as it will always return 1 for non chrome browsers I checked, and zoom for chrome.

I have not ability now to check this on mobile or other browsers, and that is just a discussion about how to avoid not needed state changes which looks like FPS issue. (Haven't checked but i think it just set same scrollTop twice)

[bvaughn]

I'm swamped today so I don't really have time to look into this. I'll leave the issue open for now and come back to it. 😄

I'm not really comfortable with factoring zoom level into RV's calculations though. IMO zoom should be transparent to application or library code. (The fact that browser's don't expose a very consistent way of querying this seems to back this up.)

I'm also at a bit of a disadvantage here because- even running the code you linked, zooming in (eg 250% zoom level), and using spacebar and arrow keys to navigate- both lists scroll smoothly for me. The Timeline backs this up, showing ~60fps.

I'll look at it more soon. 😁

[bvaughn]

By the way, this app you linked to looks very cool. 😄

[istarkov]

Thank you, I wrote simple minimal example to show the problem:
source

It runs scrollTop simple animation on button Click.

Now If you add

console.log('B', this.state.scrollTop);

at this line

In my example I log scrollTop at render method here

If you run this example on Chrome and click button, at zoom=100% you will get the result:

example A

1     // example render
B1   // grid
2     // example render
B2   // grid
... and so on

But if you change zoom on 75% or so,
you will get somewhere in the log multiple times that internal scrollTop state at Grid is really different from render scrollTop.

example B

B 1.3333333333333333 // OOPS
...
7
B 5.333333333333333 // OOPS

Zoom hack ;-) will solve this issue and you will get the same results as in example A
(_I also think that it should be done somehow without zoom calculation_)

So looks like the problem is in race condition with this raf
https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/Grid.js#L741
which could override results from props.scrollTop

Having ability to detect somehow that scrollTop-scrollLeft prop here was changed by user (it's come from props) we should avoid to enter that if somehow. May be adding somewhere yet another one scrollPositionChangeReason

And I really see this spikes on my 3 year old mac, and it's looking the same as FPS issue.

[bvaughn]

Getting the chance to sit down and look into this a little. Sorry for the delay. :)

I'm still not convinced that this change is a good idea. For example, this scrolling precision issue is outside of react-virtualized (and even React) so I'm _very_ hesitant to try coding around it within Grid.

I can reproduce the behavior you're reporting with the following snippet:

<body>
  <div id="outer" style="height: 100px; overflow: auto;">
    <div style="height: 500px;"></div>
  </div>
</body>

const outer = document.getElementById('outer')
outer.addEventListener('scroll', (event) => console.log('scrollTop:', event.target.scrollTop))
outer.scrollTop = 50 // prints scrollTop: 49.333333333333336

Or you can see it here on Plunker: http://plnkr.co/edit/ln3Kg1JjNTaRwW98rz4A?p=preview

[bvaughn]

So looks like the problem is in race condition with this raf
https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/Grid.js#L741
which could override results from props.scrollTop

Seeing as how I can reproduce a similar behavior with a simple div (above) I don't think this has anything to do with the animation frame you linked to.

Having ability to detect somehow that scrollTop-scrollLeft prop here was changed by user (it's come from props) we should avoid to enter that if somehow. May be adding somewhere yet another one scrollPositionChangeReason

This is exactly what scrollPositionChangeReason is currently doing. The only time it's set to "requested" is when it's set as a result of an external property change. It's set to "observed" when it's changed as the result of a scroll event. (There's a bit of an exception in there with regard to Safari that I _think_ I can remove but I'm a little reluctant to).

And I really see this spikes on my 3 year old mac, and it's looking the same as FPS issue.

I have a suspicious that the stuttery scrolling you're experiencing is _actually_ the result of the fraction values for scrollTop but I guess I can't prove that. Can you try applying the following DIFF to your local copy of react-virtualized and see if it makes any difference?

--- node_modules/react-virtualized/dist/es/Grid/Grid.js
+++ node_modules/react-virtualized/dist/es/Grid/Grid.js
@@ -752,8 +752,8 @@ var Grid = function (_Component) {
       var scrollbarSize = this._scrollbarSize;
       var totalRowsHeight = this._rowSizeAndPositionManager.getTotalSize();
       var totalColumnsWidth = this._columnSizeAndPositionManager.getTotalSize();
-      var scrollLeft = Math.min(Math.max(0, totalColumnsWidth - width + scrollbarSize), event.target.scrollLeft);
-      var scrollTop = Math.min(Math.max(0, totalRowsHeight - height + scrollbarSize), event.target.scrollTop);
+      var scrollLeft = Math.round(Math.min(Math.max(0, totalColumnsWidth - width + scrollbarSize), event.target.scrollLeft));
+      var scrollTop = Math.round(Math.min(Math.max(0, totalRowsHeight - height + scrollbarSize), event.target.scrollTop));

       // Certain devices (like Apple touchpad) rapid-fire duplicate events.
       // Don't force a re-render if this is the case.
@@ -960,4 +960,4 @@ Grid.defaultProps = {
   style: {},
   tabIndex: 0
 };
-export default Grid;
\ No newline at end of file
+export default Grid;

_Edit_: I don't think rounding is a "solution" really as sometimes the values will round _down_ causing a mis-match. I was just wondering if the fractional offsets were the actual issue for what you were seeing and was hoping that rounding could help clarify that.

[bvaughn]

Potentially related issues:

• MacBook Pro Retina - Choppy Safari Scrolling while ZOOMED
• Really slow zoom and scroll (older)

[istarkov]

Hi, Thank you for the anser.
This patch does not solve the problem,
also it visually makes it worse than base library.

Having an example I provided, now it gives

...
3  // scrollTop  I provided
B 1 // this.state.scrollTop value below patch lines
4
B 0 // !!! It becomes worse
5
B 5
..
12
B 8 //!!!!
13
B 13
14
B 12 // !!!
// the same along all animation - it's not a double precision errors

For now I just added the simple sed patch ;-) at the prepublish section of package.json which solves my issue for now and for some period in the future ;-)

Also time you need to answer does not matter for me ;-). You could even stop to answer at all, and I will love this library ;-)

[bvaughn]

Aw 😄 That's kind of you to say. I appreciate it.

Hm. Interesting. I wish I could reproduce what you're seeing on my end. It would make it easier to tackle.

[bvaughn]

Interesting. 💡 Using Virtual box, and throttling the CPU & ram, I am able to reproduce the behavior you're describing.

However, applying the patch you mentioned above doesn't seem to improve or change the behavior for me.

[istarkov]

Few days ago I thought that I have one of the fastest computers in the world (core i7 bla bla) and it's just cpu throttled virtual box ;-)

[bvaughn]

Well your comment...

And I really see this spikes on my 3 year old mac, and it's looking the same as FPS issue.

...made me think that the biggest difference between your 3yo laptop and my 1yo Macbook is probably just CPU/Ram. 😅

[bvaughn]

Even now that I'm able to _reproduce_ the behavior- looking at Chrome's Timeline- it _still_ doesn't seem like it's FPS. (FPS looks great, even though I can see a bit of a hiccup/stutter toward the end of the scrolling animation.

[bvaughn]

More potentially related links:

• Chrome slow scrolling with fixed position elements
• Scrolling Smoothness

[bvaughn]

It looks like _if_ this happens on a given scroll event- it happens towards the end of the scrolling animation. I wonder if the browser is messing up its easing or something when the scroll is applied?

Tried swapping a few things (eg top/left positioning with transform:translate but no change).

[istarkov]

It looks like FPS but it's not,
scrollTop just sometimes jumps a little back as I logged above.

Preventing this https://github.com/bvaughn/react-virtualized/blob/master/source/Grid/Grid.js#L863-L887 code to execute during scrollTop animation, solves the problem.
As that code also changes the scrollTop value and pass not the same as my input. (In chrome with zoom other than 100% that equality is not correct (zoom used equality is correct ;-)))

The other way is to detect that scrollTop has changed by the parent component, and prevent that code block to execute, I tried a naive solution but was not successful, (also need a time).

(I wrote live wallpapers for android this for example in the past, and since that time I see even small fps issues with eyes without tooling, and even can predict FPS I see ;-))

[bvaughn]

Those wallpapers look neat! 👍 😁

Ok back to this scrolling animation issue. I can actually see it on a VM even when zoom level is 100%! The tail end of the animation is just sometimes choppy.

The more I see of this... I'm not sure it is an issue I can fix in react-virtualized. (As I mentioned before, applying your patch in my local branch doesn't change the behavior for me.)

[istarkov]

May be you are talking about small jump at the end of animation? Near 0-15px in height, It is my jump. And controlled by precision here. BTW I see issues along all animation, not just at the end.
Also small low speed scroll changes always looks bad for eyes as I know, so that precision change was to just fix that effect.

[bvaughn]

Sorry, I should have specified. I'm not using your test harness- I'm using the demo site (but testing your patch to Grid as well as a few other changes)

[bvaughn]

Hello! I believe the newly-released version 8 fixes this issue (at least in my testing).

Check out the upgrade steps for instructions on how to migrate from version 7. And let me know if you run into any problems!

[istarkov]

Super!!!, will check in a few days!!!

[bvaughn]

Sounds good. 😁 We'll talk more then.

[istarkov]

I've moved on v8.
I don't no how to say but FPS now with scrollTop animation below than 1 (yes ONE ;-)) frame per second,
here are the PR https://github.com/istarkov/revue/pull/1

I haven't investigated yet, looks like some issue with RAF

[bvaughn]

Hm... that's discouraging. The jitter-fix for Safari mostly boiled down to _removing_ the animation frame delay. 😦

So if I check out istarkov/revue and apply the PR you linked to, I should be able to repro this @istarkov ?

[istarkov]

I found the bottleneck, the problem that I use setState in onScroll handler,

So the cycle is:
I change scrollTop List property, it calls onScroll, I call setState.

Will check now, as I can avoid this in my code. But feel that can create a PR ;-)

[istarkov]

So if I check out istarkov/revue and apply the PR you linked to, I should be able to repro this @istarkov ?

Yes, now I think your incredible fast computer will not help,
but it could be my bad code design so don't waste your time.

It's not a big problem.

[istarkov]

Don't you think that onScroll should not be called if scrollTop was changed not by the scroll event?
Like <input value={x} onChange={...} /> if I change x, onChange will not be called.

[bvaughn]

Don't you think that onScroll should not be called if scrollTop was changed not by the scroll event?

Unfortunately that _used_ to be how it worked but it caused problems for certain use-cases of scrollToRow / scrollToColumn (see issue #399).

In your case, you could _ignore_ onScroll events with scrollTop values that match your component's state (or wherever you're storing the scrollTop prop)?

[istarkov]

Got it.
Yes I can easily fix my app logic, so it's not a problem.
At first sight looks like all zooming issues are gone.
Amazing work and library!

I rewrote a lot of internal controls to use react-virtualized and no issues at all,
it looks non normal ;-)

Thank you!!!

[bvaughn]

That's so great to hear!! 👏 😀 Thanks @istarkov!