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

🚀 [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
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
1fca7ce
Added lcp, tests, etc
mszylkowski Jun 11, 2021
11c5ccb
Cleaned up
mszylkowski Jun 11, 2021
46ed6c0
Merge branch 'main' of github.com:ampproject/amphtml into px_firstpage
mszylkowski Jun 15, 2021
6d20a91
Added return
mszylkowski Jun 17, 2021
792ba51
Merge branch 'main' of github.com:ampproject/amphtml into px_firstpage
mszylkowski Jun 17, 2021
8595d55
Use webp
mszylkowski Jun 17, 2021
345f1bc
Fixed imports
mszylkowski Jun 17, 2021
4b397e3
Fixed page loading before distance set
mszylkowski Jun 17, 2021
b63c8a7
Fix isExperimentOn and overrideDistance
mszylkowski Jun 18, 2021
a6d2d9d
Removed log
mszylkowski Jun 18, 2021
080e7b0
Fixed setDistance on navigation
mszylkowski Jun 22, 2021
064bafd
Removed unused import / export
mszylkowski Jun 22, 2021
9a37526
Fix html checks
mszylkowski Jun 22, 2021
6e31458
Updated tests
mszylkowski Jun 23, 2021
ee43c1f
revert const to let
mszylkowski Jun 23, 2021
9c4602a
Update extensions/amp-story/1.0/amp-story.js
mszylkowski Jun 23, 2021
03d6e6c
Simplified preloadAll call
mszylkowski Jun 23, 2021
51bccf9
Removed comment
mszylkowski Jun 23, 2021
da3d7fc
Updated tests and removed getDistance
mszylkowski Jun 23, 2021
c87d3dc
Rely on store service
mszylkowski Jun 24, 2021
acbab68
Added csi experiment
mszylkowski Jun 24, 2021
57d67dc
Merge branch 'main' of github.com:ampproject/amphtml into px_firstpage
mszylkowski Jun 24, 2021
f3a9b94
Merge branch 'main' of github.com:ampproject/amphtml into px_firstpage
mszylkowski Jun 25, 2021
d261b82
Add comment and move experiment
mszylkowski Jul 1, 2021
ca67192
Split tests in compressed / uncompressed
mszylkowski Jul 2, 2021
3566ac6
Merge branch 'main' of github.com:ampproject/amphtml into px_firstpage
mszylkowski Jul 2, 2021
3de9ad7
Catch inside then
mszylkowski Jul 2, 2021
939f55e
Merge branch 'main' of github.com:ampproject/amphtml into px_firstpage
mszylkowski Jul 7, 2021
ba37419
Fixed test
mszylkowski Jul 7, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions examples/amp-story/lcp.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<!-- LCP test. The three images at q=100 will be 1.5MB each. Set q=30 to simulate compressed images at 0.18MB each. -->
<!doctype html>
<html ⚡ lang="en">
<head>
<meta charset="utf-8">
<link rel="canonical" href="lcp.html">
<meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
<script async src="https://cdn.ampproject.org/v0.js"></script>
<title>LCP test</title>

<script async custom-element="amp-story" src="https://cdn.ampproject.org/v0/amp-story-1.0.js"></script>
<script async custom-element="amp-video" src="https://cdn.ampproject.org/v0/amp-video-0.1.js"></script>

<style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
<style amp-custom>
* {
box-sizing: border-box;
}
amp-story * {
font-family: 'Helvetica Neue', sans-serif;
color: white;
}
amp-story p {
line-height: 1.5;
}
.bold {
font-weight: bold;
}
.bottom {
align-content: end;
}
.last {
padding-bottom: 8.83vh;
}
.byline {
letter-spacing: 1.28px;
}
</style>
</head>
<body>
<amp-story standalone id="cats"
title="LCP test" publisher="The AMP team"
publisher-logo-src="./img/amplogo.png"
poster-portrait-src="./img/overview.jpg">

<amp-story-page id="page-1" title="Lorem ipsum dolor sit amet">
<amp-story-grid-layer template="fill">
<amp-img
id="img1"
width="400"
height="750"
src="https://images.unsplash.com/photo-1515705576963-95cad62945b6?w=1750&q=30&fm=webp"
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
layout="fill">
</amp-img>
</amp-story-grid-layer>
<amp-story-grid-layer template="vertical" class="bottom">
<p class="bold">LCP for images</p>
<p class="byline">Active page weight: 1.5MB</p>
<p class="byline">Inactive pages weight: 3MB</p>
</amp-story-grid-layer>
</amp-story-page>

<amp-story-page id="page-2">
<amp-story-grid-layer template="fill">
<amp-img
id="img2"
width="400"
height="750"
src="https://images.unsplash.com/photo-1502318217862-aa4e294ba657?w=1080&q=30&fm=webp"
layout="fill">
</amp-img>
</amp-story-grid-layer>
</amp-story-page>

<amp-story-page id="page-3">
<amp-story-grid-layer template="fill">
<amp-img
id="img3"
width="400"
height="750"
src="https://images.unsplash.com/photo-1513809491260-0e192158ae44?w=1080&q=30&fm=webp"
layout="fill">
</amp-img>
</amp-story-grid-layer>
</amp-story-page>
</amp-story>
</body>
</html>
3 changes: 3 additions & 0 deletions extensions/amp-story/1.0/amp-story-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -1297,6 +1297,9 @@ export class AmpStoryPage extends AMP.BaseElement {
if (this.isAd()) {
distance = Math.min(distance, 2);
}
if (distance == this.getDistance()) {
jridgewell marked this conversation as resolved.
Show resolved Hide resolved
return;
}

this.element.setAttribute('distance', distance);
this.element.setAttribute('aria-hidden', distance != 0);
Expand Down
27 changes: 22 additions & 5 deletions extensions/amp-story/1.0/amp-story.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ import {endsWith} from '#core/types/string';
import {escapeCssSelectorIdent} from '#core/dom/css-selectors';
import {findIndex, lastItem, toArray} from '#core/types/array';
import {getConsentPolicyState} from '../../../src/consent';
import {getDetail} from '../../../src/event-helper';
import {getDetail, listenOncePromise} from '../../../src/event-helper';
import {getLocalizationService} from './amp-story-localization-service';
import {getMediaQueryService} from './amp-story-media-query-service';
import {getMode, isModeDevelopment} from '../../../src/mode';
Expand Down Expand Up @@ -1533,7 +1533,7 @@ export class AmpStory extends AMP.BaseElement {
// the navigation happened, like preloading the following pages, or
// sending analytics events.
() => {
this.preloadPagesByDistance_();
this.preloadPagesByDistance_(/* prioritizeActivePage */ !oldPage);
this.triggerActiveEventForPage_();

this.systemLayer_.resetDeveloperLogs();
Expand Down Expand Up @@ -2226,8 +2226,11 @@ export class AmpStory extends AMP.BaseElement {
return map;
}

/** @private */
preloadPagesByDistance_() {
/**
* @param {=boolean} prioritizeActivePage
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
* @private
*/
preloadPagesByDistance_(prioritizeActivePage = false) {
if (this.platform_.isBot()) {
this.pages_.forEach((page) => {
page.setDistance(0);
Expand All @@ -2237,13 +2240,27 @@ export class AmpStory extends AMP.BaseElement {

const pagesByDistance = this.getPagesByDistance_();

this.mutateElement(() => {
const preloadAllPages = () => {
pagesByDistance.forEach((pageIds, distance) => {
pageIds.forEach((pageId) => {
const page = this.getPageById(pageId);
page.setDistance(distance);
});
});
};

this.mutateElement(() => {
if (prioritizeActivePage) {
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
// Load page with distance 0 first, and then load the other ones.
return Promise.all(
pagesByDistance[0].map((pageId) => {
const page = this.getPageById(pageId);
page.setDistance(0);
return listenOncePromise(page.element, EventType.PAGE_LOADED);
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
})
).then(() => preloadAllPages());
}
preloadAllPages();
});
}

Expand Down
38 changes: 38 additions & 0 deletions extensions/amp-story/1.0/test/test-amp-story.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {createElementWithAttributes} from '#core/dom';
import {registerServiceBuilder} from '../../../../src/service-helpers';
import {toggleExperiment} from '#experiments';
import {waitFor} from '#testing/test-helper';
import { EventType } from '../events';
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved

// Represents the correct value of KeyboardEvent.which for the Right Arrow
const KEYBOARD_EVENT_WHICH_RIGHT_ARROW = 39;
Expand All @@ -58,6 +59,8 @@ describes.realWin(
let replaceStateStub;
let win;

const nextTick = () => new Promise((resolve) => win.setTimeout(resolve, 0));

/**
* @param {number} count
* @param {Array<string>=} ids
Expand Down Expand Up @@ -1837,5 +1840,40 @@ describes.realWin(
).to.be.false;
});
});

describe('page loading', () => {
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
it('should load the active page on layout', async () => {
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
const pages = await createStoryWithPages(
2,
['page-1', 'page-2'],
false
);
env.sandbox.stub(story, 'mutateElement').callsFake((mutator) => {
mutator();
return Promise.resolve();
});
await story.buildCallback();
await story.layoutCallback();
expect(pages[0].hasAttribute('distance')).to.be.true;
});

it('should load the inactive pages after the active page is loaded', async () => {
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
const pages = await createStoryWithPages(
2,
['page-1', 'page-2'],
false
);
env.sandbox.stub(story, 'mutateElement').callsFake((mutator) => {
mutator();
return Promise.resolve();
});
await story.buildCallback();
await story.layoutCallback();
expect(pages[1].hasAttribute('distance')).to.be.false;
pages[0].dispatchEvent(new CustomEvent(EventType.PAGE_LOADED));
await nextTick();
expect(pages[1].hasAttribute('distance')).to.be.true;
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
});
});
mszylkowski marked this conversation as resolved.
Show resolved Hide resolved
}
);