Stuttery scrolling in Chrome with VirtualScroll
#87
Comments
|
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? |
|
OS X 10.11, using the builtin trackpad. I can try to reproduce it on a simpler page. |
|
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. |
|
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. |
|
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. Give it a try and let me know? :) https://s3.amazonaws.com/storage.briandavidvaughn.com/react-virtualized-4.9.0.tgz |
|
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. |
|
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 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? |
|
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 notifications@github.com wrote:
|
@jlongster: Ah, actually yes. I meant for you to pass in |
|
Here's another build for you with 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): PS. I know it's the weekend and Super Bowl Sunday at that, so feel free to ignore this message for the evening :) |
|
Interestingly enough, in my harness (with a full-page I actually expected fewer events in Firefox given the visual behavior I'm seeing. |
|
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. |
|
FYI this went out as release 4.10.0 |
|
Funny enough, I was reading this post about FF 46 and APZ and saw:
That's interesting :) That's basically the same thing I ended up doing for #53 and this issue. |
|
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 :) |
|
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. :) |
|
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/ |
|
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. |
|
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. |
|
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. |
|
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. |
|
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... :) |
|
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. |
|
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. |
|
Interesting. Thanks for sharing. I do see it now, although it's subtle. Hm. I'll look into it. :) |
|
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 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. |
|
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 |
|
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 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 |
|
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. |
|
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. |
|
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? |
|
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. |
|
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. |
|
Haha, I'm trying the exact same thing right now :) A small problem I hit is knowing what number to use for the modulus. 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! |
|
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 |
|
I think I found one potential problem: each row is placed by setting the |
|
@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. |
|
Thanks for the back and forth on this James. I do plan on looking into TBH I'm not sure what changed in master vs the last RC I sent you. Nothing On Friday, February 12, 2016, James Long notifications@github.com wrote:
|
|
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 :) |
|
@bvaughn I figured it out. On master the So good job removing that :) |
|
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. |
|
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. |
|
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: 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; |
|
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? |
|
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? |
|
No problem. I think that's a common pitfall for React users. :) I don't have any examples of that, no. You could override 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. |
|
Awesome, makes total sense. Thank you! |
I just noticed that the
VirtualScrollelement has stuttery scrolling in Chrome. Here's a video: http://jlongster.com/s/chrome-stutter.mp4I'm wrapping it in
AutoSizerfwiw, but I don't see how that could be related. Don't worry much about the code (it's ClojureScript), the rows it's rendering is pretty basic: just a row of inputs with values. I don't know if anyone else has seen this (I'm using latest master of react-virtualized) and Chrome 48.I can try to make a reduced test case later, just thought I'd post this now.
The text was updated successfully, but these errors were encountered: