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

Remove preload for AMP runtime again when boilerplate was removed #17

Closed
schlessera opened this issue Aug 17, 2020 · 22 comments · Fixed by #261
Closed

Remove preload for AMP runtime again when boilerplate was removed #17

schlessera opened this issue Aug 17, 2020 · 22 comments · Fixed by #261
Labels
Optimizer P1 Medium Priority Performance SSR Related to the serverside rendering of the Optimizer
Milestone

Comments

@schlessera
Copy link
Collaborator

Feature description

When the AMP boilerplate is removed by the SSR logic, preloading of the AMP runtime is less critical and it seems to slow down other assets that are of higher priority in that scenario.

The Optimizer should remove an existing preload for the AMP runtime again when the boilerplate was indeed removed.

This was originally reported by @mehigh for an XWP project.

/cc @sebastianbenz


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

  • Add small transformer that runs after the ServerSideRendering transformer and removes an existing AMP runtime preload iff the AMP boilerplate was removed.

QA testing instructions

Demo

Changelog entry

@sebastianbenz
Copy link

Interesting! Have we got any benchmarks? The xwp link unfortunately doesn't work.

@schlessera
Copy link
Collaborator Author

/cc @mehigh Do you have something that can be shared?

@sebastianbenz
Copy link

Also adding @kristoferbaxter and @jridgewell for thoughts on this.

@mehigh
Copy link

mehigh commented Aug 17, 2020

With preload:
https://files.urldocs.com/share/with-preload-v0/with-preload-v0.html

Without preload:
https://files.urldocs.com/share/no-preload-v0/no-preload-v0.html

https://user-images.githubusercontent.com/367593/90236974-32c63800-de2c-11ea-91fc-dfc483913266.png

The difference is visible in GPSI as well:
With preload (speed index 1.8s+)
https://developers.google.com/speed/pagespeed/insights/?url=https%3A%2F%2Ffiles.urldocs.com%2Fshare%2Fwith-preload-v0%2Fwith-preload-v0.html

without preload (speed index decreases to 1.5s and below):
https://developers.google.com/speed/pagespeed/insights/?url=https%3A%2F%2Ffiles.urldocs.com%2Fshare%2Fno-preload-v0%2Fno-preload-v0.html

I've had this hunch after noticing the painting gets a speed penalty when preloading web fonts which are optional, hence tried this thing out when aiming towards the 100 :)

@kristoferbaxter
Copy link

Taking a look, thanks for the ping @sebastianbenz.

@sebastianbenz
Copy link

Webpagetest comparing @mehigh's samples.

@kristoferbaxter
Copy link

A major issue with comparisons like this is the network variability.

Here's another Webpagetest comparison.

https://webpagetest.org/video/compare.php?tests=200817_A9_1975f033d84f7c32dc11e5208906a9fd,200817_DJ_040b5eaf15b7ff00fd36896c6b262295

@mehigh
Copy link

mehigh commented Aug 17, 2020 via email

@kristoferbaxter
Copy link

To try and get to the bottom of this, I am running a few experiments and will publish them on a Github repo later today.

Here's the comparison of preload all versus no-preload on this document using Largest Contentful Paint as the driving metric.

Screen Shot 2020-08-17 at 12 50 21 PM

@mehigh
Copy link

mehigh commented Aug 17, 2020

I think the first contentful paint might be a better metric, as that's what I observed to improve when running local performance audits (with throttling turned on).

@kristoferbaxter
Copy link

kristoferbaxter commented Aug 17, 2020

I've created a repo here where you can explore these modifications a little more cleanly.

Adding a test for FCP would be quite easy.

Here's the comparison for LCP.

example

@mehigh
Copy link

mehigh commented Aug 19, 2020

I did test a scenario where I'm preloading a script (a large 700KB random JS file) from the same domain to see whether there's a negative impact to FCP:
https://files.urldocs.com/share/large-preload-extreme-test/large-preload-extreme-test.html

Here's the webpage test where both tests had 0.850s and 0.871s first byte times - where a 0.2s delay for the first paint of the background color is observed
https://www.webpagetest.org/video/compare.php?tests=200819_YC_f0fc22d26439e8b28fd58ef2740846ba%2C200819_5D_91e2a80180dfa8ee12e3bb87a0eff346&thumbSize=200&ival=100&end=visual

And then another one with a 2.1MB random JS file
https://files.urldocs.com/share/large-preload-extreme-test-2/large-preload-extreme-test-2.html

The largest contentful paint is clearly pushed out by 2.6s (which minimises the inconsistent network behavior)
https://www.webpagetest.org/video/compare.php?tests=200819_65_0fa64d15f15bb8d245aab63f1d9097d2%2C200819_YC_f0fc22d26439e8b28fd58ef2740846ba&thumbSize=200&ival=100&end=visual

And then I did a test with the largest contentful paint element being inlined to see if inline assets are also negatively impacted:
https://files.urldocs.com/share/large-preload-extreme-test-inline-svg/large-preload-extreme-test-inline-svg.html

The webpage test shows the first background paint happening shortly with 0.1s later vs. the baseline with no preload at all:
https://www.webpagetest.org/video/compare.php?tests=200819_B4_9da11d2719fb54ca5bd92a4f053c1a93%2C200819_YC_f0fc22d26439e8b28fd58ef2740846ba&thumbSize=200&ival=100&end=visual

The presence of a preload that's not absolutely necessary to be preloaded (there's no urgency attached to it) seems to have a negative impact on the Speed Index metric and mostly surrounding the fact that the paint of the body background happens sooner (giving the user the perception of 'stuff happening on screen faster'), while other metrics (FCP/LCP) only have a small negative impact when adding the preload to a JS asset.

Also on the LCP aspect, as long as the element is to be downloaded from the network, the queueing can happen earlier unless there's a preload queuing something else sooner, and that's another reason I see why the LCP can be impacted.

Thank you all for your thought around this topic. It's so bleeding edge and exciting to make the web faster 👍

@schlessera schlessera transferred this issue from ampproject/amp-wp Nov 6, 2020
@schlessera schlessera added Performance SSR Related to the serverside rendering of the Optimizer P1 Medium Priority labels Nov 6, 2020
@schlessera
Copy link
Collaborator Author

Given that the test results all seem to indicate this to be a net positive change, I'd like to implement this for the PHP Optimizer. @sebastianbenz, @kristoferbaxter should I also open corresponding issues for the Go and JS versions of the Optimizer?

@kristoferbaxter
Copy link

We can run a more thorough experiment before moving forward.

Will sync with team today on this.

@sebastianbenz
Copy link

Are there any updates on this?

@jridgewell
Copy link

Maybe @kevinkimball or @erwinmombay can run an AMP Cache experiment for this? Should be simple enough.

@schlessera
Copy link
Collaborator Author

@sebastianbenz @kristoferbaxter @jridgewell Any updates on this? It's looking like a promising change, so I'd love to implement this for the library and WordPress integration.

@westonruter
Copy link
Member

westonruter commented Jun 22, 2021

I've prototyped this along with postponing the loading of scripts that aren't render-critical at ampproject/amp-wp#6182 (comment).

@kristoferbaxter
Copy link

There is no update beyond the need for statistical analysis at a large enough scale to validate results.

If we choose to adopt this and other suggestions there are several changes required for systems that ingest AMP documents and validate them.

@westonruter westonruter added this to the 0.6.0 milestone Jun 25, 2021
@kristoferbaxter
Copy link

We should move forward with removal for now, data suggests the origin performance can be better without, but the preload version from a cache will continue to insert and perform better with in that scenario.

@robin-scott
Copy link

@westonruter found link to this from here https://wordpress.org/support/topic/preload-amp-runtime-2/#:~:text=1%20month%2C%204%20weeks%20ago - the issue which users face running AMP plugin is they get dings saying to add the preload here:

https://amp.dev/page-experience/

Unless the test itself is updated not to prompt users on this, we're looping everyone back to then tell them to ignore it.

@westonruter
Copy link
Member

@robin-scott The issue to fix this is ampproject/amp-toolbox#1256

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Optimizer P1 Medium Priority Performance SSR Related to the serverside rendering of the Optimizer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants