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 6 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
70 changes: 67 additions & 3 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,6 +148,25 @@
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
Expand All @@ -157,10 +179,52 @@
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 tryNewTraceEngine = true;
if (allFrameShiftEvents.some(e => e.event.args.data?.had_recent_input)) {
tryNewTraceEngine = false;
}
if (tryNewTraceEngine) {
try {
const newEngineResult =
await this.computeWithSharedTraceEngine(allFrameShiftEvents, mainFrameShiftEvents);
const differ =
newEngineResult.cumulativeLayoutShift !== cumulativeLayoutShift ||
newEngineResult.cumulativeLayoutShiftMainFrame !== cumulativeLayoutShiftMainFrame;
if (differ) {
Copy link
Member

Choose a reason for hiding this comment

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

Could stick it in debugData as well if it would be interesting to compare in HTTP Archive to get ballpark numbers ("out of 7M or whatever runs where both approaches ran, there were no differences"). That would require passing it back in the computed artifact, but having something in the return value would also be an avenue for a test asserting that this path is running.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. Can I leave it like this or should I flatten it out?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also added a newEngineResultDiffered boolean to help with querying

Copy link
Member

Choose a reason for hiding this comment

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

leave it like this or should I flatten it out

probably like this is more ergonomic in this file and it doesn't really matter in HTTP Archive since it's querying into JSON either way

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 204 in core/computed/metrics/cumulative-layout-shift.js

View check run for this annotation

Codecov / codecov/patch

core/computed/metrics/cumulative-layout-shift.js#L201-L204

Added lines #L201 - L204 were not covered by tests
} catch (err) {
console.error(err);

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 222 in core/computed/metrics/cumulative-layout-shift.js

View check run for this annotation

Codecov / codecov/patch

core/computed/metrics/cumulative-layout-shift.js#L206-L222

Added lines #L206 - L222 were not covered by tests
}

return {
cumulativeLayoutShift: CumulativeLayoutShift.calculate(allFrameShiftEvents),
cumulativeLayoutShiftMainFrame: CumulativeLayoutShift.calculate(mainFrameShiftEvents),
cumulativeLayoutShift,
cumulativeLayoutShiftMainFrame,
impactByNodeId,
};
}
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,
};
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@
"webtreemap-cdt": "^3.2.1"
},
"dependencies": {
"@paulirish/trace_engine": "^0.0.7",
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't really matter regardless so just out of curiosity, what's the bundle size impact? 😬

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.

@paulirish can this be revived? https://lh-build-tracker.herokuapp.com/

the devtools bundle: 1948329 -> 2069598 (+118KB), pre-compression

"@sentry/node": "^6.17.4",
"axe-core": "^4.8.1",
"chrome-launcher": "^1.1.0",
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1075,6 +1075,11 @@
"@nodelib/fs.scandir" "2.1.3"
fastq "^1.6.0"

"@paulirish/trace_engine@^0.0.7":
version "0.0.7"
resolved "https://registry.yarnpkg.com/@paulirish/trace_engine/-/trace_engine-0.0.7.tgz#c9494729f39a1c742325726474532aabfdee6f1b"
integrity sha512-TC639y9YlejyQcTG6LqJ3GJGuFfV9IJTBRsxyl+bGqxkfzyqJ+LG00fOht2FgnfwkCHImlIFBg4HDrX+2LHPHQ==

"@protobufjs/aspromise@^1.1.1", "@protobufjs/aspromise@^1.1.2":
version "1.1.2"
resolved "https://registry.yarnpkg.com/@protobufjs/aspromise/-/aspromise-1.1.2.tgz#9b8b0cc663d669a7d8f6f5d0893a14d348f30fbf"
Expand Down
Loading