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

Editor: Improve render performance (scrolling and typing) #23568

Closed
wants to merge 9 commits into from

Conversation

ItsJonQ
Copy link

@ItsJonQ ItsJonQ commented Jun 29, 2020

85607658-520dc980-b622-11ea-8a7c-0d80a6ef5e1c

This update improves the rendering performance of the editor, especially for user interactions such as typing and scrolling.

This was achieved using a combination of CSS and JS updates that leverage GPU rendering where appropriate and optimize layout calculations.

Here are the results:

(Recorded on Chrome with the paint rendering debug tool enabled).
A green flash indicates a browser repaint.

Before
https://d.pr/v/g3eX8A

Notice that every interaction causes the entire editor to flash. It's bad enough when scrolling is involved. It's particularly concerning when the user types anything (anywhere, including the sidebar).

After
https://d.pr/v/I3uOyb

Wayyyy less repainting! Notice when typing, the repaint is contained in the typing area (e.g. RichText or input)

How has this been tested?

This has been tested in local Gutenberg across Chrome, Firefox, and Safari.

  • Run npm install
  • Run npm run dev
  • Use the editor (add content, etc...)
  • Ensure that things render correctly

Certain things to pay attention to. Anything involving popovers (e.g. Toolbar, color picker).

For visual repaint testing...

Types of changes

  • Moved the <Popover.Slot /> for BlockToolbar
  • Added CSS layering with translate3d(0, 0, 0) for various elements

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [n/a] My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • [n/a] I've updated all React Native files affected by any refactorings/renamings in this PR.

Resolves: #23427

@ItsJonQ ItsJonQ added [Type] Performance Related to performance efforts [Package] Editor /packages/editor labels Jun 29, 2020
@ItsJonQ ItsJonQ requested review from youknowriad and ellatrix June 29, 2020 19:00
@ItsJonQ ItsJonQ self-assigned this Jun 29, 2020
@github-actions
Copy link

github-actions bot commented Jun 29, 2020

Size Change: +846 B (0%)

Total Size: 1.14 MB

Filename Size Change
build/block-editor/style-rtl.css 10.8 kB +14 B (0%)
build/block-editor/style.css 10.8 kB +14 B (0%)
build/edit-post/index.js 304 kB +181 B (0%)
build/edit-post/style-rtl.css 5.64 kB +47 B (0%)
build/edit-post/style.css 5.63 kB +47 B (0%)
build/edit-site/index.js 16.8 kB +180 B (1%)
build/edit-site/style-rtl.css 3.07 kB +46 B (1%)
build/edit-site/style.css 3.07 kB +46 B (1%)
build/edit-widgets/index.js 9.53 kB +179 B (1%)
build/edit-widgets/style-rtl.css 2.5 kB +47 B (1%)
build/edit-widgets/style.css 2.49 kB +45 B (1%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.67 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/index.js 115 kB 0 B
build/block-library/editor-rtl.css 7.54 kB 0 B
build/block-library/editor.css 7.54 kB 0 B
build/block-library/index.js 130 kB 0 B
build/block-library/style-rtl.css 7.75 kB 0 B
build/block-library/style.css 7.76 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/index.js 199 kB 0 B
build/components/style-rtl.css 15.8 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.56 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.46 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 45 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.4 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this 💯 Thanks a lot, this is very cool.

packages/components/src/toolbar/stories/index.js Outdated Show resolved Hide resolved
@@ -266,6 +266,7 @@ function Layout() {
<KeyboardShortcutHelpModal />
<WelcomeGuide />
<Popover.Slot />
<Popover.Slot name="block-toolbar" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this slot was not rendered her for a valid reason. Otherwise, it just duplicated the slot that is above it. cc @ellatrix

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad Hmm! Interesting.. I wasn't aware of that !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be rendered in the content scroll container, because when you scroll while the pointer is over the toolbar, the content still needs to scroll.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should have commented here 😅
There might be other reasons I'm forgetting.

@ZebulanStanphill
Copy link
Member

Instead of the clever, but somewhat hacky method of using transform to trigger GPU optimizations, why not try using the contain property instead?

@ItsJonQ
Copy link
Author

ItsJonQ commented Jun 29, 2020

@ZebulanStanphill Oh interesting! I didn't know about contain. It looks like it's not support on Safari and IE. Bummer :(

@ItsJonQ
Copy link
Author

ItsJonQ commented Jul 6, 2020

Hmm.. Not sure how to debug the failing E2E tests.
Locally, the tests fail for me on master as well 🙈

@ntsekouras
Copy link
Contributor

Hey @ItsJonQ -- great additions here!

Locally, the tests fail for me on master as well

Have you tried commenting observeConsoleLogging() for running e2e locally? I tried some of them locally on master and they pass ( with the above commented ).

I'll take a look now on your branch to see if I can help :)

@ItsJonQ
Copy link
Author

ItsJonQ commented Jul 7, 2020

@ntsekouras Thanks for helping!

I'm not too sure how to resolve this. The test passes for me (locally) in interactive mode.

npm run test-e2e:watch -- packages/e2e-tests/specs/editor/various/multi-block-selection.test.js --puppeteer-interactive --puppeteer-slowmo=100

(I run it with .only, so it targets the failing test)

	it.only( 'should clear selection when clicking next to blocks', async () => { ... } );

@ellatrix
Copy link
Member

ellatrix commented Jul 7, 2020

What are the performance results compared to master?

@ItsJonQ
Copy link
Author

ItsJonQ commented Jul 7, 2020

What are the performance results compared to master?

@ellatrix Excellent question! I just ran some Performance + FPS tests against master.

Typing

I think typing is where we'd see/feel noticeable improvements.

In the following performance tests, I typed "Typing into the editor" into a Paragraph block.

Before

typing-before

After

typing-after

(Higher overall FPS levels)

The performance tool makes things laggier (in general). However, it felt smoother typing with the new render improvements.


Scrolling

Not a huge change. But a bit!

Before

scrolling-before

After

scrolling-after

Looks like lower CPU spikes during start/stops.

Scrolling FPS

🚨 Warning: Fast GIFs incoming...

Before

scrolling-before

(Up to approx. 50-51FPS on heavy scrolling)

After

scrolling-after

(Up to approx. 54-55FPS on heavy scrolling)

@ItsJonQ
Copy link
Author

ItsJonQ commented Jul 7, 2020

Updates. After a bunch of poking and testing... it looks like the E2E tests aren't happy with the translate3d(0, 0, 0) change applied to:

.interface-interface-skeleton__content,
.interface-interface-skeleton__left-sidebar,
.interface-interface-skeleton__sidebar

Maybe it's something up with Puppeteer? Maybe not.
It looks like the Android test is unhappy as well.

Unfortunately, that is important piece for this performance enhancement, which attempts to tame repaint/layout calc.

Maybe we should give up on this for now?

@ZebulanStanphill
Copy link
Member

@ZebulanStanphill Oh interesting! I didn't know about contain. It looks like it's not support on Safari and IE. Bummer :(

I think it's still worth trying out as a progressive enhancement for the browsers that do support it, regardless of whether we also proceed with this PR or not.

@ItsJonQ
Copy link
Author

ItsJonQ commented Jul 8, 2020

I think it's still worth trying out as a progressive enhancement for the browsers that do support it, regardless of whether we also proceed with this PR or not.

@ZebulanStanphill That's fair. What's strange is... I experimented with contain, but it didn't seem to make a difference (repaint/performance wise). translate3d did though.

@ItsJonQ
Copy link
Author

ItsJonQ commented Nov 16, 2020

Closing this for now. The initial research was helpful in identifying the problem areas.

@ItsJonQ ItsJonQ closed this Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Editor /packages/editor [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor: Interaction triggering drastic browser repaint
5 participants