[EuiTextTruncate] Performance improvements; Remove non-canvas rendering methods #7210

cee-chen · 2023-09-20T21:49:13Z

Summary

Related PRs: #7028 has several heavy performance-related discussions, and #7116 for initial component creation

This PR greatly improves the number of iterations that both DOM and Canvas truncation utils via a fairly simple approach: since we know 1. the available width, and 2. the calculable width of the full text, we can use those two widths to obtain an approximate ratio of characters to slice from the full text. This initial approximately truncated text should be slightly over the available width (due to the added ellipsis), and we can then resume our existing character-by-character removal logic until we detect that the text fits as expected, which should now take <10 iterations.

⚠️ As always, I strongly recommend following along by commit as I tried to separate out misc cleanup preferences/tasks from the main logic (2nd through 4th commit).

See below GitHub thread: This PR, by consensus, also removes DOM rendering completely in favor of canvas-only rendering.

Before

# of character by character iterations:

After

QA

text_truncate.spec.tsx passes as before (with no changes)
combo_box.spec.tsx truncation tests pass as before with no changes
All staging examples should behave the same as production
- Note: The "Truncation position" example has some minute character differences when at the bounds of including vs omitting the start/end ellipsis. I don't think it's significant enough to warrant deeper investigation, but LMK if you feel otherwise
http://eui.elastic.co/pr_7210/#/utilities/text-truncation#performance should feel significantly less laggy than production

General checklist

Browser QA
- Checked in Chrome, Safari, Edge, and Firefox
Docs site QA
- Updated documentation
Code quality checklist
- Added or updated jest and cypress tests
Release checklist
- A changelog entry exists and is marked appropriately.
- Revert [REVERT ME] commit

Prefer `+` concatenation over template literals, just feels easier to read + minor naming tweaks

- no longer building text scratch 🎉 - update `middle` to dogfood `startEndAt` function - use `alternating` logic from `middle` for more granular truncation (was previously truncating 2 chars at a time)

- allow testing greater than 100 lines + fix resize observer on load - initial logic is off due to font not yet loading in - update beta warning - ~100 lines is fairly snappy at this point, and # of characters should no longer be a factor

cee-chen · 2023-09-20T22:15:02Z

@markov00 @dej611 @tkajtoch As always, I'd greatly appreciate your feedback and any perf testing you feel like doing!

From a preliminary test, Firefox and Chrome/Edge now perform well when resizing 100 lines, even when using DOM rendering (previously was extremely sluggish).

Safari, unfortunately, still feels fairly laggy/choppy on DOM rendering at 100 lines whereas canvas rendering when resizing feels much better.

All browsers started to struggle around 500-1000 instances/usages. Not too sure what we could do there (vs putting the onus on the consumer to use virtualization).

cee-chen · 2023-09-20T22:20:05Z

Also, if any of you feel like you are a strong yes to either of the two below questions, please let me know!

Is performance still enough of a concern that we should the EuiTextTruncate component to default to canvas rendering instead of DOM rendering?

and

If yes to the above, should we also consider removing TruncationUtilsWithDOM entirely so consumers can't use it?
- One argument for keeping two methods - DOM rendering is more accurate than Canvas rendering (see Add EuiTextTruncate component #7116 (comment)), and some consumers may prefer accuracy to perf for, e.g. singleton use cases

dej611 · 2023-09-21T09:25:32Z

Is performance still enough of a concern that we should the EuiTextTruncate component to default to canvas rendering instead of DOM rendering?

To me the DOM implementation made sense only if the Canvas one wasn't available. Given that we have the Canvas one I think performance wise it does not make sense to have the DOM alternative.
I've tested the perf page with all three major browsers and the difference between the two, without virtualisation, are still huge when resizing on my machine (MacBook with M1 Pro).

If yes to the above, should we also consider removing TruncationUtilsWithDOM entirely so consumers can't use it?

To me the precision difference (sub-pixel, which translate to 1 or 2 chars difference at best) won't justify the performance penalty or the cose of manual handling of the virtualization of the wrapper component.

cee-chen · 2023-09-21T15:32:19Z

I've tested the perf page with all three major browsers and the difference between the two, without virtualisation, are still huge when resizing on my machine (MacBook with M1 Pro).

Ahh, that's valid, I keep forgetting I'm on a very fancy M2 now. We could just channel our inner game developers and tell everyone to upgrade their machines (joking, not serious at all )

So it sounds like 1 vote for removing the DOM utils entirely, which I'd be down for (re-implementing would be trivial if a consumer did have a request for it in the future, or we could easily suggest a custom implementation).

@tkajtoch, @markov00, any votes from you all for removing DOM utils vs keeping them?

dej611 · 2023-09-21T15:34:04Z

So it sounds like 1 vote for removing the canvas utils entirely, which I'd be down for (re-implementing would be trivial if a consumer did have a request for it in the future, or we could easily suggest a custom implementation).

You mean removing the DOM utils, right?

cee-chen · 2023-09-21T15:41:27Z

🤦 Yep, will edit. Sorry, early morning brain

tkajtoch · 2023-09-22T12:37:52Z

+1 to canvas rendering being the default and removing DOM utils

markov00 · 2023-09-22T12:58:50Z

good for me to remove the DOM calculation!

- underlying mechanism is better described in new comment above ratio utils

and make canvas the only rendering method + change class inheritance - I'm making canvas text utils its own separate import because there's another component in another PR that will be able to use it soon, that doesn't need truncation logic + remove `measurementRenderAPI` prop

+ revert debugging in source code

+ update copy/recommendations

kibanamachine · 2023-09-22T22:45:43Z

Preview staging links for this PR:

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

elasticmachine · 2023-09-22T22:52:09Z

💚 Build Succeeded

Buildkite Build
Commit: 1fda857

History

💚 Build #666 succeeded 2a8a1d0

tkajtoch

Thank you for removing DOM utils. The PR looks great!

cee-chen · 2023-09-25T17:22:02Z

Thanks everyone on this thread for the awesome feedback, recommendations, and advice. Really appreciated!

`v88.3.0`⏩`v88.5.0` closes #151514 --- ## [`88.5.0`](https://github.com/elastic/eui/tree/v88.5.0) - Updated `EuiCallOut` with a new `onDismiss` prop ([#7156](elastic/eui#7156)) - Added a new `renderCustomToolbar` prop to `EuiDataGrid`, which allows custom rendering of the toolbar. ([#7190](elastic/eui#7190)) - Added a new `allowResetButton` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows hiding the "Reset to default" button from the display settings popover. ([#7190](elastic/eui#7190)) - Added a new `additionalDisplaySettings` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows rendering extra settings inside the display settings popover. ([#7190](elastic/eui#7190)) - Updated `EuiDataGrid`'s toolbar display settings button icon ([#7190](elastic/eui#7190)) - Updated `EuiTextTruncate` with significantly improved iteration performance. Removed `measurementRenderAPI` prop, as `EuiTextTruncation` now only uses more performant canvas render API ([#7210](elastic/eui#7210)) - Updated `EuiPopover` with a new configurable `repositionToCrossAxis` prop ([#7211](elastic/eui#7211)) - Updated `EuiDatePicker` to support `compressed` input styling ([#7218](elastic/eui#7218)) - Added `gradient` and `palette` icon glyphs. ([#7220](elastic/eui#7220)) **Bug fixes** - Fixed `EuiPopover`'s missing animations on popover close ([#7211](elastic/eui#7211)) - Fixed `EuiInputPopover` anchoring to the wrong side and missing shadows on smaller screens ([#7211](elastic/eui#7211)) - Fixed `EuiSuperDatePicker` icon spacing on the quick select button ([#7217](elastic/eui#7217)) - Fixed a missing type in `EuiMarkdownEditor`'s default processing plugins ([#7221](elastic/eui#7221)) ## [`88.4.1`](https://github.com/elastic/eui/tree/v88.4.1) **Bug fixes** - Fixed missing `className`s on `EuiTextTruncate` ([#7212](elastic/eui#7212)) - Fixed `title`s on `EuiComboBox` dropdown options to always be present ([#7212](elastic/eui#7212)) - Fixed `EuiComboBox` truncation issues when search is an empty space ([#7212](elastic/eui#7212)) ## [`88.4.0`](https://github.com/elastic/eui/tree/v88.4.0) - Updated `EuiComboBox` to allow configuring text truncation behavior via `truncationProps`. These props can be set on the entire combobox as well as on on individual dropdown options. ([#7028](elastic/eui#7028)) - Updated `EuiInMemoryTable` with a new `searchFormat` prop (defaults to `eql`). When setting this prop to `text`, the built-in search bar will ignore EQL syntax and allow searching for plain strings with special characters and symbols. ([#7175](elastic/eui#7175)) **Bug fixes** - `EuiComboBox` now always shows the highlighted search text, even on truncated text ([#7028](elastic/eui#7028)) - Fixed missing i18n in `EuiSearchBar`'s default placeholder and aria-label text ([#7175](elastic/eui#7175)) - Fixed the inline compressed styles of `EuiDescriptionListTitle` to use a taller line-height for readability ([#7185](elastic/eui#7185)) - Fixed `EuiComboBox` to correctly truncate selected items when displayed as pills and plain text ([#7193](elastic/eui#7193)) **Accessibility** - Added `aria-current` attribute to `EuiTablePagination` ([#7186](elastic/eui#7186)) **CSS-in-JS conversions** - Converted `EuiDroppable` and `EuiDraggable` to Emotion; Removed `$euiDragAndDropSpacing` Sass variables ([#7187](elastic/eui#7187)) --------- Co-authored-by: Patryk Kopycinski <contact@patrykkopycinski.com> Co-authored-by: Jan Monschke <jan.monschke@elastic.co> Co-authored-by: Thomas Watson <watson@elastic.co>

cee-chen added 10 commits September 20, 2023 11:38

[misc minor cleanup] mostly around template literals

c39feee

Prefer `+` concatenation over template literals, just feels easier to read + minor naming tweaks

Set up internal ratio util helpers

1e279fb

Convert start, end, and middle methods to use new ratio utils

4bd1d90

Convert startEnd truncation position to use new ratio logic

f520bc6

- no longer building text scratch 🎉 - update `middle` to dogfood `startEndAt` function - use `alternating` logic from `middle` for more granular truncation (was previously truncating 2 chars at a time)

Remove now unused substring utils

7e76bd1

DRY out typical test assertions

a5ed6a2

[Optional] Add performance debug utilities/testing

7c4d709

[docs] Improve perf example

f91c032

- allow testing greater than 100 lines + fix resize observer on load - initial logic is off due to font not yet loading in - update beta warning - ~100 lines is fairly snappy at this point, and # of characters should no longer be a factor

[REVERT ME] enable DOM debug logging for QA

4b45dfc

changelog

2a8a1d0

cee-chen mentioned this pull request Sep 19, 2023

[EuiTextTruncate] Binary algorithm spike #7203

Closed

cee-chen requested review from dej611 and markov00 September 20, 2023 22:09

cee-chen marked this pull request as ready for review September 20, 2023 22:10

cee-chen requested a review from a team as a code owner September 20, 2023 22:10

cee-chen requested a review from tkajtoch September 20, 2023 22:10

cee-chen added 6 commits September 22, 2023 09:48

Merge branch 'main' into truncation-ratio

782f529

Remove now-inaccurate comment

966369f

- underlying mechanism is better described in new comment above ratio utils

[organization] move canvas utils before truncation utils

810583d

Update/fix tests & performance debugging

1d1506e

+ revert debugging in source code

Update docs to remove measurementRenderAPI

1fda857

+ update copy/recommendations

cee-chen changed the title ~~[EuiTextTruncate] Performance improvements~~ [EuiTextTruncate] Performance improvements; Remove non-canvas rendering methods Sep 22, 2023

tkajtoch approved these changes Sep 25, 2023

View reviewed changes

cee-chen merged commit 020905e into elastic:main Sep 25, 2023
7 checks passed

cee-chen deleted the truncation-ratio branch September 25, 2023 17:22

cee-chen mentioned this pull request Sep 25, 2023

Upgrade EUI to v88.5.0 elastic/kibana#166868

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiTextTruncate] Performance improvements; Remove non-canvas rendering methods #7210

[EuiTextTruncate] Performance improvements; Remove non-canvas rendering methods #7210

cee-chen commented Sep 20, 2023 •

edited

Loading

cee-chen commented Sep 20, 2023

cee-chen commented Sep 20, 2023

dej611 commented Sep 21, 2023

cee-chen commented Sep 21, 2023 •

edited

Loading

dej611 commented Sep 21, 2023

cee-chen commented Sep 21, 2023

tkajtoch commented Sep 22, 2023

markov00 commented Sep 22, 2023

kibanamachine commented Sep 22, 2023

elasticmachine commented Sep 22, 2023

tkajtoch left a comment

cee-chen commented Sep 25, 2023

[EuiTextTruncate] Performance improvements; Remove non-canvas rendering methods #7210

[EuiTextTruncate] Performance improvements; Remove non-canvas rendering methods #7210

Conversation

cee-chen commented Sep 20, 2023 • edited Loading

Summary

Before

After

QA

General checklist

cee-chen commented Sep 20, 2023

cee-chen commented Sep 20, 2023

dej611 commented Sep 21, 2023

cee-chen commented Sep 21, 2023 • edited Loading

dej611 commented Sep 21, 2023

cee-chen commented Sep 21, 2023

tkajtoch commented Sep 22, 2023

markov00 commented Sep 22, 2023

kibanamachine commented Sep 22, 2023

elasticmachine commented Sep 22, 2023

💚 Build Succeeded

History

tkajtoch left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 25, 2023

cee-chen commented Sep 20, 2023 •

edited

Loading

cee-chen commented Sep 21, 2023 •

edited

Loading