-
Notifications
You must be signed in to change notification settings - Fork 841
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
Optimized in-memory datagrid mount performance #3628
Optimized in-memory datagrid mount performance #3628
Conversation
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3628/ |
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 did a functional test to see if anything changed. I noticed a sorting error (maybe, could just be unclear docs) that also seems to happen in master.
Column sorting in particular is going to be imprecise because there is no backend service to call, and data grid instead defaults to naively applying JavaScript's default array sort which doesn't work well with numeric data and doesn't sort React elements such as the links. This is a good example of what happens when you don't utilize schemas for complex data.
Should we instead say that sorting does not work at all? It gives the impression it would try? This might be because we now pass these things as react elements where we didn't before (possibly before it was just as strings).
Basically, this is unrelated to this PR, but we might want to clean up the docs while we're in there.
Enhancements only sorting does not apply
Enhancements with pagination does
^^ ignore my comment above. The location is a React node that has links in it. The docs are clear, I just didn't notice the makeup of that column. |
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 see no functional changes or regressions from this change. I did not review the code.
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.
Code changes look good. I really like the version approach.
Was able to verify substantial performance improvements via profiling
🚀
Preview documentation changes for this PR: https://eui.elastic.co/pr_3628/ |
Summary
onCellRender
callback updated to mutate the values object instead of cloning every time a cell reports a new valuePerformance
I modified the in_memory_sorting.js example and took the timings from the first 3 togglings-on
see modified example file
Previous
Mount - 2760ms, 2520ms, 2400ms
Set inMemory values - 349ms, 384ms, 326ms
Optimized
Mount - 1160ms, 1040ms, 946ms
Set inMemory values - 119ms, 92ms, 95ms
Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Props have proper autodocs- [ ] Added documentation- [ ] Added or updated jest tests- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] A changelog entry exists and is marked appropriately