-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGrid] New virtualization implementation #2673
Conversation
8179ea6
to
e6695f9
Compare
e6695f9
to
05e2e54
Compare
73496de
to
1774576
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
55ae98d
to
afb4b91
Compare
da735f5
to
60df198
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
6035ae6
to
ca2d153
Compare
ca2d153
to
f286fed
Compare
f286fed
to
43b31b9
Compare
const renderingZoneRef = React.useRef<HTMLDivElement>(null); | ||
const rootRef = React.useRef<HTMLDivElement>(null); | ||
const handleRef = useForkRef<HTMLDivElement>(ref, rootRef); | ||
const [renderContext, setRenderContext] = React.useState<RenderContext | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this one to be internal will help a lot if we want to migrate to a regular React state (useState
) instead of a ref in the future 👍
@@ -59,7 +59,7 @@ function GridColumnHeadersItemCollection(props: GridColumnHeadersItemCollectionP | |||
|
|||
return ( | |||
<GridColumnHeaderItem | |||
key={col.field} | |||
key={idx} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This small change yielded one of the main performance achievements. Previously, on each render React had to create a new element since it didn't know one to reuse.
handleRowsScrollEnd({ left, top }); | ||
}, | ||
[handleRowsScrollEnd], | ||
); | ||
|
||
// TODO: Check if onViewportRowsChange works as expected once virtualization is reworked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DanailH What's the purpose of this prop? Maybe with the render context being passed in the rowsScroll
params it might not be necessary anymore. I didn't have the time to look into how this prop works, so for now it's not being called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes - this is the first step of the server-side infinite loader. When this was created the virtualization worked in 'pages' so this called every time a new virtual page
becomes visible and passes in the first and last row indexes.
I guess since we no longer have these "pages" this will be called each time the grid scrolls with the new first and last row indexes, or it can be when the overscan changes?
Update: Now that I got familiar with the PR I guess this prop rowBuffer
is the one that is needed, so when this changes the mentioned callback should be called. That way it will provide the ability for developers to preload rows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you say 'virtual page' you can now replace it with 'render context'. They work almost the same way. My point is that I think this prop is not relevant anymore. If we need to know when the first and last row indexes have changed, it can be done only by comparing the reference to the render context. I'm already doing that for the columns headers in https://github.com/mui-org/material-ui-x/pull/2673/files#diff-8160a4255306366ba065fa7036042dba41c51e2602d8967706b95fe37aa655eaR116
|
||
const handleResize = React.useCallback(() => { | ||
if (rootRef.current) { | ||
setContainerWidth(rootRef.current.clientWidth); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing Element.clientWidth
or Element.scrollTop
all the time is harmful to the performance since it may triggers layout recalculation. See: https://gist.github.com/paulirish/5d52fb081b3570c81e3a
/** | ||
* Styles applied to the data container element. | ||
*/ | ||
dataContainer: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we list the removed classes as breaking changes? I don't feel that the new ones are stable enough to be used as replacements.
|
||
virtualPage = apiRef.current.state.rendering!.virtualPage; | ||
expect(virtualPage).to.equal(0); | ||
it('should render extra columns when the columnBuffer prop is present', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asserting virtualization stuff in unit-tests is hard. Each browser may use different translate
values which would break our tests. I didn't put too much effort in testing here. I relayed more on argos-ci. I'll do another PR to clean all these tests and exercise every single bit of the virtualization.
@@ -24,7 +26,7 @@ const useUtilityClasses = (ownerState: OwnerState) => { | |||
const { scrollDirection, classes } = ownerState; | |||
|
|||
const slots = { | |||
root: ['scrollArea', `scrollArea__${scrollDirection}`], | |||
root: ['scrollArea', `scrollArea--${scrollDirection}`], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small regression I found. Dragging one column to the very edge, would scroll only to the left.
|
||
const style = { width, height }; | ||
|
||
return <div className="MuiDataGrid-cell" style={style} />; // TODO change to .MuiDataGrid-emptyCell or .MuiDataGrid-rowFiller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think one component should share styles from another one.
firstColumnToRender: number; | ||
lastColumnToRender: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used, but since there's a slot for the row, somebody might want to do something with it.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@m4theushw Maybe I missed something but why did you remove the |
Based off this issue mui/mui-x#2673 (comment), I believe the reason for the column issue has to do with how mui is virtualizing the headers. I increased the columnBuffer to a large enough number so all the column headers are always generated. Prior to this, I could regularly break the headers, but I haven't been able to since. In theory, this is slower, but it's a very small amount of content and the trade-off seems well worth it, because there isn't really a way to solve the virtualization issue if my diagnosis is correct.
Breaking changes
[DataGridPro] Remove the
onViewportRowsChange
prop and theviewportRowsChange
event. Add a listener on therowsScroll
event as replacement. Check https://codesandbox.io/s/create-react-app-forked-ty1w7?file=/src/App.js.[DataGrid] The CSS classes
.MuiDataGrid-window
and.MuiDataGrid-windowContainer
were removed.[DataGrid] The following CSS classes were renamed:
.MuiDataGrid-viewport
was renamed to.MuiDataGrid-virtualScroller
.MuiDataGrid-dataContainer
was renamed to.MuiDataGrid-virtualScrollerContent
.MuiDataGrid-renderingZone
was renamed to.MuiDataGrid-virtualScrollerRenderZone
Preview: https://deploy-preview-2673--material-ui-x.netlify.app/components/data-grid/#commercial-version
EDIT: I renamed
GridVirtualizedContainer
toGridVirtualScroller
to use a shorter name.Diagnostic
Based mostly on #1933, the current virtualization has the following problems:
useGridVirtualization
anduseGridContainerProps
.scrollTop
position is not respected, which leads to the famous "jump". [DataGrid] Virtualization not respecting scrollTop #1911disableVirtualization
prop is not a true non-virtualized mode as it still needs to apply a CSS transformation to sync the scroll position.I didn't list all issues here but after the merge I'll go on each one closing them.
Proposed solution
Based on the problems mentioned above, this PR is proposing to add a new
GridVirtualScroller
which will control everything related to virtualization of rows and columns. The new virtualization doesn't reinvent the wheel and implements the same behavior of well known React libraries (e.g. react-virtualized). To begin to understand it, first we have to have in mind what a "render context" means here. The render context is simply a object containing the first and last rows/columns to render. Note that these values don't take into account overscan. The virtualization works by watching thescrollTop
position of a scrollable container and each time a scroll event occurs, it calculates which should be the new first rendered row. Based on that value, the last rendered row is also calculated. After that, it compares how many rows (distance) the user has scrolled since the last scroll event. If this distance is equals or greater thanrowThreshold
, then it triggers a re-render and updates the render context. When actually rendering the rows, then overscan kicks in to render extra rows to avoid blank spaces. Since now the virtualization relays on the native scroll, there will be situations where no single re-render will occur, as the render context didn't change. The same behavior is also applied to the columns now, which solves the issue with horizontal scroll. After every scroll, therowsScroll
event is published with the scroll position and current render context.For the virtualization of the column headers, it works almost the same way. The main difference is that instead of watching the scroll position of the main container, it listens to the
rowsScroll
event. Since the headers can't leverage the native scroll, it needs to do a CSS transformation on every scroll. See the trade-off below.FPS comparison
Taken from running
yarn benchmark:browser
.Before:
After:
Trade-off
The new virtualization is not perfect. The main issue which users might notice is the column headers appearing mis-aligned or out-of-sync with the their respective columns during scrolling. It can happen on a low-end device or when scrolling very fast in some browsers (I noticed it in Firefox). The rationale behind it is that the main container relays on the native scroll, which runs on a separate thread. In the other side, the virtualization of the column headers runs on the UI thread and this one is updated at intervals which might not match with the other thread. The
ScrollSync
component from react-virtualized suffers from the same problem. Here's an explanation from the author of this library: bvaughn/react-virtualized#369 (comment)If that becomes an issue, we can add a prop to slow down the virtualization and force it to stay in sync with the headers. That's the last resort, only to be used if we can't argue why the native scroll is better.
Why it didn't happen with the old virtualization? Previously, the native scroll was not being used. On every scroll event, there was a CSS transformation to update the rendering zone position, so headers and columns stayed always in sync.
ZmsDbMOQBv.mp4
Components removed
<GridStickyContainer />
- Hides anything that exceeds the grid's size. Despite the name, it renders theMuiDataGrid-viewport
element. The same role is done now by theMuiDataGrid-virtualizedContainer
element.<GridViewport />
- This is the old virtualization component. Replaced by the newGridVirtualScroller
.<GridEmptyCell />
- When the width taken by the columns is less than the grid's width, this component fills the empty space of the row. The filler now is embedded into the row component.[DataGrid] Remove GridRowCells #2811<GridRowCells />
- Returns the cells that make part of the row. Its functionality was merged intoGridRow
.<GridDataContainer />
- Used to force the scrollbars to be visible. Has width and height equal to the content's size. It's represented now by theMuiDataGrid-content
element rendered by the new virtualization component.<GridWindow />
- Used to display the content below the header, sinceMuiDataGrid-columnsContainer
is absolute positioned. It was replaced by adding a top margin to the virtualization container.Hooks removed
useGridVirtualization
- It watches the scroll position, calculates which rows and columns should be rendered and updatesstate.rendering.renderContext
. It also moves the column headers when there's a horizontal scroll. It was replaced byGridVirtualizationContainer
.useGridNoVirtualization
- Like the previous one, but always rendering all rows and columns. This behavior is now embedded intoGridVirtualizationContainer
, which means that when virtualization is disabled there's no re-render in the content area.useRenderInfoLog
- Watches the scroll position and logs everytimestate.rendering.renderContext
is changed. I could've migrated it to work with the new virtualization, but I found that this hook generates too much noise.New props
rowBuffer
- On each scroll, it's calculated the first and last row that should be visible. This prop controls how many additional rows (a.k.a. "overscan") should be rendered before and after the calculated ones. Having these extra rows prevents the user from seeing a blank space when scrolling faster.rowThreshold
- Controls how many rows from the buffer can be visible. It can also be understood as how late the virtualization will render new rows.columnThreshold
- Same as rows but for columns. ThecolumnBuffer
prop already exists.Future work
GridVirtualScroller
from the state to allow reusability?