[EuiFlyout] Performance cleanup #7462

davismcphee · 2024-01-14T05:10:01Z

Summary

This PR includes two changes which optimize browser reflows when using EuiResizableFlyout, which has a significant impact on the performance of the flyout in heavy pages such as Discover:

Switched from the useResizeObserver hook to a ResizeObserver directly to track the flyout size, since the use of getBoundingClientRect in useResizeObserver causes layout thrashing when combined with the body padding modifications made by the flyout.
- No longer applies as of [EuiResizeObserver] Remove reflow/getBoundingClientRect() usage on resize #7575, but a small microperf addition has been made to not instantiate a resize observer at all for non-push flyouts
No longer toggle the euiBody--hasFlyout class, or remove/re-add body padding whenever the flyout size changes. Instead the class is only added/removed on mount/unmount, and the body padding is only updated on resize and removed on unmount.

In my tests of frantically resizing the Discover flyout for 10 seconds while profiling, these changes brought the time Chrome spent on Recalculate Style from 4.8s to 0.8s:

Unoptimized:

Optimized:

And a couple of videos from Discover for reference:

Unoptimized:

unoptimized.mp4

Optimized:

chromium.mp4

The optimizations also improve the performance in Firefox and Safari. Performance in Safari is still a bit slow compared to the other browsers, but it's acceptable and at least much better than before.

Firefox:

firefox.mp4

Safari:

safari.mp4

QA

General checklist

Browser QA
- Checked in both light and dark modes
- Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
  ~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
Docs site QA - N/A
Code quality checklist
- Added or updated jest and cypress tests
Release checklist
- A changelog entry exists and is marked appropriately.
  ~~- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)~~
Designer checklist - N/A

davismcphee · 2024-01-14T05:51:08Z

src/components/flyout/flyout.test.tsx

+        expect(add).toHaveBeenLastCalledWith('euiBody--hasFlyout');
+      });
+
+      it('should not remove and re-add "euiBody--hasFlyout" class on resize', async () => {


The changes are mainly an optimization which is sort of hard to test, so instead I wrote some tests for the euiBody--hasFlyout class and included this test case that would otherwise fail without the optimization.

cee-chen · 2024-02-05T20:20:25Z

Hey @davismcphee! Just wanted to check in on this PR - it looks great to me honestly, should I go ahead and mark its as ready for review + add a changelog to it? Happy to take over and bring it across the finish line if you're busy with other things!

kertal · 2024-02-07T09:27:13Z

We are currently evaluating EuiDataGrid for the UnifiedDocViewer, and created an instance which applies resizable flyouts and a basic migration of the DocViewer, to measure the performance impact

your can test is here
https://kertal-pr-176273-doc-viewer-push-and-eui-data-grid.kbndev.co/app/discover#/?_g=(filters:!(),refreshInterval:(pause:!t,value:60000),time:(from:now-15m,to:now))&_a=(columns:!(),filters:!(),index:ff959d40-b880-11e8-a6d9-e546fe2bba5f,interval:auto,query:(language:kuery,query:''),sort:!(!(order_date,desc)))

We merged also this fix in this deployment, it resolved the issue in Chrome 🥳 , but still, Firefox + Safari 😿 don't provide the expected performance, Chrome - Left, Firefox - Right

Kapture.2024-02-07.at.10.18.22.mp4

cee-chen · 2024-02-07T16:08:58Z

Adding a throttler/debouncer to the width changes would be the next step to improving performance - would that be something y'all would like to request as a feature/prop as well?

kertal · 2024-02-08T06:41:31Z

Adding a throttler/debouncer to the width changes would be the next step to improving performance - would that be something y'all would like to request as a feature/prop as well?

@cee-chen is this the reason for the performance difference between Chrome and the rest of the gang? If this is the case I think throttling resizing should be applied (not optional)

cee-chen · 2024-02-08T18:54:31Z

It's the most obvious approach that I can think of, but I have no idea if that's the difference between browsers, and it isn't guaranteed to fix the issue per se.

The other approach would be to check/optimize the number of rerenders on the rest of the page that's reflowing, as that's likely the culprit of repaint/reflow performance issues, but that's obviously far more involved & harder to do.

…and styles to be toggled multiple times each render, resulting in a lot of extra reflows in the browser

… prevent layout thrashin g when resizing

davismcphee · 2024-03-04T22:59:13Z

@cee-chen Apologies for taking so long getting back to you on this one! I finally had a chance to revisit this PR and track down the main culprit for the performance issue.

It turns out that in addition to toggling the euiBody--hasFlyout hurting performance, the most impactful issue was the useResizeObserver hook. Internally it uses getBoundingClientRect, which causes layout thrashing when combined with frequent writes to DOM elements, like when we set the body padding on resize. To fix it I updated EuiFlyout to use a ResizeObserver directly instead of depending on the hook, which resolves the performance issue in all browsers.

It looks like useResizeObserver relies on getBoundingClientRect because contentRect only provides the content sizing for elements instead of box sizing:

eui/src/components/observer/resize_observer/resize_observer.tsx

Lines 106 to 109 in 635f860

    
           // `entry.contentRect` provides incomplete `height` and `width` data. 
        
           // Use `getBoundingClientRect` to account for padding and border. 
        
           // https://developer.mozilla.org/en-US/docs/Web/API/DOMRectReadOnly 
        
           const { height, width } = container.getBoundingClientRect();

But I assume this code is old and isn't actually needed anymore since ResizeObserverEntry now supports borderBoxSize to access box sizing. So technically this could have been solved directly within useResizeObserver, but I ended up just addressing EuiFlyout since I wasn't sure if we'd be worried about the impact of changing the hook (although it should technically work the same).

cee-chen · 2024-03-07T20:03:58Z

It looks like useResizeObserver relies on getBoundingClientRect because contentRect only provides the content sizing for elements instead of box sizing:
[...]
But I assume this code is old and isn't actually needed anymore since ResizeObserverEntry now supports borderBoxSize to access box sizing. So technically this could have been solved directly within useResizeObserver, but I ended up just addressing EuiFlyout since I wasn't sure if we'd be worried about the impact of changing the hook (although it should technically work the same).

Super interesting, thanks for discovering this Davis! I actually think this is important enough to address at the useResizeObserver level rather than just for EuiFlyout. I'd be open to another PR that fixes this for all usages of our resize observer(s) as I think that should greatly improve performance across the board.

WDYT?

davismcphee · 2024-03-13T00:44:53Z

@cee-chen Thanks for opening #7575! I think it's a great idea to implement this at the resize observer level. We'll probably even see performance gains elsewhere in Kibana just from that.

For this PR, the resize observer change would of course need to be reverted, but would you be open to still taking the original euiBody--hasFlyout class optimization? It seemed less impactful in Firefox and Safari in my tests, but still had a noticeable improvement on performance in Chrome.

cee-chen · 2024-03-13T01:50:54Z

@davismcphee Yes, 100%, I definitely want that change! 😄

- add a ternary to *not* instantiate the resize observer if this isn't a push flyout (addresses the old TODO) - DRY out `paddingSide` logic more - move body className comment to multiline format (file organization)

- the body class code is now much simpler, so we can adjust tests accordingly

cee-chen · 2024-03-13T18:43:35Z

@davismcphee Alrighty, I've updated the PR with latest main and pushed up the minor changes I had - did you want to re-test the performance impacts to make sure everything's still shipshape, or is this good to merge in your opinion?

kibanamachine · 2024-03-13T19:09:59Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7462/
Storybook: https://eui.elastic.co/pr_7462/storybook

elasticmachine · 2024-03-13T19:10:03Z

💚 Build Succeeded

Buildkite Build
Commit: 313ae2d

History

💚 Build #1436 succeeded f85b22f
💚 Build #1432 succeeded 0624fc8
💔 Build #1431 failed ef70c47
💚 Build #1277 succeeded b6a5a04

davismcphee

@cee-chen I gave it another try in Discover with the resize observer fix and latest changes, and I can confirm performance is good in Chrome, Firefox, and Safari (relatively)!

davismcphee · 2024-03-14T04:21:57Z

src/components/flyout/flyout.tsx

+
+        document.body.style[paddingSide] = `${width}px`;
+        return () => {
+          document.body.style[paddingSide] = '';


I noticed while testing this gets called on each frame as the width changes when resizing, but it didn't seem to have much if any performance impact in Chrome, Firefox, or Safari.

Thanks a million for checking that Davis! 🙇

cee-chen

Merging - thanks again for your awesome work Davis, you rock!! ❤️‍🔥

`v93.3.0`⏩ `v93.4.0` --- ## [`v93.4.0`](https://github.com/elastic/eui/releases/v93.4.0) - Added the following properties to `EuiButtonGroup`'s `options` configs: `toolTipContent`, `toolTipProps`, and `title`. These new properties allow wrapping buttons in `EuiToolTips`, and additionally customizing or disabling the native browser `title` tooltip. ([#7461](elastic/eui#7461)) - Enhanced `EuiResizeObserver` and `useResizeObserver`'s performance to not trigger page reflows on resize event ([#7575](elastic/eui#7575)) - Updated `EuiSuperUpdateButton` to support custom button text via an optional `children` prop ([#7576](elastic/eui#7576)) **Bug fixes** - Fixed `EuiFlyout` to not repeatedly remove/add a body class on resize ([#7462](elastic/eui#7462)) - Fixed `EuiToast` title text to wrap instead of overflowing out of the container ([#7568](elastic/eui#7568)) - Fixed a visual bug with `EuiHeaderBreadcrumbs` with popovers ([#7580](elastic/eui#7580)) **Deprecations** - Deprecated `euiPalettePositive` and `euiPaletteNegative` in favour of a more culturally inclusive `euiPaletteGreen` and `euiPaletteRed` ([#7570](elastic/eui#7570)) - Deprecated all charts theme exports in favor of `@elastic/charts` exports: ([#7572](elastic/eui#7572)) - Deprecated `EUI_CHARTS_THEME_<DARK|LIGHT>` in favor of `<DARK|LIGHT>_THEME` from `@elastic/charts`. ([#7572](elastic/eui#7572)) - Deprecated `EUI_SPARKLINE_THEME_PARTIAL` in favor of `useSparklineOverrides` theme from the kibana `charts` plugin `theme` service. **Accessibility** - Updated `EuiModal` to set an `aria-modal` attribute and a default `dialog` role ([#7564](elastic/eui#7564)) - Updated `EuiConfirmModal` to set a default `alertdialog` role ([#7564](elastic/eui#7564)) - Fixed `EuiModal` and `EuiConfirmModal` to properly trap Safari+VoiceOver's virtual cursor ([#7564](elastic/eui#7564))

davismcphee commented Jan 14, 2024

View reviewed changes

davismcphee added 3 commits March 3, 2024 21:38

Fix performance issue in EuiResizableFlyout that would cause classes …

6b96eee

…and styles to be toggled multiple times each render, resulting in a lot of extra reflows in the browser

Add unit tests for euiBody--hasFlyout class

2f7db69

Replace flyout useResizeObserver call with ResizeObserver directly to…

ef70c47

… prevent layout thrashin g when resizing

davismcphee force-pushed the optimize-resizable-flyout-reflows branch from b6a5a04 to ef70c47 Compare March 4, 2024 05:01

davismcphee added 2 commits March 4, 2024 15:55

Check for ResizeObserver availability in flyout using hasResizeObserver

0624fc8

Added changelog entry

f85b22f

davismcphee force-pushed the optimize-resizable-flyout-reflows branch from 482b9ef to f85b22f Compare March 4, 2024 22:38

davismcphee marked this pull request as ready for review March 4, 2024 22:40

davismcphee requested a review from a team as a code owner March 4, 2024 22:40

cee-chen mentioned this pull request Mar 12, 2024

[EuiResizeObserver] Remove reflow/getBoundingClientRect() usage on resize #7575

Merged

4 tasks

cee-chen added 4 commits March 13, 2024 11:22

Merge branch 'main' into optimize-resizable-flyout-reflows

4750158

[PR updates] Return to using useResizeObserver + misc cleanup

904323b

- add a ternary to *not* instantiate the resize observer if this isn't a push flyout (addresses the old TODO) - DRY out `paddingSide` logic more - move body className comment to multiline format (file organization)

[PR feedback] Simplify tests

22d0659

- the body class code is now much simpler, so we can adjust tests accordingly

Changelog tweak to reflect new changes

313ae2d

cee-chen changed the title ~~[EuiResizableFlyout] Optimize resizable flyout browser reflows~~ [EuiFlyout] Performance cleanup Mar 13, 2024

cee-chen added tech debt performance labels Mar 13, 2024

davismcphee commented Mar 14, 2024

View reviewed changes

cee-chen approved these changes Mar 14, 2024

View reviewed changes

cee-chen merged commit f4e7570 into elastic:main Mar 14, 2024
11 checks passed

davismcphee deleted the optimize-resizable-flyout-reflows branch March 15, 2024 00:24

cee-chen mentioned this pull request Mar 18, 2024

Upgrade EUI to v93.4.0 elastic/kibana#178890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiFlyout] Performance cleanup #7462

[EuiFlyout] Performance cleanup #7462

davismcphee commented Jan 14, 2024 •

edited by cee-chen

Loading

davismcphee Jan 14, 2024 •

edited

Loading

cee-chen commented Feb 5, 2024

kertal commented Feb 7, 2024

cee-chen commented Feb 7, 2024

kertal commented Feb 8, 2024

cee-chen commented Feb 8, 2024

davismcphee commented Mar 4, 2024

cee-chen commented Mar 7, 2024

davismcphee commented Mar 13, 2024

cee-chen commented Mar 13, 2024

cee-chen commented Mar 13, 2024 •

edited

Loading

kibanamachine commented Mar 13, 2024

elasticmachine commented Mar 13, 2024

davismcphee left a comment

davismcphee Mar 14, 2024

cee-chen Mar 14, 2024

cee-chen left a comment •

edited

Loading

[EuiFlyout] Performance cleanup #7462

[EuiFlyout] Performance cleanup #7462

Conversation

davismcphee commented Jan 14, 2024 • edited by cee-chen Loading

Summary

QA

General checklist

davismcphee Jan 14, 2024 • edited Loading

Choose a reason for hiding this comment

cee-chen commented Feb 5, 2024

kertal commented Feb 7, 2024

cee-chen commented Feb 7, 2024

kertal commented Feb 8, 2024

cee-chen commented Feb 8, 2024

davismcphee commented Mar 4, 2024

cee-chen commented Mar 7, 2024

davismcphee commented Mar 13, 2024

cee-chen commented Mar 13, 2024

cee-chen commented Mar 13, 2024 • edited Loading

kibanamachine commented Mar 13, 2024

elasticmachine commented Mar 13, 2024

💚 Build Succeeded

History

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Mar 14, 2024

Choose a reason for hiding this comment

cee-chen Mar 14, 2024

Choose a reason for hiding this comment

cee-chen left a comment • edited Loading

Choose a reason for hiding this comment

davismcphee commented Jan 14, 2024 •

edited by cee-chen

Loading

davismcphee Jan 14, 2024 •

edited

Loading

cee-chen commented Mar 13, 2024 •

edited

Loading

cee-chen left a comment •

edited

Loading