[EuiCode/Block] Avoid recomputing code syntax colors #7486

eokoneyo · 2024-01-25T11:34:36Z

Summary

Hi Friends 👋🏾

This PR results from elastic/kibana#174290 (comment). On looking into this I realized that a contributing factor to the delay in the rendering of the component happens because multiple calls are made to euiCodeSyntaxTokens, which in turn calls the method euiCodeSyntaxColors, within this method the colors for the tokens get recomputed every time. This change applies memoization to the euiCodeSyntaxColors function so that when the colour determinant hasn't changed we return the already computed result.

Before

After

cee-chen · 2024-01-25T19:16:43Z

src/components/code/code_syntax.styles.ts

+    if (cache[backgroundColor]) {
+      return cache[backgroundColor];
+    }


Unfortunately consumers (not Kibana specifically, but in general) can customize more than just the backgroundColor. They could, for example, also override any or all of the text colors used below, which then would not correctly update per-theme or per-override. So we need to cache/memoize the variables per theme rather than just per background color.

@eokoneyo I spiked out a local attempt at converting this util to a hook with useMemo(): main...cee-chen:eui:code/emotion-perf

Is there any chance you could give it a shot in Kibana and see if it also reduces renders/improves performance similar to what's in your current PR?

Unfortunately consumers (not Kibana specifically, but in general) can customize more than just the backgroundColor. They could, for example, also override any or all of the text colors used below, which then would not correctly update per-theme or per-override. So we need to cache/memoize the variables per theme rather than just per background color.

Thanks for providing this additional context, totally makes sense to me. I'll test out your referenced spike.

Hey @cee-chen, I finally got around to trying out the referenced spike, it does offer significant improvement to what the current state is and we get to keep the implementation native to react. see screenshot below;

Woohoo, that's super exciting!! Thank you so much for checking @eokoneyo! How would you like to proceed with this work? If you want, we can cherry-pick my branch into this branch/PR and I can approve and merge. Or alternatively, I can open a new PR.

@cee-chen given we are opting for the react implementation, I'm not sure I see any value in cherry-picking the work you've done into this PR especially that we can use it as is, I'd be happy to review once we have it up!!

Haha, I mostly want you to get credit for the commit! It was your impetus/idea after all :) I might take over this PR just to get it merged in if that's cool.

Totally fine by me, I do appreciate the consideration too. Thanks Cee!

This reverts commit cab955d.

…che-when-possible

…ible

cee-chen

Going to go ahead and merge this in since we've essentially had 2 devs test/look over it. Thanks again for bringing this to our attention Eyo! I'll try to take a look at other similar patterns/usages in EUI in the future.

kibanamachine · 2024-01-31T20:55:25Z

Preview staging links for this PR:

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

elasticmachine · 2024-01-31T20:58:31Z

💚 Build Succeeded

Buildkite Build
Commit: 4010bce

History

💚 Build #1312 succeeded 82911b0

`v92.2.1` ⏩ `v93.0.0` --- ## [`v93.0.0`](https://github.com/elastic/eui/releases/v93.0.0) **Bug fixes** - Fixed `EuiTextTruncate` component to clean up timer from side effect on unmount ([#7495](elastic/eui#7495)) **Breaking changes** - Removed deprecated `anchorClassName` prop from `EuiPopover`. Use `className` instead ([#7488](elastic/eui#7488)) - Removed deprecated `buttonRef` prop from `EuiPopover`. Use `popoverRef` instead ([#7488](elastic/eui#7488)) - Removed deprecated `toolTipTitle` and `toolTipPosition` props from `EuiContextMenuItem`. Use `toolTipProps.title` and `toolTipProps.position` instead ([#7489](elastic/eui#7489)) - Removed deprecated internal `setSelection` ref method from `EuiInMemoryTable` and `EuiBasicTable`. Use the new controlled `selection.selected` prop API instead. ([#7491](elastic/eui#7491)) - `EuiTourStep`'s `className` and `style` props now apply to the anchoring element instead of to the popover panel, to match `EuiPopover` behavior. ([#7497](elastic/eui#7497)) - Convert your existing usages to `panelClassName` and `panelStyle` respectively instead. **Performance** - Improved the amount of recomputed styles being generated by `EuiCode` and `EuiCodeBlock` ([#7486](elastic/eui#7486)) **CSS-in-JS conversions** - Converted `EuiSearchBar` to Emotion ([#7490](elastic/eui#7490)) - Converted `EuiEmptyPrompt` to Emotion ([#7494](elastic/eui#7494)) - Added `euiBorderColor` and `useEuiBorderColorCSS` style utilities ([#7494](elastic/eui#7494)) --------- Co-authored-by: Jon <[email protected]>

`v92.2.1` ⏩ `v93.0.0` --- ## [`v93.0.0`](https://github.com/elastic/eui/releases/v93.0.0) **Bug fixes** - Fixed `EuiTextTruncate` component to clean up timer from side effect on unmount ([elastic#7495](elastic/eui#7495)) **Breaking changes** - Removed deprecated `anchorClassName` prop from `EuiPopover`. Use `className` instead ([elastic#7488](elastic/eui#7488)) - Removed deprecated `buttonRef` prop from `EuiPopover`. Use `popoverRef` instead ([elastic#7488](elastic/eui#7488)) - Removed deprecated `toolTipTitle` and `toolTipPosition` props from `EuiContextMenuItem`. Use `toolTipProps.title` and `toolTipProps.position` instead ([elastic#7489](elastic/eui#7489)) - Removed deprecated internal `setSelection` ref method from `EuiInMemoryTable` and `EuiBasicTable`. Use the new controlled `selection.selected` prop API instead. ([elastic#7491](elastic/eui#7491)) - `EuiTourStep`'s `className` and `style` props now apply to the anchoring element instead of to the popover panel, to match `EuiPopover` behavior. ([elastic#7497](elastic/eui#7497)) - Convert your existing usages to `panelClassName` and `panelStyle` respectively instead. **Performance** - Improved the amount of recomputed styles being generated by `EuiCode` and `EuiCodeBlock` ([elastic#7486](elastic/eui#7486)) **CSS-in-JS conversions** - Converted `EuiSearchBar` to Emotion ([elastic#7490](elastic/eui#7490)) - Converted `EuiEmptyPrompt` to Emotion ([elastic#7494](elastic/eui#7494)) - Added `euiBorderColor` and `useEuiBorderColorCSS` style utilities ([elastic#7494](elastic/eui#7494)) --------- Co-authored-by: Jon <[email protected]>

avoid recomputing code syntax color if it happened already

cab955d

eokoneyo requested a review from a team as a code owner January 25, 2024 11:34

eokoneyo and others added 3 commits January 25, 2024 12:44

add changelog

82911b0

Wrap useMemo around code syntax CSS variables

3f4e9ac

Update EuiCode components to use/pass new hook to styles

8d666dd

cee-chen reviewed Jan 25, 2024

View reviewed changes

cee-chen mentioned this pull request Jan 26, 2024

[Emotion] Convert EuiSearchBar #7490

Merged

16 tasks

cee-chen changed the title ~~Avoid recomputing code syntax colors~~ [EuiCode/Block] Avoid recomputing code syntax colors Jan 31, 2024

cee-chen added 3 commits January 31, 2024 12:20

Revert "avoid recomputing code syntax color if it happened already"

16f5b05

This reverts commit cab955d.

Merge branch 'code/emotion-perf' into feat/return-code-syntax-from-ca…

172f4ec

…che-when-possible

Merge branch 'main' into feat/return-code-syntax-from-cache-when-poss…

90e160d

…ible

cee-chen approved these changes Jan 31, 2024

View reviewed changes

cee-chen enabled auto-merge (squash) January 31, 2024 20:24

changelog copy

4010bce

cee-chen force-pushed the feat/return-code-syntax-from-cache-when-possible branch from d7e4b18 to 4010bce Compare January 31, 2024 20:32

cee-chen merged commit 0abab26 into elastic:main Jan 31, 2024
7 checks passed

eokoneyo deleted the feat/return-code-syntax-from-cache-when-possible branch February 1, 2024 10:02

cee-chen mentioned this pull request Feb 5, 2024

Upgrade EUI to v93.0.0 elastic/kibana#176246

Merged

cee-chen added emotion performance labels Feb 14, 2024

cee-chen mentioned this pull request Feb 14, 2024

[Emotion] Create hook utility for memoizing component styles per-theme #7529

Merged

10 tasks

cee-chen mentioned this pull request Aug 6, 2024

[Emotion][microperf] Separate form max-width variable to its own utility #7948

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiCode/Block] Avoid recomputing code syntax colors #7486

[EuiCode/Block] Avoid recomputing code syntax colors #7486

eokoneyo commented Jan 25, 2024 •

edited

Loading

cee-chen Jan 25, 2024

eokoneyo Jan 26, 2024

eokoneyo Jan 30, 2024

cee-chen Jan 30, 2024

eokoneyo Jan 31, 2024

cee-chen Jan 31, 2024 •

edited

Loading

eokoneyo Feb 1, 2024

cee-chen left a comment

kibanamachine commented Jan 31, 2024

elasticmachine commented Jan 31, 2024

[EuiCode/Block] Avoid recomputing code syntax colors #7486

[EuiCode/Block] Avoid recomputing code syntax colors #7486

Conversation

eokoneyo commented Jan 25, 2024 • edited Loading

Summary

Before

After

cee-chen Jan 25, 2024

Choose a reason for hiding this comment

eokoneyo Jan 26, 2024

Choose a reason for hiding this comment

eokoneyo Jan 30, 2024

Choose a reason for hiding this comment

cee-chen Jan 30, 2024

Choose a reason for hiding this comment

eokoneyo Jan 31, 2024

Choose a reason for hiding this comment

cee-chen Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

eokoneyo Feb 1, 2024

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 31, 2024

elasticmachine commented Jan 31, 2024

💚 Build Succeeded

History

eokoneyo commented Jan 25, 2024 •

edited

Loading

cee-chen Jan 31, 2024 •

edited

Loading