[Cascade] Virtualizer Improvements#256699
Conversation
cdcb83b to
26bc412
Compare
a033e53 to
24d22bb
Compare
- structural changes to virtualizer implementation - favor anchor item index over scroll offset for scroll restoration - use cascade virtualizer for nesting in discover - deferred mounting for nested rows - add foundation for controlled child virtualizer - add orchestration for nested virtualized children - adjustments so returning expanded rows do not show loading skeleton - render a loader in place of white screen whilst cascade is bootstrapping
- scroll position restoration for nested virtualized grid - adjustments so scroll restoration matches previous row exactly
24d22bb to
6810255
Compare
|
@elasticmachine merge upstream |
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
|
@elasticmachine merge upstream |
davismcphee
left a comment
There was a problem hiding this comment.
Data Discovery code changes LGTM, and it feels like an improvement when testing, thanks!
| cellId, | ||
| rowIndex, | ||
| // @ts-expect-error - required to allow the use of the visibleRows array | ||
| rows: visibleRows, |
There was a problem hiding this comment.
I'm really not a fan of @ts-expect-error. Not a blocking issue or anything, but it tells me the types could be tightened up.
There was a problem hiding this comment.
Nice cleanup in this file 👍
| const updateESQLQuery = useCallback( | ||
| (...args: Parameters<typeof onUpdateESQLQuery>) => { | ||
| onUpdateESQLQuery(...args); | ||
| }, | ||
| [onUpdateESQLQuery] | ||
| ); |
There was a problem hiding this comment.
Why wrap onUpdateESQLQuery? It's in the dependency array anyway so I don't think there's any advantage to memoizing it.
There was a problem hiding this comment.
In the restoration PR we apply a condition here
kertal
left a comment
There was a problem hiding this comment.
Confirming that with this #255745 can be closed, even the huge lifting seems to have been done in previous PRs. Great work @eokoneyo.
The most significant change with this PR appears to be a further reduction the computation, compared to what we currently do in main
it's roughly 60% compared what we had before. I've tested loading and restoring a Group By Tab
All in all this make the experience a little bit faster, but as I said before, the main enhancements have been applied before
race-side-by-side-8.mov
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
cc @eokoneyo |
|
Starting backport for target branches: 9.3 https://github.com/elastic/kibana/actions/runs/23736598901 |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
## Summary This PR is part of the cascade performance improvement work(elastic#255745), culled from elastic#256037. Also resolves elastic#255745 In this PR the virtualizer implementation has been reworked, notable amongst the implementation change; - Introduces a ChildVirtualizerController that orchestrates nested virtualized lists within the data cascade, replacing the previous approach where each leaf cell independently managed its own useVirtualizer instance and manually wired scroll elements, margins, and offsets via individual props - Adds staggered activation for child virtualizers so expanded rows mount their heavy content (e.g. UnifiedDataTable) progressively rather than all at once, preventing main-thread stalls - introduces item-index-based anchoring (initialAnchorItemIndex), which survives row resizes and remeasurements more reliably than a raw pixel offset - Defers initial overscan to zero on mount and applies the configured value after the first paint, reducing DOM node count and layout thrashing during initial render - Exposes connectedChildren state and scrollAnchorItemIndex through the public API snapshot, enabling external consumers to observe nested virtualizer state. - Row measurements are now cached by the row key, this enables stable reference for previously measured rows reducing the style recalculation that needs to be performed when a new row gets rendered into the DOM on scroll - Lifts the skeleton/content gate from the consumer (ESQLDataCascadeLeafCell) into the library (CascadeRowCellPrimitive), so the skeleton is coordinated by the controller's activation signal rather than being opt-in per consumer ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. <!-- - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials --> - [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 <!-- - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] 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) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --> --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Matthias Polman <matthias.wilhelm@elastic.co>
## Summary This PR is part of the cascade performance improvement work(elastic#255745), culled from elastic#256037. Also resolves elastic#255745 In this PR the virtualizer implementation has been reworked, notable amongst the implementation change; - Introduces a ChildVirtualizerController that orchestrates nested virtualized lists within the data cascade, replacing the previous approach where each leaf cell independently managed its own useVirtualizer instance and manually wired scroll elements, margins, and offsets via individual props - Adds staggered activation for child virtualizers so expanded rows mount their heavy content (e.g. UnifiedDataTable) progressively rather than all at once, preventing main-thread stalls - introduces item-index-based anchoring (initialAnchorItemIndex), which survives row resizes and remeasurements more reliably than a raw pixel offset - Defers initial overscan to zero on mount and applies the configured value after the first paint, reducing DOM node count and layout thrashing during initial render - Exposes connectedChildren state and scrollAnchorItemIndex through the public API snapshot, enabling external consumers to observe nested virtualizer state. - Row measurements are now cached by the row key, this enables stable reference for previously measured rows reducing the style recalculation that needs to be performed when a new row gets rendered into the DOM on scroll - Lifts the skeleton/content gate from the consumer (ESQLDataCascadeLeafCell) into the library (CascadeRowCellPrimitive), so the skeleton is coordinated by the controller's activation signal rather than being opt-in per consumer ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. <!-- - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials --> - [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 <!-- - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] 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) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --> --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Matthias Polman <matthias.wilhelm@elastic.co>
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
4 similar comments
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
This PR is part of the cascade performance improvement work(#255745), culled from #256037.
Also resolves #255745
In this PR the virtualizer implementation has been reworked, notable amongst the implementation change;
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.