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.

Sign up for GitHub

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

Jump to bottom

🚀 [Story performance] Experiment to load first page assets before loading other pages to improve LCP #34846

Merged
merged 29 commits into from
Jul 12, 2021
Merged

🚀 [Story performance] Experiment to load first page assets before loading other pages to improve LCP #34846

merged 29 commits into from
Jul 12, 2021

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Jun 11, 2021 •
edited
Loading

In order to improve the Largest Contentful Paint which is the major failing component of the Core Web Vitals for stories, we can delay the loading of the assets (images, audios and videos) on the inactive pages until the active page (the first page that shows up when the story is loaded).

Since we use the distance attribute to determine what to load, following pages will not show up until the active page appears, or we navigate to another page.
lcp3

LCP lab data for this PR

image

LCP field data from WebPageTest

LCP with change 10.09s
LCP without change 11.07s

@mszylkowski mszylkowski marked this pull request as ready for review June 17, 2021 18:39
@amp-owners-bot
Copy link

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-page.js
extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/test/test-amp-story.js

1 similar comment
@amp-owners-bot
Copy link

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-page.js
extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/test/test-amp-story.js

gmajoulet
gmajoulet reviewed Jun 17, 2021
View reviewed changes
Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

Can you launch this behind a CSI reported experiment so we can gather performance metrics?

examples/amp-story/lcp.html Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
@mszylkowski
Copy link
Contributor Author

mszylkowski commented Jun 17, 2021 •
edited
Loading

@kristoferbaxter will remove the logs 👍

kristoferbaxter
kristoferbaxter reviewed Jun 17, 2021
View reviewed changes
Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Left a few comments, but let's also get @jridgewell or @erwinmombay to take a look.

Perhaps for some period of time asking @ampproject/wg-performance for reviews on performance related changes to the stories format would be prudent.

gmajoulet
gmajoulet reviewed Jun 18, 2021
View reviewed changes
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
@gmajoulet
Copy link
Contributor

Can you clarify what you're measuring in the metrics you're posted? A LCP of 0.5s without any optimization is very very surprising to me

@mszylkowski mszylkowski self-assigned this Jun 22, 2021
@mszylkowski mszylkowski added this to In progress in wg-stories Performance via automation Jun 22, 2021
@mszylkowski mszylkowski added this to In progress in wg-stories Sprint via automation Jun 22, 2021
gmajoulet
gmajoulet reviewed Jun 22, 2021
View reviewed changes
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/test/test-amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/test/test-amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/test/test-amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/test/test-amp-story.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/test/test-amp-story.js Outdated Show resolved Hide resolved
jridgewell
jridgewell reviewed Jun 29, 2021
View reviewed changes
Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

It seems instead of "prealoding all" (parallel), it should just use serial preloading? Ie, prioritize based on distance.

extensions/amp-story/1.0/amp-story.js Show resolved Hide resolved
gmajoulet
gmajoulet reviewed Jun 29, 2021
View reviewed changes
examples/amp-story/lcp.html Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved
gmajoulet
gmajoulet approved these changes Jul 1, 2021
View reviewed changes
examples/amp-story/lcp.html Outdated Show resolved Hide resolved
@kristoferbaxter
Copy link
Contributor

Before moving forward here, as an additional verification can you provide a few runs (9 is likely ok) on WebPageTest of this diff on a midrange device?

@mszylkowski
Copy link
Contributor Author

@kristoferbaxter thanks for the suggestion, didn't know about WebPageTest. Added the results to the PR description but here they are:

LCP field data from WebPageTest

LCP with change 10.09s
LCP without change 11.07s

@mszylkowski mszylkowski merged commit 1594e79 into ampproject:main Jul 12, 2021
wg-stories Performance automation moved this from In progress to Done Jul 12, 2021
wg-stories Sprint automation moved this from In progress to Done Jul 12, 2021
@mszylkowski mszylkowski deleted the px_firstpage branch July 12, 2021 15:00
@mszylkowski mszylkowski changed the title 🚀 [Story performance] Load first page assets before loading other pages to improve LCP 🚀 [Story performance] Experiment to load first page assets before loading other pages to improve LCP Jul 12, 2021
@gmajoulet
Copy link
Contributor

@mszylkowski can you send a follow up PR to launch this to 10% plz? We can control further using the new instant experiment system later on once it's back, but for this one let's use the regular experiments.

@mszylkowski
Copy link
Contributor Author

For reference: Launching experiment to 10% in #35192

@mszylkowski mszylkowski mentioned this pull request Jul 19, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@jridgewell jridgewell jridgewell approved these changes

@rcebulko rcebulko Awaiting requested review from rcebulko

@kristoferbaxter kristoferbaxter Awaiting requested review from kristoferbaxter

Assignees

@mszylkowski mszylkowski

Labels
PR use: In Beta / Experimental PR use: In Stable
Projects
wg-stories Performance
  
Done
wg-stories Sprint
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mszylkowski @gmajoulet @kristoferbaxter @jridgewell @rcebulko @ampprojectbot