[Dashboards] Rendering slow down after moving to new grid-layout engine #207011
Assignees
Labels
bug
Fixes for quality problems that affect the customer experience
Team:Presentation
Presentation Team for Dashboard, Input Controls, and Canvas
Comments
thomasneirynck
added
bug
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
|
Closing based on offline discussion |
|
This might not be as solved as we originally thought 🙈 Looking into a possible solution here - will close this again depending on the results there. Sorry for all the noise on this one🙇 |
2 tasks
|
So, the results of the above PR are.... inconclusive based on the one-off test:
Our telemetry has gotten very inconsistent as of late, with a large variance in some of the tests - this makes it pretty difficult to compare to a one-off result unfortunately :( All I'm really seeing is another "swap" of time-to-data + dashboard overhead. I think it's still worth merging the double render fix, but it will take some time to see if that PR actually closes this issue. |
Heenawter
added a commit
that referenced
this issue
Feb 13, 2025
This PR **might** resolve #207011 but we will need time for the telemetry metrics to settle before we know for sure. ## Summary This PR removes the conditional rendering of the default drag handle in `kbn-grid-layout`, which has two benefits: 1. It removes the double render of `GridPanel` that was caused by relying on the `dragHandleCount` to be updated in order to determine whether the default drag handle should be rendered 2. The default drag handle no longer "flashes" when Dashboards are loading and waiting for `dragHandleCount` to update - **Before:** https://github.com/user-attachments/assets/30a032fc-4df3-42ce-9494-dd7f69637c03 - **After:** https://github.com/user-attachments/assets/db447911-cbe2-40dd-9a07-405d1e35a75d Instead, the consumer of `kbn-grid-layout` is responsible for setting the `useCustomDragHandle` prop to `true` when they want to use a drag handle other than the default one. When adding the `useCustomDragHandle` prop, I got annoyed that I had to pass this prop all the way down to `grid_panel` - so I decided to swap to using React context in this PR, as well. The API for the grid layout component will most likely continue to grow, so this should make it easier to manage the props. ### 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 - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this issue
Feb 13, 2025
This PR **might** resolve elastic#207011 but we will need time for the telemetry metrics to settle before we know for sure. ## Summary This PR removes the conditional rendering of the default drag handle in `kbn-grid-layout`, which has two benefits: 1. It removes the double render of `GridPanel` that was caused by relying on the `dragHandleCount` to be updated in order to determine whether the default drag handle should be rendered 2. The default drag handle no longer "flashes" when Dashboards are loading and waiting for `dragHandleCount` to update - **Before:** https://github.com/user-attachments/assets/30a032fc-4df3-42ce-9494-dd7f69637c03 - **After:** https://github.com/user-attachments/assets/db447911-cbe2-40dd-9a07-405d1e35a75d Instead, the consumer of `kbn-grid-layout` is responsible for setting the `useCustomDragHandle` prop to `true` when they want to use a drag handle other than the default one. When adding the `useCustomDragHandle` prop, I got annoyed that I had to pass this prop all the way down to `grid_panel` - so I decided to swap to using React context in this PR, as well. The API for the grid layout component will most likely continue to grow, so this should make it easier to manage the props. ### 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 - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 200922a)
kibanamachine
added a commit
that referenced
this issue
Feb 13, 2025
…0463) (#211115) # Backport This will backport the following commits from `main` to `8.x`: - [[kbn-grid-layout] Add `useCustomDragHandle` prop (#210463)](#210463) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-02-13T22:18:00Z","message":"[kbn-grid-layout] Add `useCustomDragHandle` prop (#210463)\n\nThis PR **might** resolve\r\nhttps://github.com//issues/207011 but we will need time\r\nfor the telemetry metrics to settle before we know for sure.\r\n\r\n## Summary\r\n\r\nThis PR removes the conditional rendering of the default drag handle in\r\n`kbn-grid-layout`, which has two benefits:\r\n\r\n1. It removes the double render of `GridPanel` that was caused by\r\nrelying on the `dragHandleCount` to be updated in order to determine\r\nwhether the default drag handle should be rendered\r\n2. The default drag handle no longer \"flashes\" when Dashboards are\r\nloading and waiting for `dragHandleCount` to update\r\n \r\n - **Before:**\r\n \r\n\r\nhttps://github.com/user-attachments/assets/30a032fc-4df3-42ce-9494-dd7f69637c03\r\n \r\n - **After:**\r\n \r\n\r\nhttps://github.com/user-attachments/assets/db447911-cbe2-40dd-9a07-405d1e35a75d\r\n\r\n\r\nInstead, the consumer of `kbn-grid-layout` is responsible for setting\r\nthe `useCustomDragHandle` prop to `true` when they want to use a drag\r\nhandle other than the default one.\r\n\r\n\r\nWhen adding the `useCustomDragHandle` prop, I got annoyed that I had to\r\npass this prop all the way down to `grid_panel` - so I decided to swap\r\nto using React context in this PR, as well. The API for the grid layout\r\ncomponent will most likely continue to grow, so this should make it\r\neasier to manage the props.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"200922a5124d20c2d7ff6826a2c62a36befaa56c","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Dashboard","release_note:fix","Team:Presentation","loe:small","impact:high","Project:Collapsable Panels","backport:version","v9.1.0","v8.19.0"],"title":"[kbn-grid-layout] Add `useCustomDragHandle` prop","number":210463,"url":"https://github.com/elastic/kibana/pull/210463","mergeCommit":{"message":"[kbn-grid-layout] Add `useCustomDragHandle` prop (#210463)\n\nThis PR **might** resolve\r\nhttps://github.com//issues/207011 but we will need time\r\nfor the telemetry metrics to settle before we know for sure.\r\n\r\n## Summary\r\n\r\nThis PR removes the conditional rendering of the default drag handle in\r\n`kbn-grid-layout`, which has two benefits:\r\n\r\n1. It removes the double render of `GridPanel` that was caused by\r\nrelying on the `dragHandleCount` to be updated in order to determine\r\nwhether the default drag handle should be rendered\r\n2. The default drag handle no longer \"flashes\" when Dashboards are\r\nloading and waiting for `dragHandleCount` to update\r\n \r\n - **Before:**\r\n \r\n\r\nhttps://github.com/user-attachments/assets/30a032fc-4df3-42ce-9494-dd7f69637c03\r\n \r\n - **After:**\r\n \r\n\r\nhttps://github.com/user-attachments/assets/db447911-cbe2-40dd-9a07-405d1e35a75d\r\n\r\n\r\nInstead, the consumer of `kbn-grid-layout` is responsible for setting\r\nthe `useCustomDragHandle` prop to `true` when they want to use a drag\r\nhandle other than the default one.\r\n\r\n\r\nWhen adding the `useCustomDragHandle` prop, I got annoyed that I had to\r\npass this prop all the way down to `grid_panel` - so I decided to swap\r\nto using React context in this PR, as well. The API for the grid layout\r\ncomponent will most likely continue to grow, so this should make it\r\neasier to manage the props.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"200922a5124d20c2d7ff6826a2c62a36befaa56c"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/210463","number":210463,"mergeCommit":{"message":"[kbn-grid-layout] Add `useCustomDragHandle` prop (#210463)\n\nThis PR **might** resolve\r\nhttps://github.com//issues/207011 but we will need time\r\nfor the telemetry metrics to settle before we know for sure.\r\n\r\n## Summary\r\n\r\nThis PR removes the conditional rendering of the default drag handle in\r\n`kbn-grid-layout`, which has two benefits:\r\n\r\n1. It removes the double render of `GridPanel` that was caused by\r\nrelying on the `dragHandleCount` to be updated in order to determine\r\nwhether the default drag handle should be rendered\r\n2. The default drag handle no longer \"flashes\" when Dashboards are\r\nloading and waiting for `dragHandleCount` to update\r\n \r\n - **Before:**\r\n \r\n\r\nhttps://github.com/user-attachments/assets/30a032fc-4df3-42ce-9494-dd7f69637c03\r\n \r\n - **After:**\r\n \r\n\r\nhttps://github.com/user-attachments/assets/db447911-cbe2-40dd-9a07-405d1e35a75d\r\n\r\n\r\nInstead, the consumer of `kbn-grid-layout` is responsible for setting\r\nthe `useCustomDragHandle` prop to `true` when they want to use a drag\r\nhandle other than the default one.\r\n\r\n\r\nWhen adding the `useCustomDragHandle` prop, I got annoyed that I had to\r\npass this prop all the way down to `grid_panel` - so I decided to swap\r\nto using React context in this PR, as well. The API for the grid layout\r\ncomponent will most likely continue to grow, so this should make it\r\neasier to manage the props.\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n- [x] The PR description includes the appropriate Release Notes section,\r\nand the correct `release_note:*` label is applied per the\r\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"200922a5124d20c2d7ff6826a2c62a36befaa56c"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Hannah Mudge <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Introducing the new grid-layout engine introduced a rendering slowdown in some benchmarks (#205341)
Rendering time of a dashboard went up with about 300ms on average, which is about a 10% overall slowdown.
It seems this is relate do the new way how drag-handles are implemented.
@Heenawter in internal discussion shared:
cc @andreadelrio @ThomThomson
The text was updated successfully, but these errors were encountered: