Skip to content

Commit

Permalink
performance-impl: use fcp and ofv entryTime when summing for cls-fcp …
Browse files Browse the repository at this point in the history 
…and cls-ofv (#33295)

* performance-impl: ensure cls and cls-fcp are ticked at the same time.

* performance-impl: use event times rather to ensure cls-ofv and cls-fcp are accurate

* fix tests + add a new one

* better fallback

* prevent unbounded growth
  • Loading branch information
@samouri
samouri committed Mar 17, 2021
1 parent 68021ad commit 78cfa79
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 46 deletions.
54 changes: 29 additions & 25 deletions src/service/performance-impl.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,10 @@ export class Performance {
this.shiftScoresTicked_ = 0;

/**
* The sum of all layout shift fractions triggered on the page from the
* Layout Instability API.
*
* @private {number}
* The collection of layout shift events from the Layout Instability API.
* @private {Array<LayoutShift>}
*/
this.aggregateShiftScore_ = 0;
this.layoutShifts_ = [];

const supportedEntryTypes =
(this.win.PerformanceObserver &&
Expand Down Expand Up @@ -241,11 +239,6 @@ export class Performance {

this.ampdoc_.whenFirstVisible().then(() => {
this.tick(TickLabel.ON_FIRST_VISIBLE);
// TODO(#33207): Remove after data collection
this.tick(
TickLabel.CUMULATIVE_LAYOUT_SHIFT_BEFORE_VISIBLE,
this.aggregateShiftScore_
);
this.flush();
});

Expand Down Expand Up @@ -355,13 +348,6 @@ export class Performance {
this.tickDelta(TickLabel.FIRST_CONTENTFUL_PAINT, value);
this.tickSinceVisible(TickLabel.FIRST_CONTENTFUL_PAINT_VISIBLE, value);
recordedFirstContentfulPaint = true;

// TODO(#33207): remove after data collection
// On the first contentful paint, report cumulative CLS score
this.tickDelta(
TickLabel.CUMULATIVE_LAYOUT_SHIFT_BEFORE_FCP,
this.aggregateShiftScore_
);
} else if (
entry.entryType === 'first-input' &&
!recordedFirstInputDelay
Expand All @@ -372,8 +358,9 @@ export class Performance {
} else if (entry.entryType === 'layout-shift') {
// Ignore layout shift that occurs within 500ms of user input, as it is
// likely in response to the user's action.
if (!entry.hadRecentInput) {
this.aggregateShiftScore_ += entry.value;
// 1000 here is a magic number to prevent unbounded growth. We don't expect it to be reached.
if (!entry.hadRecentInput && this.layoutShifts_.length < 1000) {
this.layoutShifts_.push(entry);
}
} else if (entry.entryType === 'largest-contentful-paint') {
if (entry.loadTime) {
Expand Down Expand Up @@ -524,18 +511,35 @@ export class Performance {
* amount of visibility into this metric.
*/
tickLayoutShiftScore_() {
const cls = this.layoutShifts_.reduce((sum, entry) => sum + entry.value, 0);
const fcp = this.metrics_.get(TickLabel.FIRST_CONTENTFUL_PAINT) ?? 0; // fallback to 0, so that we never overcount.
const ofv = this.metrics_.get(TickLabel.ON_FIRST_VISIBLE) ?? 0;

// TODO(#33207): Remove after data collection
const clsBeforeFCP = this.layoutShifts_.reduce((sum, entry) => {
if (entry.startTime < fcp) {
return sum + entry.value;
}
return sum;
}, 0);
const clsBeforeOFV = this.layoutShifts_.reduce((sum, entry) => {
if (entry.startTime < ofv) {
return sum + entry.value;
}
return sum;
}, 0);

if (this.shiftScoresTicked_ === 0) {
this.tick(TickLabel.CUMULATIVE_LAYOUT_SHIFT_BEFORE_VISIBLE, clsBeforeOFV);
this.tickDelta(
TickLabel.CUMULATIVE_LAYOUT_SHIFT,
this.aggregateShiftScore_
TickLabel.CUMULATIVE_LAYOUT_SHIFT_BEFORE_FCP,
clsBeforeFCP
);
this.tickDelta(TickLabel.CUMULATIVE_LAYOUT_SHIFT, cls);
this.flush();
this.shiftScoresTicked_ = 1;
} else if (this.shiftScoresTicked_ === 1) {
this.tickDelta(
TickLabel.CUMULATIVE_LAYOUT_SHIFT_2,
this.aggregateShiftScore_
);
this.tickDelta(TickLabel.CUMULATIVE_LAYOUT_SHIFT_2, cls);
this.flush();
this.shiftScoresTicked_ = 2;

Expand Down
113 changes: 92 additions & 21 deletions test/unit/test-performance.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ describes.realWin('performance', {amp: true}, (env) => {
]).then(() => {
expect(flushSpy).to.have.callCount(4);
expect(perf.isMessagingReady_).to.be.false;
const count = 6;
const count = 5;
expect(perf.events_.length).to.equal(count);
});
});
Expand Down Expand Up @@ -653,15 +653,13 @@ describes.realWin('performance', {amp: true}, (env) => {
const initialCount = tickSpy.callCount;
return ampdoc.whenFirstVisible().then(() => {
clock.tick(400);
// TODO(#33207): reduce call counts by 1 after data collection
expect(tickSpy).to.have.callCount(initialCount + 2);
expect(tickSpy.withArgs('cls-ofv')).to.be.calledOnce;
expect(tickSpy).to.have.callCount(initialCount + 1);
whenViewportLayoutCompleteResolve();
return perf.whenViewportLayoutComplete_().then(() => {
expect(tickSpy).to.have.callCount(initialCount + 2);
expect(tickSpy).to.have.callCount(initialCount + 1);
expect(tickSpy.withArgs('ofv')).to.be.calledOnce;
return whenFirstVisiblePromise.then(() => {
expect(tickSpy).to.have.callCount(initialCount + 3);
expect(tickSpy).to.have.callCount(initialCount + 2);
expect(tickSpy.withArgs('pc')).to.be.calledOnce;
expect(Number(tickSpy.withArgs('pc').args[0][1])).to.equal(400);
});
Expand Down Expand Up @@ -851,6 +849,8 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
let windowEventListeners;
let performanceObserver;
let viewerVisibilityState;
let whenFirstVisiblePromise;
let whenFirstVisibleResolve;

function setupFakesForVisibilityStateManipulation() {
// Fake window to fake `document.visibilityState`.
Expand Down Expand Up @@ -900,12 +900,15 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
// Install services on fakeWin so some behaviors can be stubbed.
installRuntimeServices(fakeWin);

whenFirstVisiblePromise = new Promise((resolve) => {
whenFirstVisibleResolve = resolve;
});
const unresolvedPromise = new Promise(() => {});
const viewportSize = {width: 0, height: 0};
env.sandbox.stub(Services, 'ampdoc').returns({
hasBeenVisible: () => {},
onVisibilityChanged: () => {},
whenFirstVisible: () => unresolvedPromise,
whenFirstVisible: () => whenFirstVisiblePromise,
getVisibilityState: () => viewerVisibilityState,
getFirstVisibleTime: () => 0,
isSingleDoc: () => true,
Expand Down Expand Up @@ -967,7 +970,7 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {

const perf = Services.performanceFor(env.win);

expect(perf.events_.length).to.equal(4);
expect(perf.events_.length).to.equal(3);
expect(perf.events_[0]).to.be.jsonEqual(
{
label: 'fp',
Expand All @@ -980,10 +983,6 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
{
label: 'fcpv',
delta: 15,
},
{
label: 'cls-fcp',
delta: 15,
}
);

Expand Down Expand Up @@ -1033,7 +1032,7 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
};
// Fake a triggering of the first-input event.
performanceObserver.triggerCallback(list);
expect(perf.events_.length).to.equal(4);
expect(perf.events_.length).to.equal(3);
expect(perf.events_[0]).to.be.jsonEqual(
{
label: 'fp',
Expand All @@ -1046,10 +1045,6 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
{
label: 'fcpv',
delta: 15,
},
{
label: 'cls-fcp',
delta: 15,
}
);
});
Expand Down Expand Up @@ -1223,6 +1218,82 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
(windowEventListeners[eventName] || []).forEach((cb) => cb(event));
}

// TODO(#33207): Remove after data collection
it('Forwards cls-fcp and cls-ofv', async () => {
fakeWin.PerformanceObserver.supportedEntryTypes = [
'layout-shift',
'paint',
];
fakeWin.document.visibilityState = 'hidden';

// Document should be initially hidden.
expect(fakeWin.document.visibilityState).to.equal('hidden');

// visibilitychange/beforeunload listeners are now added.
const perf = getPerformance();
perf.coreServicesAvailable();

// Pre-visible and pre-fcp layout shifts
performanceObserver.triggerCallback({
getEntries() {
return [
{
entryType: 'layout-shift',
value: 0.25,
hadRecentInput: false,
startTime: 0,
},
{
entryType: 'layout-shift',
value: 0.25,
hadRecentInput: false,
startTime: 50,
},
];
},
});

whenFirstVisibleResolve();
await new Promise(setTimeout);
toggleVisibility(fakeWin, true);

// Post visible, pre-fcp layout-shift
performanceObserver.triggerCallback({
getEntries() {
return [
{
entryType: 'layout-shift',
value: 0.5,
hadRecentInput: false,
startTime: 100,
},
{
entryType: 'paint',
name: 'first-contentful-paint',
startTime: 150,
duration: 0,
},
{
entryType: 'layout-shift',
value: 0.5,
hadRecentInput: false,
startTime: 200,
},
];
},
});

toggleVisibility(fakeWin, false);
const clsEvents = perf.events_.filter((event) =>
event.label.startsWith('cls')
);
expect(clsEvents).jsonEqual([
{label: 'cls-ofv', delta: 0.5},
{label: 'cls-fcp', delta: 1},
{label: 'cls', delta: 1.5},
]);
});

it('for Chromium 77', () => {
// Specify an Android Chrome user agent, which supports the
// visibilitychange event.
Expand Down Expand Up @@ -1258,7 +1329,7 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
toggleVisibility(fakeWin, false);
let clsEvents = perf.events_.filter((event) => event.label === 'cls');
expect(clsEvents.length).equal(1);
expect(perf.events_[0]).to.be.jsonEqual({
expect(perf.events_).deep.includes({
label: 'cls',
delta: 0.55,
});
Expand All @@ -1285,7 +1356,7 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {

toggleVisibility(fakeWin, false);
clsEvents = perf.events_.filter((event) => event.label.startsWith('cls'));
expect(clsEvents.length).to.equal(2);
expect(clsEvents.length).to.equal(4);
expect(clsEvents).to.deep.include({
label: 'cls-2',
delta: 1.5501,
Expand All @@ -1301,7 +1372,7 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {

toggleVisibility(fakeWin, false);
clsEvents = perf.events_.filter((event) => event.label.startsWith('cls'));
expect(clsEvents.length).to.equal(2);
expect(clsEvents.length).to.equal(4);
});

it('when the viewer visibility changes to inactive', () => {
Expand Down Expand Up @@ -1336,7 +1407,7 @@ describes.realWin('PeformanceObserver metrics', {amp: true}, (env) => {
const clsEvents = perf.events_.filter((evt) =>
evt.label.startsWith('cls')
);
expect(clsEvents.length).to.equal(1);
expect(clsEvents.length).to.equal(3);
expect(perf.events_).deep.include({
label: 'cls',
delta: 0.55,
Expand Down

0 comments on commit 78cfa79

Please sign in to comment.