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

core(cumulative-layout-shift): experiment with new shared trace engine #15702

Merged
merged 9 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 14 additions & 4 deletions cli/test/smokehouse/__snapshots__/report-assert-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`getAssertionReport works (multiple failing) 1`] = `
"X difference at cumulative-layout-shift audit.details.items.length
expected: []
found: [{\\"cumulativeLayoutShiftMainFrame\\":0.13570762803819444}]
found: [{\\"cumulativeLayoutShiftMainFrame\\":0.13570762803819444,\\"newEngineResult\\":{\\"cumulativeLayoutShift\\":0.13570762803819444,\\"cumulativeLayoutShiftMainFrame\\":0.13570762803819444},\\"newEngineResultDiffered\\":false}]


X difference at cumulative-layout-shift audit.details.blah
Expand All @@ -28,7 +28,12 @@ exports[`getAssertionReport works (multiple failing) 1`] = `
\\"type\\": \\"debugdata\\",
\\"items\\": [
{
\\"cumulativeLayoutShiftMainFrame\\": 0.13570762803819444
\\"cumulativeLayoutShiftMainFrame\\": 0.13570762803819444,
\\"newEngineResult\\": {
\\"cumulativeLayoutShift\\": 0.13570762803819444,
\\"cumulativeLayoutShiftMainFrame\\": 0.13570762803819444
},
\\"newEngineResultDiffered\\": false
}
]
}
Expand All @@ -38,7 +43,7 @@ exports[`getAssertionReport works (multiple failing) 1`] = `
exports[`getAssertionReport works (trivial failing) 1`] = `
"X difference at cumulative-layout-shift audit.details.items.length
expected: []
found: [{\\"cumulativeLayoutShiftMainFrame\\":0.13570762803819444}]
found: [{\\"cumulativeLayoutShiftMainFrame\\":0.13570762803819444,\\"newEngineResult\\":{\\"cumulativeLayoutShift\\":0.13570762803819444,\\"cumulativeLayoutShiftMainFrame\\":0.13570762803819444},\\"newEngineResultDiffered\\":false}]

found result:
{
Expand All @@ -58,7 +63,12 @@ exports[`getAssertionReport works (trivial failing) 1`] = `
\\"type\\": \\"debugdata\\",
\\"items\\": [
{
\\"cumulativeLayoutShiftMainFrame\\": 0.13570762803819444
\\"cumulativeLayoutShiftMainFrame\\": 0.13570762803819444,
\\"newEngineResult\\": {
\\"cumulativeLayoutShift\\": 0.13570762803819444,
\\"cumulativeLayoutShiftMainFrame\\": 0.13570762803819444
},
\\"newEngineResultDiffered\\": false
}
]
}
Expand Down
78 changes: 74 additions & 4 deletions core/computed/metrics/cumulative-layout-shift.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
import {makeComputedArtifact} from '../computed-artifact.js';
import {ProcessedTrace} from '../processed-trace.js';
import * as RectHelpers from '../../lib/rect-helpers.js';
import * as TraceEngine from '../../lib/trace-engine.js';
import {Sentry} from '../../lib/sentry.js';

/** @typedef {{ts: number, isMainFrame: boolean, weightedScore: number, impactedNodes?: LH.Artifacts.TraceImpactedNode[]}} LayoutShiftEvent */
/** @typedef {{ts: number, isMainFrame: boolean, weightedScore: number, impactedNodes?: LH.Artifacts.TraceImpactedNode[], event: LH.TraceEvent}} LayoutShiftEvent */

const RECENT_INPUT_WINDOW = 500;

Expand Down Expand Up @@ -67,6 +69,7 @@
isMainFrame: event.args.data.is_main_frame,
weightedScore: event.args.data.weighted_score_delta,
impactedNodes: event.args.data.impacted_nodes,
event,
});
}

Expand Down Expand Up @@ -145,10 +148,29 @@
return maxScore;
}

/**
* @param {LayoutShiftEvent[]} allFrameShiftEvents
* @param {LayoutShiftEvent[]} mainFrameShiftEvents
*/
static async computeWithSharedTraceEngine(allFrameShiftEvents, mainFrameShiftEvents) {
/** @param {LH.TraceEvent[]} events */
const run = async (events) => {
const processor = new TraceEngine.TraceProcessor({
LayoutShifts: TraceEngine.TraceHandlers.LayoutShifts,
Screenshots: TraceEngine.TraceHandlers.Screenshots,
});
await processor.parse(events);
return processor.data.LayoutShifts.sessionMaxScore;
};
const cumulativeLayoutShift = await run(allFrameShiftEvents.map(e => e.event));
const cumulativeLayoutShiftMainFrame = await run(mainFrameShiftEvents.map(e => e.event));
return {cumulativeLayoutShift, cumulativeLayoutShiftMainFrame};
}

/**
* @param {LH.Trace} trace
* @param {LH.Artifacts.ComputedContext} context
* @return {Promise<{cumulativeLayoutShift: number, cumulativeLayoutShiftMainFrame: number, impactByNodeId: Map<number, number>}>}
* @return {Promise<{cumulativeLayoutShift: number, cumulativeLayoutShiftMainFrame: number, impactByNodeId: Map<number, number>, newEngineResult?: {cumulativeLayoutShift: number, cumulativeLayoutShiftMainFrame: number}, newEngineResultDiffered: boolean}>}
*/
static async compute_(trace, context) {
const processedTrace = await ProcessedTrace.request(trace, context);
Expand All @@ -157,11 +179,59 @@
CumulativeLayoutShift.getLayoutShiftEvents(processedTrace);
const impactByNodeId = CumulativeLayoutShift.getImpactByNodeId(allFrameShiftEvents);
const mainFrameShiftEvents = allFrameShiftEvents.filter(e => e.isMainFrame);
const cumulativeLayoutShift = CumulativeLayoutShift.calculate(allFrameShiftEvents);
const cumulativeLayoutShiftMainFrame = CumulativeLayoutShift.calculate(mainFrameShiftEvents);

// Run with the new trace engine, and only throw an error if we are running our unit tests.
// Otherwise, simply report any differences or errors to Sentry.
// TODO: TraceEngine always drops `had_recent_input` events, but Lighthouse is more lenient.
Copy link
Member

@brendankenny brendankenny Dec 19, 2023

Choose a reason for hiding this comment

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

I always get mixed up on this so just correct me if I'm wrong, but isn't it less lenient?

Some events may have had_recent_input: true so wouldn't be counted by Chrome/DT toward the CLS total, but Lighthouse includes those events (potentially making CLS worse) because it wasn't really input, it was the emulation viewport change?

edit: I guess unless it's saying it's more lenient from the perspective of the events getting filtered out, haha

Copy link
Collaborator Author

@connorjclark connorjclark Dec 19, 2023

Choose a reason for hiding this comment

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

yeah, the filter itself is more lenient in what it allows through. It's a lot of summarize so I just have the comment defer to "the comment in getLayoutShiftEvents"

// See comment in `getLayoutShiftEvents`. We need to upstream this.
let newEngineResult;
let newEngineResultDiffered = false;
let tryNewTraceEngine = true;
if (allFrameShiftEvents.some(e => e.event.args.data?.had_recent_input)) {
tryNewTraceEngine = false;
}
if (tryNewTraceEngine) {
try {
newEngineResult =
await this.computeWithSharedTraceEngine(allFrameShiftEvents, mainFrameShiftEvents);
newEngineResultDiffered =
newEngineResult.cumulativeLayoutShift !== cumulativeLayoutShift ||
newEngineResult.cumulativeLayoutShiftMainFrame !== cumulativeLayoutShiftMainFrame;
if (newEngineResultDiffered) {
newEngineResultDiffered = true;
const expected = JSON.stringify({cumulativeLayoutShift, cumulativeLayoutShiftMainFrame});
const got = JSON.stringify(newEngineResult);
throw new Error(`new trace engine differed. expected: ${expected}, got: ${got}`);
}

Check warning on line 207 in core/computed/metrics/cumulative-layout-shift.js

View check run for this annotation

Codecov / codecov/patch

core/computed/metrics/cumulative-layout-shift.js#L203-L207

Added lines #L203 - L207 were not covered by tests
} catch (err) {
console.error(err);
newEngineResultDiffered = true;

const error = new Error('Error when using new trace engine', {cause: err});
// @ts-expect-error Check for running from tests.
if (global.expect) {
throw error;
} else {
Sentry.captureException(error, {
tags: {computed: 'new-trace-engine'},
level: 'error',
extra: {
// Not sure if Sentry handles `cause`, so just in case add the info in a second place.
errorMsg: err.toString(),
},
});
}
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
}

Check warning on line 226 in core/computed/metrics/cumulative-layout-shift.js

View check run for this annotation

Codecov / codecov/patch

core/computed/metrics/cumulative-layout-shift.js#L209-L226

Added lines #L209 - L226 were not covered by tests
}

return {
cumulativeLayoutShift: CumulativeLayoutShift.calculate(allFrameShiftEvents),
cumulativeLayoutShiftMainFrame: CumulativeLayoutShift.calculate(mainFrameShiftEvents),
cumulativeLayoutShift,
cumulativeLayoutShiftMainFrame,
impactByNodeId,
newEngineResult,
newEngineResultDiffered,
};
}
}
Expand Down
111 changes: 111 additions & 0 deletions core/lib/polyfill-dom-rect.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
// @ts-nocheck
/* eslint-disable */

function polyfillDOMRect() {
// devtools assumes clientside :(

// Everything else in here is the DOMRect polyfill
// https://raw.githubusercontent.com/JakeChampion/polyfill-library/master/polyfills/DOMRect/polyfill.js

(function (global) {
function number(v) {
return v === undefined ? 0 : Number(v);
}

Check warning on line 13 in core/lib/polyfill-dom-rect.js

View check run for this annotation

Codecov / codecov/patch

core/lib/polyfill-dom-rect.js#L12-L13

Added lines #L12 - L13 were not covered by tests

function different(u, v) {
return u !== v && !(isNaN(u) && isNaN(v));
}

Check warning on line 17 in core/lib/polyfill-dom-rect.js

View check run for this annotation

Codecov / codecov/patch

core/lib/polyfill-dom-rect.js#L16-L17

Added lines #L16 - L17 were not covered by tests

function DOMRect(xArg, yArg, wArg, hArg) {
let x, y, width, height, left, right, top, bottom;

x = number(xArg);
y = number(yArg);
width = number(wArg);
height = number(hArg);

Object.defineProperties(this, {
x: {
get: function () { return x; },
set: function (newX) {
if (different(x, newX)) {
x = newX;
left = right = undefined;
}
},
enumerable: true
},
y: {
get: function () { return y; },
set: function (newY) {
if (different(y, newY)) {
y = newY;
top = bottom = undefined;
}
},
enumerable: true
},
width: {
get: function () { return width; },
set: function (newWidth) {
if (different(width, newWidth)) {
width = newWidth;
left = right = undefined;
}
},
enumerable: true
},
height: {
get: function () { return height; },
set: function (newHeight) {
if (different(height, newHeight)) {
height = newHeight;
top = bottom = undefined;
}
},
enumerable: true
},
left: {
get: function () {
if (left === undefined) {
left = x + Math.min(0, width);
}
return left;
},
enumerable: true
},
right: {
get: function () {
if (right === undefined) {
right = x + Math.max(0, width);
}
return right;
},
enumerable: true
},
top: {
get: function () {
if (top === undefined) {
top = y + Math.min(0, height);
}
return top;
},
enumerable: true
},
bottom: {
get: function () {
if (bottom === undefined) {
bottom = y + Math.max(0, height);
}
return bottom;
},
enumerable: true
}
});
}

Check warning on line 105 in core/lib/polyfill-dom-rect.js

View check run for this annotation

Codecov / codecov/patch

core/lib/polyfill-dom-rect.js#L20-L105

Added lines #L20 - L105 were not covered by tests

globalThis.DOMRect = DOMRect;
})(globalThis);
}

export {polyfillDOMRect};
16 changes: 16 additions & 0 deletions core/lib/trace-engine.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// @ts-expect-error missing types
import * as TraceEngine from '@paulirish/trace_engine';

import {polyfillDOMRect} from './polyfill-dom-rect.js';

polyfillDOMRect();

const TraceProcessor = TraceEngine.Processor.TraceProcessor;
const TraceHandlers = TraceEngine.Handlers.ModelHandlers;
const RootCauses = TraceEngine.RootCauses.RootCauses.RootCauses;

export {
TraceProcessor,
TraceHandlers,
RootCauses,
};
Loading
Loading