Skip to content

Commit

Permalink
performance-impls: LCP should use startTime (#35070)
Browse files Browse the repository at this point in the history 
* performance-impls: LCP should use startTime

* Update test/unit/test-performance.js

Co-authored-by: Ryan Cebulko <[email protected]>

* update TICKEVENTS.md

Co-authored-by: Ryan Cebulko <[email protected]>
  • Loading branch information
@samouri @rcebulko
samouri and rcebulko authored Jun 30, 2021
1 parent 00411d1 commit 8662b2b
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 55 deletions.
3 changes: 1 addition & 2 deletions extensions/amp-viewer-integration/TICKEVENTS.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ As an example if we executed `perf.tick('label')` we assume we have a counterpar
| Layout Jank, second exit | `lj-2` | The aggregate jank score when the user leaves the page (navigation, tab switching, dismissing application) for the second time. |
| Cumulative Layout Shift, first exit | `cls` | The aggregate layout shift score when the user leaves the page (navigation, tab switching, dismissing application) for the first time. See https://web.dev/layout-instability-api |
| Cumulative Layout Shift, second exit | `cls-2` | The aggregate layout shift score when the user leaves the page (navigation, tab switching, dismissing application) for the second time. |
| Largest Contentful Paint, load time | `lcpl` | The time in milliseconds for the first contentful element to display. This is the load time version of this metric. See https://github.com/WICG/largest-contentful-paint |
| Largest Contentful Paint, render time | `lcpr` | The time in milliseconds for the first contentful element to display. This is the render time version of this metric. https://github.com/WICG/largest-contentful-paint |
| Largest Contentful Paint | `lcp` | The time in milliseconds for the largest contentful element to display. |
| Largest Contentful Paint (since visible) | `lcpv` | The time in ms for largest contentful element to display, offset by first visible time. Based on render time, falls back to load time. |
| DOM Complete | `domComplete` | Time immediately before the browser sets the current document readiness of the current document to complete |
| DOM Content Loaded Event End | `domContentLoadedEventEnd` | Time immediately after the current document's DOMContentLoaded event completes |
Expand Down
3 changes: 1 addition & 2 deletions src/core/constants/enums.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ export const TickLabel = {
FIRST_VIEWPORT_READY: 'pc',
GOOD_FRAME_PROBABILITY: 'gfp',
INSTALL_STYLES: 'is',
LARGEST_CONTENTFUL_PAINT: 'lcp',
LARGEST_CONTENTFUL_PAINT_VISIBLE: 'lcpv',
LARGEST_CONTENTFUL_PAINT_LOAD: 'lcpl',
LARGEST_CONTENTFUL_PAINT_RENDER: 'lcpr',
LONG_TASKS_CHILD: 'ltc',
LONG_TASKS_SELF: 'lts',
MAKE_BODY_VISIBLE: 'mbv',
Expand Down
54 changes: 19 additions & 35 deletions src/service/performance-impl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export class Performance {

if (!this.supportsLargestContentfulPaint_) {
this.metrics_.rejectSignal(
TickLabel.LARGEST_CONTENTFUL_PAINT_VISIBLE,
TickLabel.LARGEST_CONTENTFUL_PAINT,
dev().createExpectedError('Largest Contentful Paint not supported')
);
}
Expand All @@ -176,20 +176,15 @@ export class Performance {
this.supportsNavigation_ = supportedEntryTypes.includes('navigation');

/**
* The latest reported largest contentful paint time, where the loadTime
* is specified.
* The latest reported largest contentful paint time. Uses entry.startTime,
* which equates to: renderTime ?? loadTime. We can't always use one or the other
* because:
* - loadTime is 0 for non-remote resources (text)
* - renderTime is undefined for crossorigin resources
*
* @private {?number}
*/
this.largestContentfulPaintLoadTime_ = null;

/**
* The latest reported largest contentful paint time, where the renderTime
* is specified.
*
* @private {?number}
*/
this.largestContentfulPaintRenderTime_ = null;
this.largestContentfulPaint_ = null;

this.onAmpDocVisibilityChange_ = this.onAmpDocVisibilityChange_.bind(this);

Expand Down Expand Up @@ -353,12 +348,7 @@ export class Performance {
this.layoutShifts_.push(entry);
}
} else if (entry.entryType === 'largest-contentful-paint') {
if (entry.loadTime) {
this.largestContentfulPaintLoadTime_ = entry.loadTime;
}
if (entry.renderTime) {
this.largestContentfulPaintRenderTime_ = entry.renderTime;
}
this.largestContentfulPaint_ = entry.startTime;
} else if (entry.entryType == 'navigation' && !recordedNavigation) {
[
'domComplete',
Expand Down Expand Up @@ -559,24 +549,18 @@ export class Performance {
* Tick the largest contentful paint metrics.
*/
tickLargestContentfulPaint_() {
/** @type {?number} */ let end;
if (this.largestContentfulPaintLoadTime_ !== null) {
this.tickDelta(
TickLabel.LARGEST_CONTENTFUL_PAINT_LOAD,
this.largestContentfulPaintLoadTime_
);
end = this.largestContentfulPaintLoadTime_;
}
if (this.largestContentfulPaintRenderTime_ !== null) {
this.tickDelta(
TickLabel.LARGEST_CONTENTFUL_PAINT_RENDER,
this.largestContentfulPaintRenderTime_
);
end = end || this.largestContentfulPaintRenderTime_;
}
if (end !== null) {
this.tickSinceVisible(TickLabel.LARGEST_CONTENTFUL_PAINT_VISIBLE, end);
if (this.largestContentfulPaint_ == null) {
return;
}

this.tickDelta(
TickLabel.LARGEST_CONTENTFUL_PAINT,
this.largestContentfulPaint_
);
this.tickSinceVisible(
TickLabel.LARGEST_CONTENTFUL_PAINT_VISIBLE,
this.largestContentfulPaint_
);
this.flush();
}

Expand Down
30 changes: 14 additions & 16 deletions test/unit/test-performance.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1025,29 +1025,31 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
perf.coreServicesAvailable();
expect(perf.events_.length).to.equal(0);

// Fake a largest-contentful-paint entry specifying a loadTime,
// simulating an image on a different origin without a proper
// Timing-Allow-Origin header.
// Fake a largest-contentful-paint entry specifying a renderTime/startTime,
// simulating an image on the same origin or with a Timing-Allow-Origin header.
performanceObserver.triggerCallback({
getEntries() {
return [
{
entryType: 'largest-contentful-paint',
loadTime: 10,
renderTime: 12,
startTime: 12,
},
];
},
});

// Fake a largest-contentful-paint entry specifying a renderTime,
// simulating an image on the same origin or with a proper
// Timing-Allow-Origin header.
// Fake a largest-contentful-paint entry with a loadTime/startTime
// simulating an image on a different origin without a Timing-Allow-Origin header.
performanceObserver.triggerCallback({
getEntries() {
return [
{
entryType: 'largest-contentful-paint',
renderTime: 23,
loadTime: 23,
renderTime: undefined,
startTime: 23,
},
];
},
Expand All @@ -1056,16 +1058,12 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
// The document has become hidden, e.g. via the user switching tabs.
toggleVisibility(perf, false);

const lcpEvents = perf.events_.filter((evt) =>
evt.label.startsWith('lcp')
const lcpEvents = perf.events_.filter(({label}) =>
label.startsWith('lcp')
);
expect(lcpEvents.length).to.equal(3);
expect(perf.events_).deep.include({
label: 'lcpl',
delta: 10,
});
expect(perf.events_).deep.include({
label: 'lcpr',
expect(lcpEvents.length).to.equal(2);
expect(lcpEvents).deep.include({
label: 'lcp',
delta: 23,
});
});
Expand Down

0 comments on commit 8662b2b

Please sign in to comment.