Stuttery scrolling in Chrome with `VirtualScroll` about react-virtualized HOT 48 CLOSED

I didn't realize this bug had been closed, sorry. No it didn't. I don't understand why rendering more rows above and below would fix this; the scrolling has "jank". I'm not talking about the blank space at the top and bottom. That is a separate issue, and I'm glad you added that option because it's definitely needed, but it doesn't do anything to make the scrolling actually smooth. Why would it? As far as I know, you're just rendering more rows above & below right?

The original bug was referring to actual jank in Chrome, for some reason. I still need to make a simple JS page to see if I can reproduce it (my app is in ClojureScript) but if I render all the rows without VirtualScroll it scrolls just fine. So I'm not sure what's going on.

from react-virtualized.

@bvaughn I figured it out. On master the _onWheel handler was removed from the container element, and previously both _onWheel and _onScroll events were firing and I think twice as much work was happening. Now that it was removed, Chrome is a lot smoother. I can replicate this on the old code by just commenting on the onWheel: this._onWheel line.

So good job removing that :)

from react-virtualized.

Awesome, makes total sense. Thank you!

from react-virtualized.

Hey @jlongster

This is interesting. I haven't seen this in Chrome or anywhere else- excepting the iOS simulator if I scroll really fast. Although I do have a branch in progress that uses an overscan approach that seems to reduce the frequency in the simulator. I wonder what's different about my testing environment and yours... hm.

What OS? Are you using a trackpad or a mouse wheel?

from react-virtualized.

OS X 10.11, using the builtin trackpad. I can try to reproduce it on a simpler page.

from react-virtualized.

Interesting. My dev box is 10.10.5. Maybe I need to find a 10.11 box to try it on. Thanks for the added context James.

from react-virtualized.

Took a stab at recreating a similar situation locally (full page table with a few columns). My Chrome scrolling is very smooth. OS X 10.10.5, Chrome 48.0.2564.103. (Same results for Chromium 48.0.2553.0.)

https://s3.amazonaws.com/storage.briandavidvaughn.com/issues-87.mp4

Interestingly enough I am able to reproduce (to a lesser extent) the issue your video shows with Firefox 43 and 44. I will use that as a base for now.

from react-virtualized.

Hi again @jlongster,

Now that I've been able to reproduce it I can verify that my overscan branch seems to dramatically reduce the amount of perceived "stutter" as you call it. (I think it's actually scroll events not being fired frequently/quickly enough for RV to shuffle around the pool of visible rows.)

Here's a build of the branch in question for you to check out. FlexTable has a new property called overscanRowsCount which defaults to 0 but can be set to a positive number (eg. 10 or so) to pre-render a few rows above and below the visible region. In my testing so far, this helps with the flicker.

Give it a try and let me know? :)

https://s3.amazonaws.com/storage.briandavidvaughn.com/react-virtualized-4.9.0.tgz

from react-virtualized.

I will try it soon, it's the weekend and not doing much on computers :) Thanks for the possible fix!

I'm using Firefox 46 (which is dev edition, almost like nightly), so maybe it's been fixed there. I think a lot of async rendering work was done in 45 or 46 so it's likely to be smoother in those. Not sure why you can't reproduce it in Chrome.

from react-virtualized.

Unfortunately it's still stuttery for me. I noticed that it was stuttery in Firefox 44 as well, but it smooth in 46. Can you try having an in each column, and are you wrapping it with AutoSizer?

I can still try to post a minimal test case, just haven't had the time yet.

EDIT: I wasn't supposed to pass in any different props right?

from react-virtualized.

Unfortunate! Hm. I will try what you described with input.

Yes in my full page example the FlexTable is wrapped in an AutoSizer.

On Sunday, February 7, 2016, James Long [email protected] wrote:

Unfortunately it's still stuttery for me. I noticed that it was stuttery
in Firefox 44 as well, but it smooth in 46. Can you try having an in each
column, and are you wrapping it with AutoSizer?

I can still try to post a minimal test case, just haven't had the time yet.

—
Reply to this email directly or view it on GitHub
#87 (comment)
.

from react-virtualized.

I wasn't supposed to pass in any different props right?

@jlongster: Ah, actually yes. I meant for you to pass in overscanRowsCount values greater than 0 (the default). I should have made that clearer (or just picked a default that was not 0 for you). Sorry!

from react-virtualized.

Here's another build for you with overscanRowsCount set to 30 by default:
https://s3.amazonaws.com/storage.briandavidvaughn.com/react-virtualized-4.10.0.tgz

And here's an updated video of the table with input tags (which is what I think you were asking for before but Github cut the word out):
https://s3.amazonaws.com/storage.briandavidvaughn.com/issues-87.webm

PS. I know it's the weekend and Super Bowl Sunday at that, so feel free to ignore this message for the evening :)

from react-virtualized.

Interestingly enough, in my harness (with a full-page FlexTable containing 1,000 rows) scrolling to the bottom of the page with the track pad as quickly as I can triggers ~62 scroll events in Chrome 48.0.2564, ~69 in Safari 9.0.3, and ~122 in Firefox 44.0.

I actually expected fewer events in Firefox given the visual behavior I'm seeing.

from react-virtualized.

FYI in my testing, the PR I just merged resolves the issue. Since James is a bit distracted now by the little one ;) I'm going to move forward with the fix. When you get back @jlongster, if you're still seeing this issue, please do reach back out to me and we can pick things back up.

from react-virtualized.

FYI this went out as release 4.10.0

from react-virtualized.

Funny enough, I was reading this post about FF 46 and APZ and saw:

Checkerboarding is inevitable with asynchronous scrolling due to memory limitations; it’s the cost of removing jank. However, in most cases it shouldn’t be noticeable – it only lasts a few hundred milliseconds, and we are working hard to bring that down further. The way we do this is by painting content outside of the visible area ahead of time, so that when the user scrolls it’s ready to be displayed right away.

That's interesting :) That's basically the same thing I ended up doing for #53 and this issue.

from react-virtualized.

You should try it in Firefox 46 (called "dev edition" right now, because it's 2 releases away). I wonder if more events make it stuttery because it's trying to execute more JS code and it's causing jank. Either way, it's definitely fixed in Firefox 46 because some large async composition work landed.

No idea why you can't reproduce it in Chrome. FWIW here's a timeline screenshot from Chrome after recording 1.5s of scrolling:

The yellow is scripting and the green is painting. There are some parts where it's 100ms of pure scripting, no painting. I can't really tell you why, I don't know how to use the Chrome performance tools much deeper.

Still going to create a simpler test case, sorry, just had a baby this week :)

from react-virtualized.

Excited to read about the upcoming FF 46 release. Sounds slick.

No worries on the test case. I saw (via Twitter) that you had a baby this week so I assumed you'd be AFK for a while. Take your time. :)

from react-virtualized.

FWIW you can read more about it here (good timing, it was just posted): https://hacks.mozilla.org/2016/02/smoother-scrolling-in-firefox-46-with-apz/

from react-virtualized.

Yeah, I actually referenced it in my previous comment above. :) It's kind of funny timing that you guys mentioned doing something similar to what I am trying out with the overscan properties.

from react-virtualized.

Whoops, you're totally right. The overscan stuff just helps with "checkerboarding", the effect of not seeing any content if you scroll to fast (seen in your components as just blank space if you don't overscan). But yes, the general technique of only rendering what's in the viewport is the same.

from react-virtualized.

Sorry, I actually feel kind of silly asking this, but did my change resolve (or improve) the "stuttering" problem you originally reported in any way? I did for me locally.

from react-virtualized.

I feel a little foolish now but I assumed the "jank" you're referring to was the empty space that resulted from fast scrolling (due to a lack of "overscanning"). Watching your video again now, that's the only difference I can see between the non-RV version and the RV-version. (Maybe something is lost in the mp4? or maybe I'm just looking for the wrong thing.)

I've re-opened this issue in the meanwhile although I'm a little unclear on how to proceed. The internet makes things like this a little difficult... :)

from react-virtualized.

No worries! I filed the issue in a hurry and my video is confusing. I'll try to get something online that you can use, you probably have to feel it.

from react-virtualized.

Ok, I have a reproducible test case (at least on my machine!)

Here is a similar page using react-virtualized. It does not scroll smoothly for me in latest Chrome: http://jlongster.com/s/virtual-jank/ It scrolls fine in Firefox 46.

Here is the same page that doesn't use react-virtualized. It just renders everything at once. It scrolls very smoothly in Chrome, notice the difference: http://jlongster.com/s/virtual-jank/index-no-virtual.html

You can view source to see the source, it's a very small JS file.

This may just be a problem with Chrome and dynamically adding inputs. Maybe it's not as fast.

from react-virtualized.

Interesting. Thanks for sharing. I do see it now, although it's subtle. Hm. I'll look into it. :)

from react-virtualized.

It is subtle, but for me it kills the "buttery smooth" feeling which happens if there is any jank, even a few milliseconds. After thinking about it more, I doubt this is your problem. I suspect that Chrome is slower when dynamically inserting several <input> tags. Because inserting new nodes is inherit in virtualization, I'm not sure there's anything you can do about it.

To be sure: it is inserting new rows right? I wonder if there is a way to make it "re-use" previous rows but just update it with new data.

from react-virtualized.

This topic has been on my mind recently. I think a typical approach to this pattern is to reuse a pool of renderers. Within React's virtual DOM though, and due the fact that the content of the rendered rows is dynamic based on what the user's rowRenderer function returns, I'm not sure how to achieve this. I need to do some digging I guess. Maybe there is a smarter way than what I am currently doing.

from react-virtualized.

I haven't looked at this implementation yet, but I know there is a lot of research on UITableView-like functionality (if you don't know, that's iOS' view that accomplishes the same thing). According to some facebook devs, we can never get it as perfectly smooth as iOS because various things like image decoding happen on the same thread (so if you use images in each row it may stutter), but there's no reason we can't come close.

I think re-using DOM elements will be pretty important for consistent performance. If it's not possible without a huge amount of complexity, what you have now still works pretty great. But it might be worth checking out what other people have done in this area. I found this project: https://github.com/seatgeek/react-infinite but I don't see where it re-uses DOM. From the API I can't imagine how it would (builds up new virtual DOM each time with distinct keys).

Can we leverage React's key system to force it to reuse DOM? You currently specify the row index as the key property, forcing React to create new DOM as new rows scroll into view. What if the we calculate an index that is relative to the rows that actually exist in the DOM at this point in time? So each render it would always start at 0 and count upwards for all the rows returned here: https://github.com/bvaughn/react-virtualized/blob/master/source/VirtualScroll/VirtualScroll.js#L260-L279. Basically, the key property would be i - start in that loop.

That would tell React to actually reuse any of the existing rows in the DOM. It will take your new rows and update the existing ones with the same diffing algorithm we're used to. Most likely, you are always returning rows of the same DOM but just different values, which should work well with React's diffing algorithm. If you were changing the structure frequently it would probably be more efficient to just always insert new rows, but I don't think that's the case for most people using this kind of library.

In summary: there will be more diffing work by React for each render, but less DOM insertion. This is just a theory, but it might be more efficient. I also depends on the complexity of the user's row; if you have a lot of DOM in there, new insertions may always be more efficient. Given that this is easy to tweak (just change the key property`, maybe we could even make this an option.

from react-virtualized.

Wow, I just tried it out and it seems to make Chrome smoother but Firefox a lot slower. Not sure what performance cliff we are hitting in Firefox. Would be interesting to try to profile all this and see what's going on.

from react-virtualized.

This might not be a good road to go down. Given that the only problem with the current implementation is slight stutter with lots of inputs, we can wait until we hit other perf problems with not reusing rows. I'm going to look into why Chrome is stuttering a bit in this scenario.

from react-virtualized.

Just thinking out loud. You discovered before that Firefox is firing a lot more scroll events. I wonder if that's the issue now with more CPU work: it's overloading the system in Firefox by pumping in too many events and it's doing too many unnecessary diffs. Maybe we should rate limit the scroll events to 60 FPS?

from react-virtualized.

Oh, of course. With the technique I described above, it's updating the entire list of rows with new data when new rows come on the scene. This happens because it "shifts" down the entire list of existing rows. A row with key 4 will now have different data so it needs to update the DOM. This results in every single cell being repainted:

Interestingly, in Chrome only every row is repainted, it doesn't look like every cell is:

I don't know how it's getting by without repainting as much. I would have thought that React is actually changing the contents of every cell every single time now. Any ideas on that? Maybe it is actually rerendering each cell but the paint flashing colors aren't as clear as Firefox's. And Chrome is just a lot faster in this case of repainting.

Anyway, I'll think about this some more. Seems like we could still leverage the key system to tell React exactly which rows can be reused.

from react-virtualized.

Sorry James, I had to step away from the keys for an appointment. I had been considering using a keys-tweak like you mentioned, but with a modulus operator instead to avoid the "shift" you described above. Not sure if this would have an impact though. Plan to try it sometime this afternoon or evening.

from react-virtualized.

Haha, I'm trying the exact same thing right now :) A small problem I hit is knowing what number to use for the modulus. getVisibleCellIndices returns different ranges even if you don't resize at all, depending on scroll position. I forced it to return the same length though, and then I do i % (stop - start). That should force it to "move" rows at the top down to the bottom and reuse it if scrolling down.

I don't see any difference in Chrome, but it's hard to tell if it's actually working. I can't use paint flashing because it'll repaint no matter what, so it's hard to see if it's reusing DOM nodes or not. I'll keep looking at it.

With paint flashing enabled I did notice something weird. Apparently Chrome is still repainting the entire area for some reason, while Firefox works the way you think, only painting the top and bottom rows. Not sure what's causing that, and maybe it's something unrelated to inserting new dom nodes.

EDIT: I certainly wouldn't expect a response within an hour, no need to apologize!

from react-virtualized.

I think we may be going down the wrong path for this specific problem. Try enabling paint flashing in Chrome (devtools menu > more tools > rendering settings). While scrolling, when a new row is hit you'll see that it rerenders the entire area. Why is that? It should only repaint the new row that was inserted. This happens regardless of my key tweaks.

I'm going to work on other stuff for a while so there's no rush.

EDIT: and when it does that full repaint, that's definitely when there is some jank

from react-virtualized.

I think I found one potential problem: each row is placed by setting the top CSS property, which apparently makes Chrome redraw the whole viewport. I changed it to transform: translateY(...) and it seems a bit smoother.

from react-virtualized.

@bvaughn I just pulled the latest master and it actually seems to be a lot better in Chrome. I'm not sure what you changed :) Was there anything specific to the markup or CSS that you changed? The paint flashing still indicates that it's repainting the whole area, but it's not nearly as jittery.

We can close this issue for now as there isn't a clear problem. Feel free to file a new issue if you want to look into reusing DOM nodes; so far I'm not sure if it's worth the effort.

from react-virtualized.

Thanks for the back and forth on this James. I do plan on looking into
replacing the 'top' positioning if nothing else.

TBH I'm not sure what changed in master vs the last RC I sent you. Nothing
that I was aware of.

On Friday, February 12, 2016, James Long [email protected] wrote:

@bvaughn https://github.com/bvaughn I just pulled the latest master and
it actually seems to be a lot better in Chrome. I'm not sure what you
changed :) Was there anything specific to the markup or CSS that you
changed? The paint flashing still indicates that it's repainting the whole
area, but it's not nearly as jittery.

We can close this issue for now as there isn't a clear problem. Feel free
to file a new issue if you want to look into reusing DOM nodes; so far I'm
not sure if it's worth the effort.

—
Reply to this email directly or view it on GitHub
#87 (comment)
.

from react-virtualized.

Yeah, I don't see anything in the changes. That's super weird. I'm going to do some bisecting to see if I can figure out what changed. I know I switched to the production version of React but I just switched back to dev and it's still very smooth.

Sorry for the ambiguous problem originally, and thanks for talking it through! I was going to request the "overscan" feature as well so I'm glad you went ahead and did that :)

from react-virtualized.

Haha! (˃̶᷄︿๏）

I didn't realize that hadn't been removed in the branch you and I were playing with.

Yeah, that was kind of silly of me in the first place. Geeze.

from react-virtualized.

I should have posted the exact commit I was using :) I git cloned this project a little while ago and didn't even think to try master.

from react-virtualized.

Didn't have time to read this entire exchange but made it about half way through and wanted to give you an update for me at least...

When I use on firefox 46 it scrolls BEAUTIFULLY, no stagger whatsoever. Safari has a little lag but barely noticeable. On Chrome 50 with 10.11.3 OSX I get the most stutter lag. This is in a FlexTable implementation that uses both InfiniteLoader for data fetching and AutoSizer so that might have something to do with it.

When I increase the overscanRowCount to 20-30 I run into this error: defaultCellDataGetter.js:19 Uncaught TypeError: Cannot read property 'get' of undefined

I'm guessing it has something to do with the async loaded data if I scroll down too fast. If I keep the overscan count down to about 10 it helps with the stuttering but still doesn't eliminate it. At 10 it also never throws that 'get' undefined error though.

I'm very new to react-virtualized so I might be conceptually missing a step. Here is the code that produces the lag/error:

import React, { Component, PropTypes } from 'react';
import { InfiniteLoader, AutoSizer, FlexTable, FlexColumn } from 'react-virtualized';
import { Text, Button, TableHeader } from 'athena-ui';
import css from './AppointmentList.scss';

const propTypes = {
  appointments: PropTypes.array.isRequired,
  isFetching: PropTypes.bool.isRequired,
  pagination: PropTypes.object.isRequired,
  onLoad: PropTypes.func.isRequired,
};

class AppointmentList extends Component {
  constructor(props) {
    super(props);

    this.loadMoreRows = this.loadMoreRows.bind(this);
    this.providerRenderer = this.providerRenderer.bind(this);
    this.statusRenderer = this.statusRenderer.bind(this);
    this.headerRenderer = this.headerRenderer.bind(this);
    this.textRenderer = this.textRenderer.bind(this);
  }

  loadMoreRows({startIndex, stopIndex}) {
    // If I don't want the autofetching to happen immediately I can
    // use this to only load when getting close to the end of the list.
    const { isFetching, appointments, onLoad } = this.props;
    if (isFetching) {
      return Promise.resolve();
    }

    const awayFromBottom = appointments.length - stopIndex;

    if (awayFromBottom < 15) {
      return Promise.resolve(onLoad());
    }
  }

  providerRenderer({rowData}) {
    return (
      <Text>
        {rowData.providerName}<br />
        {rowData.departmentName}
      </Text>
    );
  }

  textRenderer({cellData}) {
    return <Text>{cellData}</Text>;
  }

  statusRenderer({cellData}) {
    if (cellData === 'No Reminder Call') {
      return <Button isCta>{cellData}</Button>;
    }
    return <Button>{cellData}</Button>;
  }

  headerRenderer({label}) {
    return <TableHeader clean>{label}</TableHeader>;
  }

  render() {
    const {
      appointments,
      pagination: { nextPage },
    } = this.props;
    // Does pagination have a next page?
    const hasNextPage = typeof nextPage === 'number';
    const rowCount = hasNextPage ? appointments.length + 1 : appointments.length;
    // Every row is loaded except for our loading indicator row.
    const isRowLoaded = (index) => !hasNextPage || index < appointments.length;

    return (
      <InfiniteLoader
        isRowLoaded={isRowLoaded}
        loadMoreRows={this.loadMoreRows}
        rowCount={rowCount}
      >
        {({ onRowsRendered, registerChild }) => (
          <AutoSizer>
            {({ height, width }) => (
              <FlexTable
                ref={registerChild}
                width={width}
                height={height}
                headerHeight={30}
                onRowsRendered={onRowsRendered}
                rowHeight={60}
                rowCount={rowCount}
                rowGetter={({index}) => appointments[index]}
                rowClassName={css.appointmentRow}
                overscanRowCount={10}
              >
                <FlexColumn
                  label="Appt. Time"
                  dataKey="startTime"
                  flexGrow={1}
                  width={150}
                  headerRenderer={this.headerRenderer}
                  cellRenderer={this.textRenderer}
                />
                <FlexColumn
                  label="Appt. Type"
                  dataKey="appointmentType"
                  flexGrow={1}
                  width={200}
                  headerRenderer={this.headerRenderer}
                  cellRenderer={this.textRenderer}
                />
                <FlexColumn
                  label="Provider"
                  dataKey="providerName"
                  flexGrow={4}
                  width={200}
                  cellRenderer={this.providerRenderer}
                  headerRenderer={this.headerRenderer}
                />
                <FlexColumn
                  label="Patient"
                  dataKey="patientName"
                  flexGrow={2}
                  width={200}
                  headerRenderer={this.headerRenderer}
                  cellRenderer={this.textRenderer}
                />
                <FlexColumn
                  label="Confirmation Status"
                  dataKey="confirmationStatus"
                  cellRenderer={this.statusRenderer}
                  headerRenderer={this.headerRenderer}
                  flexGrow={1}
                  width={200}
                />
              </FlexTable>
            )}
          </AutoSizer>
        )}
      </InfiniteLoader>
    );
  }
}

AppointmentList.propTypes = propTypes;
export default AppointmentList;

from react-virtualized.

Here is how it looks on firefox
Here is what it looks like on chrome

from react-virtualized.

React (especially >= 15.x) doesn't run the smoothest in development mode, which seems to be what your demos are running in (given the Redux devtools on the right). Have you tried running in production mode?

from react-virtualized.

Totally had it running dev mode! Great catch 👍 . Running it in production helps a LOT. It is still a tiny bit staggered if I scroll really fast. Any suggestions?

When using InfiniteLoader with FlexTable do you have any example of how to show a loading icon when it's fetching the new rows? I had that working with the Virtual Scroll but there is no rowRenderer function on the FlexTable like there was on the VirtualScroll.

Is it possible the staggering from scrolling down might be from data just being loaded and if I have a loading indicator that would fix it?

from react-virtualized.

No problem. I think that's a common pitfall for React users. :)

I don't have any examples of that, no. You could override cellRenderer for your FlexColumns and render a placeholder just like you would for VirtualScroll. The default implementation is really basic anyway if you just wanted to fork it:

export default function defaultCellRenderer ({
  cellData,
  cellDataKey,
  columnData,
  rowData,
  rowIndex
}: CellRendererParams): string {
  if (cellData == null) {
    return ''
  } else {
    return String(cellData)
  }
}

It's possible the flickering could be from data loading in, yeah. With a placeholder in place you'd notice that more easily.

from react-virtualized.