Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rich text: Try debouncing useInput to improve performance and fix infinite loop #54819

Closed
wants to merge 2 commits into from

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Sep 26, 2023

What?

This PR attempts to debounce useInput inside the rich text component in order to resolve an issue when typing quickly. In my testing, it also improves the performance when typing quickly - the typed characters start appearing faster and there's way smaller delay between them.

Also related to #22822.

Why?

There are a few different problems this PR is attempting to improve:

1. React warning Maximum update depth exceeded error
When working with a large post, and typing very very fast inside a paragraph, we can see this error in the console:
Screenshot 2023-09-26 at 13 16 30
Since this will debounce the onInput() function, it resolves that issue since onInput() no longer calls that often when typing swiftly.

2. Typing performance
When working with a large post, and typing very very fast inside a paragraph, we can see that typing gets slower and the delay between each character that gets added gets bigger and bigger. I see a huge improvement when testing with this branch.

How?

We're simply debouncing useInput() so it won't be called that often, and that way it won't cause infinite loops and delayed typing feedback.

May be easier to review with ignored whitespace.

Testing Instructions

  • Start a new post and add this large post as its content.
  • Go to the bottom of the post and start a new paragraph.
  • Start typing jibberish as fast as you can.
  • Verify that you no longer get the error from above.
  • Verify that the delays of character appearance are significantly smaller or non-existent.
  • Verify all tests still pass.

Testing Instructions for Keyboard

None

Screenshots or screencast

None

@github-actions
Copy link

github-actions bot commented Sep 26, 2023

Size Change: +20.6 kB (+1%)

Total Size: 1.64 MB

Filename Size Change
build/block-directory/index.min.js 7.07 kB +15 B (0%)
build/block-editor/index.min.js 218 kB +257 B (0%)
build/block-editor/style-rtl.css 15.6 kB -2 B (0%)
build/block-editor/style.css 15.6 kB -3 B (0%)
build/block-library/blocks/image/style-rtl.css 1.4 kB -18 B (-1%)
build/block-library/blocks/image/style.css 1.39 kB -18 B (-1%)
build/block-library/blocks/image/view.min.js 1.83 kB +2 B (0%)
build/block-library/blocks/navigation-link/style-rtl.css 103 B -12 B (-10%) 👏
build/block-library/blocks/navigation-link/style.css 103 B -12 B (-10%) 👏
build/block-library/blocks/navigation/style-rtl.css 2.26 kB +13 B (+1%)
build/block-library/blocks/navigation/style.css 2.25 kB +13 B (+1%)
build/block-library/blocks/post-featured-image/style-rtl.css 345 B +23 B (+7%) 🔍
build/block-library/blocks/post-featured-image/style.css 345 B +23 B (+7%) 🔍
build/block-library/blocks/post-template/style-rtl.css 409 B +95 B (+30%) 🚨
build/block-library/blocks/post-template/style.css 408 B +94 B (+30%) 🚨
build/block-library/blocks/search/style-rtl.css 613 B +9 B (+1%)
build/block-library/blocks/search/style.css 613 B +9 B (+1%)
build/block-library/index.min.js 207 kB +349 B (0%)
build/block-library/style-rtl.css 14.1 kB +77 B (+1%)
build/block-library/style.css 14.1 kB +75 B (+1%)
build/blocks/index.min.js 51.5 kB +7 B (0%)
build/commands/index.min.js 15.5 kB +6 B (0%)
build/components/index.min.js 248 kB +1.57 kB (+1%)
build/components/style-rtl.css 11.9 kB +95 B (+1%)
build/components/style.css 11.9 kB +96 B (+1%)
build/compose/index.min.js 12.7 kB +1 B (0%)
build/core-commands/index.min.js 2.72 kB +101 B (+4%)
build/core-data/index.min.js 70.5 kB +58 B (0%)
build/customize-widgets/index.min.js 12 kB +11 B (0%)
build/data/index.min.js 8.78 kB -84 B (-1%)
build/edit-post/index.min.js 35.6 kB -1 B (0%)
build/edit-post/style-rtl.css 7.89 kB -34 B (0%)
build/edit-post/style.css 7.88 kB -34 B (0%)
build/edit-site/index.min.js 203 kB +17.3 kB (+9%) 🔍
build/edit-site/style-rtl.css 14.1 kB +157 B (+1%)
build/edit-site/style.css 14.1 kB +163 B (+1%)
build/edit-widgets/index.min.js 17 kB +18 B (0%)
build/editor/index.min.js 45.9 kB -1 B (0%)
build/format-library/index.min.js 7.79 kB +36 B (0%)
build/keyboard-shortcuts/index.min.js 1.76 kB +18 B (+1%)
build/list-reusable-blocks/index.min.js 2.21 kB +11 B (0%)
build/nux/index.min.js 2.01 kB +8 B (0%)
build/patterns/index.min.js 3.57 kB +8 B (0%)
build/plugins/index.min.js 1.81 kB +8 B (0%)
build/preferences/index.min.js 1.26 kB +3 B (0%)
build/react-i18n/index.min.js 631 B +7 B (+1%)
build/reusable-blocks/index.min.js 2.73 kB +9 B (0%)
build/rich-text/index.min.js 10.3 kB +44 B (0%)
build/router/index.min.js 1.79 kB +8 B (0%)
build/server-side-render/index.min.js 1.96 kB +11 B (+1%)
build/viewport/index.min.js 967 B -1 B (0%)
build/widgets/index.min.js 7.18 kB +9 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 964 B
build/annotations/index.min.js 2.71 kB
build/api-fetch/index.min.js 2.29 kB
build/autop/index.min.js 2.11 kB
build/blob/index.min.js 461 B
build/block-directory/style-rtl.css 1.04 kB
build/block-directory/style.css 1.04 kB
build/block-editor/content-rtl.css 4.28 kB
build/block-editor/content.css 4.27 kB
build/block-editor/default-editor-styles-rtl.css 403 B
build/block-editor/default-editor-styles.css 403 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 90 B
build/block-library/blocks/archives/style.css 90 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/audio/theme-rtl.css 138 B
build/block-library/blocks/audio/theme.css 138 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/avatar/style-rtl.css 104 B
build/block-library/blocks/avatar/style.css 104 B
build/block-library/blocks/block/editor-rtl.css 305 B
build/block-library/blocks/block/editor.css 305 B
build/block-library/blocks/button/editor-rtl.css 587 B
build/block-library/blocks/button/editor.css 587 B
build/block-library/blocks/button/style-rtl.css 633 B
build/block-library/blocks/button/style.css 632 B
build/block-library/blocks/buttons/editor-rtl.css 337 B
build/block-library/blocks/buttons/editor.css 337 B
build/block-library/blocks/buttons/style-rtl.css 332 B
build/block-library/blocks/buttons/style.css 332 B
build/block-library/blocks/calendar/style-rtl.css 239 B
build/block-library/blocks/calendar/style.css 239 B
build/block-library/blocks/categories/editor-rtl.css 113 B
build/block-library/blocks/categories/editor.css 112 B
build/block-library/blocks/categories/style-rtl.css 124 B
build/block-library/blocks/categories/style.css 124 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 121 B
build/block-library/blocks/code/style.css 121 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 421 B
build/block-library/blocks/columns/style.css 421 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 199 B
build/block-library/blocks/comment-template/style.css 198 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 840 B
build/block-library/blocks/comments/editor.css 839 B
build/block-library/blocks/comments/style-rtl.css 637 B
build/block-library/blocks/comments/style.css 636 B
build/block-library/blocks/cover/editor-rtl.css 647 B
build/block-library/blocks/cover/editor.css 650 B
build/block-library/blocks/cover/style-rtl.css 1.7 kB
build/block-library/blocks/cover/style.css 1.69 kB
build/block-library/blocks/details/editor-rtl.css 65 B
build/block-library/blocks/details/editor.css 65 B
build/block-library/blocks/details/style-rtl.css 98 B
build/block-library/blocks/details/style.css 98 B
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/embed/theme-rtl.css 138 B
build/block-library/blocks/embed/theme.css 138 B
build/block-library/blocks/file/editor-rtl.css 316 B
build/block-library/blocks/file/editor.css 316 B
build/block-library/blocks/file/style-rtl.css 311 B
build/block-library/blocks/file/style.css 312 B
build/block-library/blocks/file/view.min.js 321 B
build/block-library/blocks/footnotes/style-rtl.css 201 B
build/block-library/blocks/footnotes/style.css 199 B
build/block-library/blocks/freeform/editor-rtl.css 2.61 kB
build/block-library/blocks/freeform/editor.css 2.61 kB
build/block-library/blocks/gallery/editor-rtl.css 957 B
build/block-library/blocks/gallery/editor.css 962 B
build/block-library/blocks/gallery/style-rtl.css 1.55 kB
build/block-library/blocks/gallery/style.css 1.55 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 654 B
build/block-library/blocks/group/editor.css 654 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 189 B
build/block-library/blocks/heading/style.css 189 B
build/block-library/blocks/html/editor-rtl.css 340 B
build/block-library/blocks/html/editor.css 341 B
build/block-library/blocks/image/editor-rtl.css 834 B
build/block-library/blocks/image/editor.css 833 B
build/block-library/blocks/image/theme-rtl.css 137 B
build/block-library/blocks/image/theme.css 137 B
build/block-library/blocks/latest-comments/style-rtl.css 357 B
build/block-library/blocks/latest-comments/style.css 357 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 478 B
build/block-library/blocks/latest-posts/style.css 478 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 505 B
build/block-library/blocks/media-text/style.css 503 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 671 B
build/block-library/blocks/navigation-link/editor.css 672 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation/editor-rtl.css 2.26 kB
build/block-library/blocks/navigation/editor.css 2.26 kB
build/block-library/blocks/navigation/view.min.js 1.01 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 401 B
build/block-library/blocks/page-list/editor.css 401 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 235 B
build/block-library/blocks/paragraph/editor.css 235 B
build/block-library/blocks/paragraph/style-rtl.css 335 B
build/block-library/blocks/paragraph/style.css 335 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 508 B
build/block-library/blocks/post-comments-form/style.css 508 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 71 B
build/block-library/blocks/post-excerpt/editor.css 71 B
build/block-library/blocks/post-excerpt/style-rtl.css 141 B
build/block-library/blocks/post-excerpt/style.css 141 B
build/block-library/blocks/post-featured-image/editor-rtl.css 588 B
build/block-library/blocks/post-featured-image/editor.css 586 B
build/block-library/blocks/post-navigation-link/style-rtl.css 215 B
build/block-library/blocks/post-navigation-link/style.css 214 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-terms/style-rtl.css 96 B
build/block-library/blocks/post-terms/style.css 96 B
build/block-library/blocks/post-time-to-read/style-rtl.css 69 B
build/block-library/blocks/post-time-to-read/style.css 69 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 125 B
build/block-library/blocks/preformatted/style.css 125 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 335 B
build/block-library/blocks/pullquote/style.css 335 B
build/block-library/blocks/pullquote/theme-rtl.css 168 B
build/block-library/blocks/pullquote/theme.css 168 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 288 B
build/block-library/blocks/query-pagination/style.css 284 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 486 B
build/block-library/blocks/query/editor.css 486 B
build/block-library/blocks/query/style-rtl.css 375 B
build/block-library/blocks/query/style.css 372 B
build/block-library/blocks/query/view.min.js 609 B
build/block-library/blocks/quote/style-rtl.css 222 B
build/block-library/blocks/quote/style.css 222 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 149 B
build/block-library/blocks/rss/editor.css 149 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 184 B
build/block-library/blocks/search/editor.css 184 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/search/view.min.js 471 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 234 B
build/block-library/blocks/separator/style.css 234 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 329 B
build/block-library/blocks/shortcode/editor.css 329 B
build/block-library/blocks/site-logo/editor-rtl.css 760 B
build/block-library/blocks/site-logo/editor.css 760 B
build/block-library/blocks/site-logo/style-rtl.css 204 B
build/block-library/blocks/site-logo/style.css 204 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 116 B
build/block-library/blocks/site-title/editor.css 116 B
build/block-library/blocks/site-title/style-rtl.css 57 B
build/block-library/blocks/site-title/style.css 57 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 682 B
build/block-library/blocks/social-links/editor.css 681 B
build/block-library/blocks/social-links/style-rtl.css 1.45 kB
build/block-library/blocks/social-links/style.css 1.45 kB
build/block-library/blocks/spacer/editor-rtl.css 359 B
build/block-library/blocks/spacer/editor.css 359 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 432 B
build/block-library/blocks/table/editor.css 432 B
build/block-library/blocks/table/style-rtl.css 646 B
build/block-library/blocks/table/style.css 645 B
build/block-library/blocks/table/theme-rtl.css 157 B
build/block-library/blocks/table/theme.css 157 B
build/block-library/blocks/tag-cloud/style-rtl.css 251 B
build/block-library/blocks/tag-cloud/style.css 253 B
build/block-library/blocks/template-part/editor-rtl.css 403 B
build/block-library/blocks/template-part/editor.css 403 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/term-description/style-rtl.css 111 B
build/block-library/blocks/term-description/style.css 111 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 99 B
build/block-library/blocks/verse/style.css 99 B
build/block-library/blocks/video/editor-rtl.css 552 B
build/block-library/blocks/video/editor.css 555 B
build/block-library/blocks/video/style-rtl.css 191 B
build/block-library/blocks/video/style.css 191 B
build/block-library/blocks/video/theme-rtl.css 139 B
build/block-library/blocks/video/theme.css 139 B
build/block-library/classic-rtl.css 179 B
build/block-library/classic.css 179 B
build/block-library/common-rtl.css 1.11 kB
build/block-library/common.css 1.11 kB
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/editor-rtl.css 12.2 kB
build/block-library/editor.css 12.2 kB
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 472 B
build/block-library/reset.css 472 B
build/block-library/theme-rtl.css 700 B
build/block-library/theme.css 705 B
build/block-serialization-default-parser/index.min.js 1.13 kB
build/block-serialization-spec-parser/index.min.js 2.87 kB
build/commands/style-rtl.css 947 B
build/commands/style.css 942 B
build/customize-widgets/style-rtl.css 1.51 kB
build/customize-widgets/style.css 1.5 kB
build/data-controls/index.min.js 651 B
build/date/index.min.js 17.9 kB
build/deprecated/index.min.js 462 B
build/dom-ready/index.min.js 336 B
build/dom/index.min.js 4.68 kB
build/edit-post/classic-rtl.css 571 B
build/edit-post/classic.css 571 B
build/edit-widgets/style-rtl.css 4.84 kB
build/edit-widgets/style.css 4.84 kB
build/editor/style-rtl.css 3.58 kB
build/editor/style.css 3.58 kB
build/element/index.min.js 4.87 kB
build/escape-html/index.min.js 548 B
build/format-library/style-rtl.css 577 B
build/format-library/style.css 577 B
build/hooks/index.min.js 1.57 kB
build/html-entities/index.min.js 454 B
build/i18n/index.min.js 3.61 kB
build/interactivity/index.min.js 11.4 kB
build/is-shallow-equal/index.min.js 535 B
build/keycodes/index.min.js 1.9 kB
build/list-reusable-blocks/style-rtl.css 865 B
build/list-reusable-blocks/style.css 865 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 964 B
build/nux/style-rtl.css 775 B
build/nux/style.css 771 B
build/patterns/style-rtl.css 325 B
build/patterns/style.css 325 B
build/preferences-persistence/index.min.js 1.85 kB
build/primitives/index.min.js 994 B
build/priority-queue/index.min.js 1.52 kB
build/private-apis/index.min.js 972 B
build/react-refresh-entry/index.min.js 9.46 kB
build/react-refresh-runtime/index.min.js 6.78 kB
build/redux-routine/index.min.js 2.71 kB
build/reusable-blocks/style-rtl.css 265 B
build/reusable-blocks/style.css 265 B
build/shortcode/index.min.js 1.4 kB
build/style-engine/index.min.js 1.98 kB
build/token-list/index.min.js 587 B
build/url/index.min.js 3.84 kB
build/vendors/inert-polyfill.min.js 2.48 kB
build/vendors/react-dom.min.js 41.8 kB
build/vendors/react.min.js 4.02 kB
build/warning/index.min.js 259 B
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.18 kB
build/wordcount/index.min.js 1.03 kB

compressed-size-action

@jsnajdr
Copy link
Member

jsnajdr commented Sep 26, 2023

Adding debounce seems to me like the kind of fix that sweeps the issue under the rug, rather than addressing the real cause of it. We should know how exactly does the input event cause the recursive updates and the "Maximum update depth exceeded" error. This is not something that just typically happens, there must be some bug in the useInputAndSelection and useInputRules logic. We don't know what the bug is and are not addressing it.

@github-actions
Copy link

github-actions bot commented Sep 26, 2023

Flaky tests detected in 4ed8337.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6406267620
📝 Reported issues:

@Mamaduka
Copy link
Member

The change is also causing e2e test failures.

@tyxla
Copy link
Member Author

tyxla commented Sep 26, 2023

Agreed, it's far from ideal as a solution. I haven't been able to find a better one for the time being, but I'm happy to look further. One alternative could be optimizing how we treat batched store updates since it appears that is where the problem comes from. Let me know if you have any ideas in the meantime, @jsnajdr and @Mamaduka

@Mamaduka
Copy link
Member

Another debounce issue is that we must do it every time, even when a post is small or the user's device is powerful enough to handle the updates. We'll have to introduce a tiny delay for all these cases.

IIRC, React introduced deferred value and transition hooks to handle similar cases, but those probably aren't suitable for our case.

As @jsnajdr mentioned, eliminating all possible bottlenecks before we reach out for debounce/throttle is probably a better solution.

P.S. I tried to analyze performance calls for RichText before without any luck 😅 I understand this isn't easy to track down and fix.

@jsnajdr
Copy link
Member

jsnajdr commented Sep 27, 2023

Let me know if you have any ideas in the meantime

My first question would be, are there any reliable steps to reproduce? When typing into a paragraph block, for me personally, the editor is able to keep up with my typing speed very well 🙂 Does "typing very very fast" mean a "human" typing speed, or do I need a machine (e2e test) to type fast enough?

@tyxla
Copy link
Member Author

tyxla commented Sep 27, 2023

My first question would be, are there any reliable steps to reproduce? When typing into a paragraph block, for me personally, the editor is able to keep up with my typing speed very well 🙂 Does "typing very very fast" mean a "human" typing speed, or do I need a machine (e2e test) to type fast enough?

Yes, I have added some very reliable steps to reproduce it in the PR description. The caveat is that you need a very large post, and only then when QUICKLY typing over 50-60 characters, you'll see the error occur. We could be able to achieve it with an e2e test too, since it's fairly straightforward to start with a large post and type quickly there.

P.S. I tried to analyze performance calls for RichText before without any luck 😅 I understand this isn't easy to track down and fix.

Yeah, I think it could potentially be a complex one, and I have limited capacity over the coming weeks, but I'll try to find a solid time slot to debug it more thoroughly.

@Mamaduka
Copy link
Member

I can consistently reproduce React warnings even on small posts when throttling the CPU (4x) and trying to type fast. Based on the trace, I think the error is triggered by useSyncExternalStore, even though the message says useEffect.

@dcalhoun
Copy link
Member

dcalhoun commented Sep 27, 2023

I can consistently reproduce React warnings even on small posts when throttling the CPU (4x) and trying to type fast. Based on the trace, I think the error is triggered by useSyncExternalStore, even though the message says useEffect.

FWIW the editor in the WordPress mobile app experiences typing performance degradations on lower powered devices and/or while typing swiftly. Mobile devices often have less capable CPUs.

Our research findings on this subject also appeared to suggest that updating the global store — which I believe occurs on every keystroke — was the primary culprit for the performance issues when typing in the mobile editor. Notably, re-renders resulting from store updates were seemingly not the issue, but merely updating the global store itself.

We briefly explored debouncing the store updates, but encountered issues with rich text values becoming out of date or incorrect. Those issues likely stemmed from complexities in the mobile editor Rich Text implementation and reliance upon/communication loops with the Aztec editor, which is not to applicable to the web editor.

@tyxla
Copy link
Member Author

tyxla commented Sep 28, 2023

I can consistently reproduce React warnings even on small posts when throttling the CPU (4x) and trying to type fast. Based on the trace, I think the error is triggered by useSyncExternalStore, even though the message says useEffect.

Interesting, thanks! I know that solving these kinds of issues has been why useSyncExternalStoreWithSelector exists in the first place, but I'm not sure React exposes it anymore as a first-class API. Anyway, it could be something to consider as we're digging to find out the root cause.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 2, 2023

I can confirm that the issue is very easy to reproduce with a large post and 4x CPU throttling. I think this is almost certainly a bug in the useBlockSync hook. It's a very complex hook that I never really understood.

@tyxla
Copy link
Member Author

tyxla commented Oct 4, 2023

I did a short timeboxed session today, and while I opened an unrelated improvement (#55041), I haven't seen indications that useBlockSync could be the culprit. To me, it seems like it's a combination of updates and force rerenders, where the updates are synced to the external store, but new updates start coming in before the previous ones have been processed. The only way I managed to get it working within my time limit was by throttling/debouncing in a few other locations.

The fun part is that it works well for me locally even if I use a very low debounce wait time. I've pushed a commit just to try out all tests and see the impact.

In the meantime, though, I think the solution might be to introduce some sort of throttling mechanism at the lower levels so the updates aren't called that often, or they aren't called if they haven't been processed just yet. Will try to devote some more time, but it might be next week. In the meantime, I know Jarda wanted to dig further into useBlockSync as the potential cause, so I remain curious about how that investigation will go and what we'll find out.

@ellatrix
Copy link
Member

ellatrix commented Oct 4, 2023

Adding debounce seems to me like the kind of fix that sweeps the issue under the rug, rather than addressing the real cause of it. We should know how exactly does the input event cause the recursive updates and the "Maximum update depth exceeded" error. This is not something that just typically happens, there must be some bug in the useInputAndSelection and useInputRules logic. We don't know what the bug is and are not addressing it.

+ 100! Think of rich text as native text areas. Would you be debouncing onChange? The performance problems are not RichText. Maybe debounce all store changes instead? I would prefer a more general solution and leaving RichText alone.

@tyxla tyxla added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Oct 5, 2023
@tyxla
Copy link
Member Author

tyxla commented Oct 5, 2023

  • 100! Think of rich text as native text areas. Would you be debouncing onChange? The performance problems are not RichText. Maybe debounce all store changes instead? I would prefer a more general solution and leaving RichText alone.

Thanks for the feedback!

I think we all agree and will seek a different alternative. I'm keeping this PR open only for reference at this point.

Ideally, we'll be able to trace this to a bug with @wordpress/data, but let's see what comes up.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 9, 2023

I did some investigation today and found that:

useBlockSync is OK, my initial guess was wrong. When typing, the work that useBlockSync does is very simple, it merely calls the onChange callback on every keystroke, triggering an editEntityRecord call with the new content.

The thing that seems to cause the performance issues is the fact that on a large post with 1000 blocks, there are 80000 listeners on the core/block-editor store. That's 80 listeners (corresponding approximately to 80 useSelect usages) for every block. Even a simple one like core/paragraph. Intuitively that looks unreasonable, we should be able to significantly reduce that number.

There is async mode in useSelect, and most of the blocks are in that mode, but for some reason that doesn't have much impact. On each keystroke there are 80k callback enqueued into the async priority queue, and processing that queue takes approx 20 animation frames (16 ms each).

When profiling, I noticed two other things that stand out there, although they are unlikely to have impact on this specific issue:

  1. The updateFootnotesFromMeta function is called on every keystroke, when preparing the arguments for editEntityRecord. Do we really need to do that? Could we possibly postpone this calculation, like we postpone calculating content by wrapping it in a lambda function? There's a lot of caching there, but still there remains a .flatMap call over a 1000-element array, and that's quite significant work.
  2. The getValueFromObjectPath helper, replacement for _.get. Not only we have 6 implementations, each of them slightly different, but many of them do path.split('.') on every call. That can get expensive when called 100k times. Lodash had a cache for that inside _.get.

@youknowriad
Copy link
Contributor

There is async mode in useSelect, and most of the blocks are in that mode, but for some reason that doesn't have much impact. On each keystroke there are 80k callback enqueued into the async priority queue, and processing that queue takes approx 20 animation frames (16 ms each).

The animation frames (well not really more idle callbacks) are non blocking, if the user continues to type, they get pushed to later. If you try without the Async mode, typing is actually way worse.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 10, 2023

If you try without the Async mode, typing is actually way worse.

Yes, that's true, I wanted to say something else: that the typing is slow on the 1000-blocks post even with almost all the blocks in async mode. In other words, only the handful visible blocks are sufficient to make it slow.

The 80000 store subscriptions are caused mostly by the useSetting hook. It's used very liberally, to fetch just one individual setting, and code like this is common:

const customColors = useSetting( 'color.palette.custom' );
const themeColors = useSetting( 'color.palette.theme' );
const defaultColors = useSetting( 'color.palette.default' );
const shouldDisplayDefaultColors = useSetting( 'color.defaultPalette' );

const customGradients = useSetting( 'color.gradients.custom' );
const themeGradients = useSetting( 'color.gradients.theme' );
const defaultGradients = useSetting( 'color.gradients.default' );
const shouldDisplayDefaultGradients = useSetting( 'color.defaultGradients' );

there's also the useBlockSettings hook that does 40 individual useSetting calls and then combines the results into a settings object.

Each useSetting instance has a useSelect inside, and creates its own core/block-editor subscription. And as each block is reading a lot of settings, the subscriptions quickly add up.

The mapSelect function in useSetting is not trivial, it traverses through parent blocks, constructs path strings for property access. Quite expensive to call it 80k times on every store change. Here we're reaching the limits of the Redux architecture.

There are two ways how to move forward:

  1. Replace the useSetting hook with something else, something that doesn't create so many subscriptions. A useSettings hook comes to mind, one that could read multiple values at once. That could reduce the number of subscriptions by 2x or 4x. Still in the tens-of-thousands magnitude though.
  2. Optimize the useSetting hook to perform faster. Don't construct new objects (strings, arrays) unless really necessary, and try to turn it into a pure reader of things.

@dmsnell
Copy link
Member

dmsnell commented Oct 11, 2023

this is all interesting work, because I've been hunting for a long time major debounce-related issues and the source of tens of thousands of listeners that run after keypresses.

I've recorded video of the block editor (with third-party extensions) taking 112s to enter the characters I've typed, after I stop typing.

one thing that surprised me was that I found out that often the mouse and sometimes an enter keypress get handled instantly. in fact, in the situations I'm experiencing this latency, I can click into another paragraph or UI element and the keypresses will continue to pour out. there are certain components, however, that flush out all remaining keypresses in an instant: the block inserter search box is one such element. if I have keypresses backed up and they are slowly flushing out, then click in the search box in the block inserter, they all appear simultaneously and the search runs.

while I haven't been able to catch this in a vanilla build of Gutenberg, I wonder if you think this is likely the same problem. I've been suspecting that something is queueing up keypresses to hide other performance issues, but once it starts queuing faster than it can flush things get really bad really quickly.

@youknowriad
Copy link
Contributor

@dmsnell useSelect and the priority queue is written in a way that if a component queue two useSelect calls, the previous one that was waiting is just discarded. So (in theory at least), the list of queued items can't grow indefinitely.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 13, 2023

I've been suspecting that something is queueing up keypresses to hide other performance issues, but once it starts queuing faster than it can flush things get really bad really quickly.

I don't understand the exact differences between mouse events and key events and why some are handled instantly and some not. I believe the editor is too complex for this to be tractable.

What I found to be a problem is that we have approx 80 block-editor store subscriptions per block. And that's too much. It's unreasonable, a typical block doesn't do 80 things.

For a post with 1000 blocks, this leads to calling 80k store listeners on each action dispatch. The fact that the vast majority of blocks is in async mode, it doesn't help as much as one would expect. Even adding a new item to the priority queue is not a trivial operation, it is a .set operation on a Map with 80k items. Running synchronously a simple useSelect callback and checking its result is not that much slower. Look at this flamechart from profiler:

Screenshot 2023-10-13 at 10 59 34

It's a recording of the loop that calls the 80k listeners. The light green stripes are calls to renderQueue.add, most of the time is spent there. The light yellow stripes are where the changes are checked synchronously for the visible blocks (call the useSelect callbacks and shallow compare with previous results).

So, what we need to do is to reduce these 80k listeners to some smaller number.

One approach would be a major architecture overhaul. Don't subscribe to the entire core/block-editor store, but create a way to subscribe to individual block's changes. When typing, it's only the currently selected block's content that is changing. If any store listeners is checking for changes in one of the other 999 blocks, they are guaranteed not to be there.

In other words, we could break the core/block-editor store into a set of smaller stores. Either "real" data stores or "virtual".

But I believe we can make significant progress even with the existing Redux architecture. In #55337, by optimizing the useSetting hook, I was able to reduce the number of subscriptions from 80k to 60k, by 25%. That's not great, because we need an order-of-magnitude improvement, but also not negligible.

One place that I think is very inefficient currently is how we use BlockControls and other Slot/Fill components. This is a common pattern how to implement a block control:

const validFoos = useSelect( ( select ) => {
  /* Complex selection from block-editor store */
} );

return (
  <>
    <BlockControls>
      <FooControl foos={ validFoos } />
    <BlockControls>
    <Edit />
  </>
);

Here, every block instance does one or multiple useSelect calls to compute the validFoos value, and then passes it as a prop to <FooControl>. But most of the time, that <FooControl> component is not rendered at all! It's rendered only for the currently selected block. We do a lot of calculations and create a lot of store subscriptions to create data that in the end are not used at all.

Ideally the validFoos calculation would be inside the FooControl component, triggered only when it's actually rendered. But that's not so easy because it's rendered inside a slot, and the slot is in a very different React context. It's not even in the same document.

@tyxla tyxla deleted the try/debounce-rich-text-on-input branch October 13, 2023 11:16
@tyxla
Copy link
Member Author

tyxla commented Oct 13, 2023

Not sure you meant to close this one with #55340 but I'm fine with closing it anyway - it wasn't meant to move forward.

@jsnajdr
Copy link
Member

jsnajdr commented Oct 13, 2023

GitHub closed this automatically when it saw "fix X" in a sentence 🙂 I think we'll continue discussing in this PR for some time.

@Mamaduka
Copy link
Member

The feature controls implemented via hooks (block-editor/src/hooks) would be good candidates for similar optimizations.

Multiple features call useBlockEditingMode to check the editing mode and then render controls. This can add up as a single block can support a variety of features.

Here's an example optimization: #55345.

@Mamaduka
Copy link
Member

Based on my recent tests (#56101), I believe we've reduced the block editor store listeners by 50% to 40k.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Rich text /packages/rich-text [Status] Blocked Used to indicate that a current effort isn't able to move forward [Status] In Progress Tracking issues with work in progress [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants