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

Performance tweaking for DataGrid #2638

Merged
merged 4 commits into from
Dec 12, 2019
Merged

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Dec 12, 2019

Summary

I played a bit with the performance on EuiDataGrid component and I noticed that there's a visual lag for bigger datasets(around 100 rows and more). At the moment even the smallest change like selecting a different cell unnecessarily rerenders the whole table. Here's the gif with the updates highlighted and a React profiler view:

Animated GIF-downsized_large (1)

image

The good news is there are a couple of easy gains to make it significantly faster (from 200ms to 10ms in dev environment for table of 100rows). The changes include:

  • memoizing the context for the DataGrid - before, along with any changes on the EuiDataGrid, context was being recreated and all the cells (as they are accessing context) rerendered.
  • memoizing the row component with React.memo
  • before the prop focusedCell was passed to every Row component. But if we change the cell from [0,0] to [1,1], the row 3 doesn't have to know about the change because for it nothing changes (the cells inside the row 3 don't have to know it either). So instead of passing the [x,y] position of focusedCell I propose to pass focusedCellPositionInTheRow which is only the x value of focused position.

Example:
Let's say the cell [3,1] is focused.
The row 0 will get focusedCellPositionInTheRow=null
row 1: focusedCellPositionInTheRow=3
row 2: focusedCellPositionInTheRow=null
row 3: focusedCellPositionInTheRow=null

This way only the involved rows will be rerendered. I know we pay the cost of lower readability but... well... that's performance problems in its core, readability vs performance 😅

Here's the stats after the changes:
image

Checklist

  • [ ] Checked in dark mode
  • [ ] Checked in mobile
  • [ ] Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • [ ] Added documentation examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@mbondyra mbondyra requested review from chandlerprall, cchaos and myasonik and removed request for chandlerprall December 12, 2019 17:13
@cchaos cchaos requested review from snide and removed request for cchaos December 12, 2019 17:23
Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great improvement and should certainly help make Discover feel a little snappier!

🎉

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGreatTM, pulled & tested locally. Love it, and thank you @mbondyra !

@chandlerprall
Copy link
Contributor

Missing a CHANGELOG.md entry - should get a quick note on the performance improvement in the top Bug Fixes section https://github.com/elastic/eui/blob/master/CHANGELOG.md

@mbondyra mbondyra merged commit ee5af7e into elastic:master Dec 12, 2019
@mbondyra mbondyra deleted the perf-data-grid branch December 12, 2019 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants