[EuiDataGrid] Memoization & perf improvements #7556
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…static definitions - setting better perf examples for devs
…finitions + update snapshots
squash with complex useMemo
+ remove conditional `let`
- the inline jsx creates a new component every rerender, this does not
4 tasks
- more granular breakup of memoized vars to reduce rerenders and increase readability
…act-window` to use it
- use new deep equal memoization hook to reduce rerenders on consumer-passed `sorting` and `sortingColumns` - separate out memoized `sortedRowMap` to reduce dependencies/rerenders further - rewrite `inMemoryRowIndices` more succinctly
- remove more inline jsx vars - move fn utilities to bottom of file + wrap in `memo()` - get rid of `cellContentProps` obj, just use props
- performance changes worked I guess!
mgadewoll
reviewed
Mar 11, 2024
- `sorting` is _only_ used by header cells, so it makes sense to waterfall it just to the header row instead of to all cells (which causes rerenders across every cell every time `sorting` from a consumer is referentially unstable) + rename sorting context to `sorted` to make the purpose of the context it clearer + reorder props list slightly (by usage etc)
+ return null earlier (which requires creating new component) + convert args to obj for easier reuse + clean up/convert tests to RTL, reduce snapshots
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
kqualters-elastic
approved these changes
Mar 12, 2024
mgadewoll
approved these changes
Mar 12, 2024
|
Thanks for being so on top of reviews Lene! Will go ahead and merge this in. For context for others coming in after: Kevin and I worked pretty closely on this PR (I took his original code and wrote/pushed up the changes/refactors I wanted directly to this branch) which IMO constitutes as code review as well 😆 Huge thanks to Kevin for this impetus and his work on datagrid! |
1 task
kqualters-elastic
added a commit
to elastic/kibana
that referenced
this pull request
Apr 16, 2024
…180016) ## Summary This pr makes use of a new prop (and some generic memoization fixes) in 2 eui prs merged recently (elastic/eui#7556 and elastic/eui#7374) to improve the performance of the alerts table. Mainly, the cellContext prop is now available to consumers of the triggersActionsUi alerts table to pass in custom props in a way that allows the renderCellValue/rowCellRender functions of the various EuiDataGrid prop apis to remain referentially stable across re-renders. There are also some various changes to various hooks and props found throughout plugins that use the table to improve performance. There should be no change in functionality, just a moderate to substantial reduction in time the app spends rendering the alerts table in various scenarios. Below will be some react dev tools performance profiles, main compared to this branch, with the exact same set of generated data. Main, switching from 10-100 rows:  This branch 10-100 rows:  Pretty modest gain here, 1 render is slower than any others on main, but overall less time spent rendering by about a second. Main, opening the cell popover on 2 cells  This branch, opening cellpopover  Again nothing crazy here, modest improvement. Main opening timeline and hitting refresh  This branch, opening timeline and hitting refresh  This is the case that brought this about in the first place, as security solution shares a lot of code between tables, the alerts table recreating all cells on every render was destroying performance of the timeline rendering in a flyout/modal while users were on alerts page or the rule detail page, which is probably the most common use case. 93ms in this branch vs 2500+ ms in main. This type of performance hit happens when almost any action is taken in timeline. Main, selecting 3 alerts 1 at a time  This branch selecting 3 alerts 1 at a time  Pretty substantial improvement here as well, ~2000ms vs 67ms. Apologies if some of the gifs are cut off, please try the branch vs main on your own! This was done on a local kibana in dev mode, so things like emotion are eating up more cpu than they would for a user, but there still should be improvement, and I can't think of any circumstance where things will be worse. Also this branch was a little longer lived than I would have liked, so if you are reviewing and changed something around these files recently please double check I didn't miss anything. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Pulled out from @kqualters-elastic's #7374 PR, with more atomic git history.
The goal of this PR is to improve existing EuiDataGrid performance by reducing rerenders, via
useMemo,useCallback,memo, and replacing inline JSX with components.When reviewing changes, I strongly recommend following by commit and hiding whitespace changes.
QA
General checklist
- [ ] Checked in mobile- [ ] Checked for accessibility including keyboard-only and screenreader modes