Make WordPress Core

Opened 12 months ago

Closed 11 months ago

Last modified 10 months ago

#58472 closed defect (bug) (fixed)

Emoji loader script causes ~100ms long task

Reported by: westonruter's profile westonruter Owned by: westonruter's profile westonruter
Milestone: 6.3 Priority: high
Severity: normal Version: 4.2
Component: Emoji Keywords: needs-testing has-patch
Focuses: javascript, performance Cc:

Description

When doing a performance analysis of the WordPress frontend, Chrome DevTools on wordpress-develop and WebPageTest for a production site both identify a long task while the page is loading. A task is considered long when it exceeds 50 milliseconds, and the long task showing up during a WordPress page load is around double that: ~100ms.

Chrome DevTools identifies the long task as coming from emoji-loader.js, specifically the emojiSetsRenderIdentically() function. This function paints two strings of text onto a canvas and then compares them to see whether they are identical. This is a relatively slow synchronous operation and it is called up to four times with every page load. The long task is negatively impacting the Largest Contentful Paint (LCP) metric in Core Web Vitals (CWV). It has the same effect of increasing TTFB by 100ms via usleep(100000) on the server.

There are two key opportunities for improving performance of the emoji loader script to stop causing this long task and delaying LCP:

First, the results of the expensive computation in emojiSetsRenderIdentically() can be stored in sessionStorage so that when the user navigates to the next page, the browser doesn't have to recompute whether the emoji sets still render identically (which of course they still do). This speeds up navigations between pages on a site, but it does not improve the first page visit.

Second, and more importantly, instead of rendering the emoji in a canvas on the main thread it can instead leverage OffscreenCanvas in a worker thread to do the same. This API is currently supported by 83.69% of browsers globally (Can I use?), including the current versions of Chrome, Edge, Safari, and Firefox.

Implementing these changes should give a significant boost to all WordPress installs regardless of server performance.

See also #37788 (especially comment 4) and #35498.

Attachments (3)

devtools-performance-pane.png (95.3 KB) - added by westonruter 12 months ago.
Long task identified in Chrome DevTools
webpagetest.png (70.4 KB) - added by westonruter 12 months ago.
WebPageTest Perfetto analysis: https://www.webpagetest.org/chrome/perfetto/index.html#!/viewer?url=https%3A%2F%2Fwww.webpagetest.org%2Fgetgzip.php%3Ftest%3D230518_AiDcBB_BZT%26file%3D1_trace.json
devtools-performance-pane-with-optimization.png (118.5 KB) - added by westonruter 12 months ago.
DevTools Performance pane showing the emoji loader is no longer a long task, but rather the task is now just 18ms

Download all attachments as: .zip

Change History (43)

@westonruter
12 months ago

Long task identified in Chrome DevTools

This ticket was mentioned in PR #4562 on WordPress/wordpress-develop by @westonruter.


12 months ago
#1

  • Keywords has-patch added

Changes:

  • Use sessionStorage to remember the previous results of calls to browserSupportsEmoji().
  • When OffscreenCanvas is available, offload browserSupportsEmoji() to a web worker to prevent blocking the main thread. Currently this is implemented using async/await which will break IE11 (which may not be a problem now).

Todo:

  • [ ] Drop support for IE11?
  • [ ] Modernize script with const, let, arrow functions, etc.
  • [ ] Apply prettier formatting.
  • [ ] Try to reuse the same canvas between the two calls to browserSupportsEmoji()

Trac ticket: https://core.trac.wordpress.org/ticket/58472

#2 follow-up: @westonruter
12 months ago

The changes in the PR eliminate the long task. I expected it to reduce LCP by ~100ms, but seems it is more ~30ms on my machine (Chrome on HP Dragonfly Elite Chromebook). Presumably there will be greater gains on less powerful devices.

Last edited 12 months ago by westonruter (previous) (diff)

@westonruter
12 months ago

DevTools Performance pane showing the emoji loader is no longer a long task, but rather the task is now just 18ms

#3 @westonruter
12 months ago

  • Owner set to westonruter
  • Status changed from new to accepted

@dmsnell commented on PR #4562:


12 months ago
#4

This may be my silliness, but if OffscreenCanvas is available, why are we rendering the emoji loader there? Seems like everything that we're testing for is supported in all browsers with OffscreenCanvas is supported.

Maybe we continue testing for new Unicode versions, but right now I don't think it's possible to have OffscreenCanvas and need the Emoji loader. Is this wrong? If it's not wrong, seems like there's no point running the code at all.

@westonruter commented on PR #4562:


12 months ago
#5

@dmsnell This would be better known by @pento, but I recall he said that emoji are part of the OS and not part of the browser. So even though the browser may support all the latest web platform features, it may still lack support for all emoji. See this comment of his.

@dmsnell commented on PR #4562:


12 months ago
#6

emoji are part of the OS and not part of the browser.

this is completely true, but I think no browser that supports these features can practically run on an OS without support for the versions of Unicode that we check for. maybe I'm wrong.

@peterwilsoncc commented on PR #4562:


12 months ago
#7

but I think no browser that supports these features can practically run on an OS without support for the versions of Unicode that we check for. maybe I'm wrong.

I haven't looked in to this but even if it's the case at the time of writing, it won't remain so for long. Unicode 15.1 is scheduled for release in September 2023 at which time WP will update the emoji it checks for.

#8 in reply to: ↑ 2 @westonruter
12 months ago

Replying to westonruter:

The changes in the PR eliminate the long task. I expected it to reduce LCP by ~100ms, but seems it is more ~30ms on my machine (Chrome on HP Dragonfly Pro Chromebook). Presumably there will be greater gains on less powerful devices.

I obtained a trace using WebPageTest on a Moto G Power device and the long task was ~200ms, so even longer as expected compared to my relatively powerful laptop. Users of lower-powered devices will see the most gains from these improvements.

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


12 months ago

valterlorran commented on PR #4562:


12 months ago
#10

I also made an updated version of the file without using only promises:
https://gist.github.com/valterlorran/426bc6ba2aaa47c17991cbf56a5f5661

valterlorran commented on PR #4562:


12 months ago
#11

This is working for me. Without these changes, it took ~10ms on my m1 Mac. Applying the changes, it dropped to ~3ms; when cached, it spent ~0.30ms.

@westonruter commented on PR #4562:


12 months ago
#12

I also made an updated version of the file without using only promises:
https://gist.github.com/valterlorran/426bc6ba2aaa47c17991cbf56a5f5661

Thanks for that reference. There were two reasons why I went with async/await:

  1. The resulting diff is smaller since existing synchronous-style code can be preserved with await.
  2. The only browser that doesn't support async functions (i.e. await) is IE11. This browser also doesn't support promises. If IE11 compatibility isn't a concern anymore, then no need to use promises. But if compatibility _is_ a concern, then not only will we need to use promises but we'll also need to have a branch in the logic that avoids promises altogether for IE11's sake.

@westonruter commented on PR #4562:


12 months ago
#13

This is working for me. Without these changes, it took ~10ms on my m1 Mac. Applying the changes, it dropped to ~3ms; when cached, it spent ~0.30ms.

@valterlorran Thanks for verifying. As expected, since you have a powerful machine the time required to run emoji-loader is only ~10ms, which doesn't even qualify as a long task (unlike in my case when there is a long task on an HP Dragonfly Elite Chromebook running an Intel i7 processor). Nevertheless, even for a powerful machine there is still a 70% reduction of main thread work for the non-cached state and a 97% reduction for the cached state.

@westonruter commented on PR #4562:


12 months ago
#14

According to WordPress's Browser support, those supported are:

Browsers with > 1% usage based on can I use browser usage table

And for IE, the results are:

Version | Percentage

--

5.5| 0%
6| 0%
7| 0%
8| 0.03%
9| 0.04%
10| 0%
11| 0.37%
Sum | 0.44%

So all IE browser versions _combined_ account for less than 1%. Even less than 0.5%. Remarkable how far we've come!

It seems we can truly abandon consideration for IE!

This ticket was mentioned in Slack in #core by westonruter. View the logs.


12 months ago

#16 @flixos90
12 months ago

  • Priority changed from normal to high

The implementation from https://github.com/WordPress/wordpress-develop/pull/4562 looks like an amazing performance win! I conducted a benchmark using benchmark-web-vitals that shows that LCP is ~15% improved due to this change. 🎉

See the full results here

Note that while it may look at first glance that TTFB is a tiny bit faster with trunk, that is entirely due to variance, since the PR makes zero changes to server-side code.

I'm setting the priority here to high since this is looking to be one of the most impactful performance wins in the release if it makes it in.

#17 @westonruter
12 months ago

For discussion on whether emoji-loader can go ahead and leave behind IE compatibility (which is already deprecated) by leveraging ES6+ syntax (specifically async functions), see thread in #core Slack. Original thread message:

I'm working on enhancing emoji-loader.js to improve performance by eliminating a long task that blocks the main thread (#58472). To do this, it really means abandoning compatibility with IE11. I know that WP 5.8 had a IE 11 Support Phase Out Plan and that IE is no longer among the supported browsers (indeed, all versions of IE sum up to only 0.47% usage currently). However, my understanding has been that this policy primarily applied to the admin/editor context, and that the frontend was not going to jump toward removing IE11 compat. (Granted skip-link-focus-fix was just removed #54421.) Do we have the freedom now to disregard IE11 compat for frontend features where relevant? In the case of emoji-loader, lack of IE11 support would mean users may have broken emoji (so likely not a catastrophe, and the least of their problems browsing the web today). Thoughts?

Current concluding remarks in the thread is that a syntax error in the emoji-loader script would have the same effect as if the site owner turned off emoji-loader entirely (which sites do, as per #35498). So there would be a graceful fallback to rendering whatever emoji the platform supports. Other scripts on the page would not be impacted. The exception to this is if the user has script debugging enabled in which case a dialog would appear asking if the user wants to continue running scripts on the page. But non-developer users are extremely unlikely to have script debugging enabled. For site owners who do, they can just turn off emoji-loader (e.g. with the disable-emojis plugin).

#18 @westonruter
12 months ago

I just found a very helpful StackOverflow answer that perfectly addresses the question about whether IE halts all scripts when one has a script error. In short, this seems to have been the case up until IE8 when the script debugging dialog may appear. However, IE9 and above do not do this. Therefore, we shouldn't be concerned about exposing JS syntax errors to such browsers as IE≤8 account for ~0.03% of browser market share.

#19 @mukesh27
11 months ago

  • Keywords changes-requested added

The PR need some changes based on feedback.

@westonruter commented on PR #4562:


11 months ago
#20

Given that you still have some unchecked boxes in the PR description and the PR is still marked as a draft, could you iterate on the remaining bits to get it ready for a full review? Or are you waiting on any other intermediate feedback that is blocking you?

I welcome feedback on those todos specifically, specifically regarding updating core to allow ES6+.

#21 @westonruter
11 months ago

In comparing the performance gains with the changes, I was surprised when I tried turning off the condition to offload the canvas logic to the worker, that the performance was basically the same as if the updated code ran in the main thread. (I tested in Chrome.)

It appears that the main performance benefit here ais not actually from offloading to the worker after all. Rather, the performance gains are primarily achieved by enabling the willReadFrequently option on the canvas context. The bare minimum to achieve the performance gains is this patch:

  • src/js/_enqueues/lib/emoji-loader.js

    diff --git a/src/js/_enqueues/lib/emoji-loader.js b/src/js/_enqueues/lib/emoji-loader.js
    index 536b839f0e..7fefb03bdc 100644
    a b  
    77
    88        // Create a canvas element for testing native browser support of emoji.
    99        var canvas = document.createElement( 'canvas' );
    10         var context = canvas.getContext && canvas.getContext( '2d' );
     10        var context = canvas.getContext && canvas.getContext( '2d', { willReadFrequently: true } );
    1111
    1212        /**
    1313         * Checks if two sets of Emoji characters render the same visually.

Interestingly, I only discovered this willReadFrequently option when switching from comparing the return value of canvas.toDataURL() to instead comparing the return value of canvas.getImageData(). When I made this switch, Chrome DevTools gave me a warning:

Canvas2D: Multiple readback operations using getImageData are faster with the willReadFrequently attribute set to true. See: https://html.spec.whatwg.org/multipage/canvas.html#concept-canvas-will-read-frequently

This warning was not emitted when using toDataURL(), which seems like an oversight in Chrome DevTool's performance auditing.

Nevertheless, the willReadFrequently option is not supported yet in Safari (Can I use?).

Therefore, it seems beneficial to retain offloading the canvas logic to the Worker.

#22 @westonruter
11 months ago

  • Keywords changes-requested removed

Pull request ready for full review: https://github.com/WordPress/wordpress-develop/pull/4562

I've restored ES3 compatibility and the logic is now short-circuiting when Promise is undefined, which will prevent errors from surfacing in IE11.

#23 @flixos90
11 months ago

I've conducted another benchmark with the latest version of the PR, just to double-check that it's still as impactful as previously envisioned (since several changes have been made in the last 2 weeks). Happy to report that it is!

Since the last benchmark, the benchmark-web-vitals command line tool now also reports "LCP minus TTFB", which is the most meaningful metric to look at in order to assess the performance impact of this change. In that metric, the performance improvement is 56ms / 45%!

Overall LCP improvement based on the new metrics is ~30%, however that is skewed by TTFB being faster in the test runs for this PR, so it's not that much in reality. If we add the baseline TTFB of ~150ms back to the median "LCP-TTFB" result, we see a ~20% overall LCP improvement - somewhat of an overall estimation with cleanup of the TTFB inconsistency.

TLDR: This is an amazing performance win.

See the full results here

@westonruter commented on PR #4562:


11 months ago
#24

happy to report this is still extremely impactful for LCP performance 🎉

It should be even more so now that it's only making one call to the worker, which means there's only overhead for one worker instance and the canvas only has to be constructed once.

@flixos90 commented on PR #4562:


11 months ago
#25

happy to report this is still extremely impactful for LCP performance 🎉

It should be even more so now that it's only making one call to the worker, which means there's only overhead for one worker instance and the canvas only has to be constructed once.

Makes sense, that explains why it's now ~20% LCP improvement while before it was ~15% :)

#26 @westonruter
11 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 56074:

Emoji: Optimize emoji loader with sessionStorage, willReadFrequently, and OffscreenCanvas.

  • Use sessionStorage to remember the previous results of calls to browserSupportsEmoji() for 1 week.
  • Optimize reading from canvas by supplying the willReadFrequently option for the 2D context.
  • When OffscreenCanvas is available, offload browserSupportsEmoji() to a web worker to prevent blocking the main thread. This is of primary benefit to Safari which does not yet support willReadFrequently.
  • Remove obsolete support for IE11 since promises are now utilized. Nevertheless, ES3 syntax is maintained and IE11 will simply short-circuit.

Props westonruter, dmsnell, peterwilsoncc, valterlorran, flixos90, spacedmonkey, joemcgill.
Fixes #58472.

@westonruter commented on PR #4562:


11 months ago
#27

Committed to trunk in r56074

@westonruter commented on PR #4562:


11 months ago
#28

I'm still a bit lost on the fallback when things fail. for instance, in the previous script, as things run, if none of the tests complete, we end up loading the Twemoji script.

What I mean is that if browserSupportsEmoji() throws an error in the current stable code, then Twemoji would't get loaded. So in that way the new code here isn't any different.

@dmsnell commented on PR #4562:


11 months ago
#29

Oops. I think we may have merged this when it's broken for some of the very use-cases it's here for 🙃

@dmsnell commented on PR #4562:


11 months ago
#30

So in that way the new code here isn't any different.

The new code aborts immediately is Promise isn't available. We have a lot of safety code in here that aborts safely without crashing. We don't load Twemoji in those cases. These are on the periphery of page views, but then again, so is the need for Twemoji, which this is there for.

@westonruter commented on PR #4562:


11 months ago
#31

in this branch, however, if the browser doesn't support Promise (which would be a strong indicator that it likely also doesn't support updated Unicode revisions) then we abort immediately and never load the scripts (when we probably ought to).

WordPress doesn't support IE11 anymore, and this is the only browser that doesn't support promises. This PR was going to go all the way and use ES8+ syntax and break IE11 entirely, but for the sake of expediency ES3 compat was maintained so IE11 won't have a syntax error. See Trac comment.

@dmsnell commented on PR #4562:


11 months ago
#32

okay. we actually went through the unsupported browser discussion before and ended up not trying to go out of our way to support unsupported browsers.

the Promise thing caught me off guard at the end of review.

#33 @azaozz
11 months ago

  • Keywords needs-patch needs-testing added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Seems the minified script fails with ReferenceError: c is not defined. Happens in production where SCRIPT_DEBUG = false.

Most likely caused by the minifier failing to handle the code where functions get passed to run in the Worker. Also see the comment here: https://github.com/WordPress/wordpress-develop/pull/4562/files#diff-83d8ed9928b125b09de0268ee4775d21a58b93ed92da09828359760f9200b069R352 and a suggested fix here: https://wordpress.slack.com/archives/C02RQBWTW/p1687967842409629?thread_ts=1687963471.592929&cid=C02RQBWTW.

As this fails in production it would probably be best to revert for now and fix and reintroduce in time for beta2.

Version 1, edited 11 months ago by azaozz (previous) (next) (diff)

This ticket was mentioned in Slack in #core by azaozz. View the logs.


11 months ago

This ticket was mentioned in PR #4741 on WordPress/wordpress-develop by @westonruter.


11 months ago
#35

  • Keywords has-patch added; needs-patch removed

#36 @westonruter
11 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 56095:

Emoji: Pass functions as arguments in loader to account for minification and worker script.

Amends [56074].
Fixes #58472.
Props joemcgill, westonruter, azaozz.

@westonruter commented on PR #4741:


11 months ago
#37

Committed in r56095.

#38 @westonruter
11 months ago

In 56119:

Emoji: Give name to web worker in emoji loader and terminate when finished.

See #58472.
Follow-up to [56074], [56095].
Props westonruter, joemcgill.
Fixes #58691.

#39 @westonruter
10 months ago

Follow up: #58978.

I discovered that any reference to sessionStorage in sandboxed iframe lacking the allow-same-origin flag will cause an error to be emitted to the console (at least in Chrome DevTools), and only for the first reference. It doesn't seem to impact the execution in any way, however.

#40 @westonruter
10 months ago

In 56358:

Emoji: Suppress console errors from sessionStorage usage in sandboxed post embed iframe.

Amends [56074].

Props westonruter, flixos90.
Fixes #58978.
See #58472.

Note: See TracTickets for help on using tickets.