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 requested a review from gmajoulet June 17, 2021 18:39
@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?

@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.

@mszylkowski mszylkowski requested a review from gmajoulet June 18, 2021 15:33
gmajoulet
gmajoulet reviewed Jun 18, 2021
View reviewed changes
@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 requested a review from gmajoulet June 22, 2021 16:43
gmajoulet
gmajoulet reviewed Jun 22, 2021
View reviewed changes
@mszylkowski mszylkowski requested a review from gmajoulet June 24, 2021 23:11
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.

gmajoulet
gmajoulet reviewed Jun 29, 2021
View reviewed changes
@mszylkowski mszylkowski requested a review from gmajoulet July 1, 2021 18:32
gmajoulet
gmajoulet approved these changes Jul 1, 2021
View reviewed changes
@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 requested a review from jridgewell July 2, 2021 20:45
@mszylkowski mszylkowski merged commit 1594e79 into ampproject:main 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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mszylkowski @gmajoulet @kristoferbaxter @jridgewell @rcebulko @ampprojectbot