Skip to content
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] Use binary search to find column indexes in virtualization #1903

Merged
merged 6 commits into from
Jun 18, 2021

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jun 14, 2021

Noticed when playing around with the virtualization demo in codesandbox that when I tried 100,000 columns, it would hit codesandbox infinite loop detection. Having worked on my own toy implementation of a virtualized Material UI datagrid, I realized it was looping through all possible columns to find viewport intersection. Since it has to search an ordered array, a binary search is possible here and drastically reduces iterations on high column counts. I reviewed the Material UI Datagrid implementation and found that my implementation was easy to port. Quick measurement of performance showed about a 200x performance improvement of the getColumnFromScroll function execution time on 100,000 columns (~10ms => ~0.05ms).
I realize ofcourse that wanting to render 100k columns is very much an edge-case, so feel free to discard this PR if the added complexity isn't worth it. On 1000 columns, like in the current example, the performance improvement is much less dramatic.

@Janpot Janpot marked this pull request as ready for review June 14, 2021 17:24
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 14, 2021

I realize ofcourse that wanting to render 100k columns is very much an edge-case

Can the same optimization be applied to the row virtualization? This would make it a more appealing. My guess is yes as soon as we support variable row height.

Regarding the complexity of a dichotomy. With an abstraction that we could use in more places, I think that it could potentially do it.

@Janpot
Copy link
Member Author

Janpot commented Jun 15, 2021

Can the same optimization be applied to the row virtualization?

Absolutely, the algorithm is agnostic in terms of direction. I've extracted a getIdxFromScroll that can be reused in other virtualization scenarios of variable sized items.

@oliviertassinari oliviertassinari added performance component: data grid This is the name of the generic UI component, not the React module! labels Jun 15, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 15, 2021

@Janpot It looks great 👌.

It's impressive to see what you have built on https://mui-plus.vercel.app/components/DataGrid/pinned in 3 months!

MUI+ performs about +44% better than XGrid (36 FPS vs. 25 FPS) and doesn't suffer from blocking screens every time the window jumps (the red bars). It's kicking ass! And it's not even using the CSS class name delegation strategy (move the style of the cells to a parent, avoid emotion creation for each cell).

@dtassone Could you have a closer look at the implementation of the virtualization? Maybe we can replace our implementation with this one?

Also cc @DanailH as you have been working on how to benchmark more systematically (so we can compare different implementations, PRs, etc.).

@Janpot
Copy link
Member Author

Janpot commented Jun 15, 2021

Oh that's interesting, I hadn't done any real benchmarking yet. I'm glad it's useful. Happy to answer any questions. I converted the styles as per your suggestion, thanks!

@m4theushw m4theushw changed the title [DataGrid] Use binary search to find column indices in virtualization [DataGrid] Use binary search to find column indexes in virtualization Jun 18, 2021
@m4theushw m4theushw merged commit a5974e7 into mui:master Jun 18, 2021
@Janpot Janpot deleted the bin-search-virt branch June 18, 2021 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants