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 5 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
53 changes: 50 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 @@ -158,9 +161,53 @@
const impactByNodeId = CumulativeLayoutShift.getImpactByNodeId(allFrameShiftEvents);
const mainFrameShiftEvents = allFrameShiftEvents.filter(e => e.isMainFrame);

let useNewTraceEngine = true;

// 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`.
if (allFrameShiftEvents.some(e => e.event.args.data?.had_recent_input)) {
useNewTraceEngine = false;
}

let cumulativeLayoutShift;
let cumulativeLayoutShiftMainFrame;
if (useNewTraceEngine) {
/** @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;
};

try {
cumulativeLayoutShift = await run(allFrameShiftEvents.map(e => e.event));
cumulativeLayoutShiftMainFrame = await run(mainFrameShiftEvents.map(e => e.event));
} catch (e) {
console.error(e);
useNewTraceEngine = false;
// @ts-expect-error Checking for running from tests.
if (global.expect) {
throw new Error('Error when using new trace engine');
} else {
Sentry.captureException(e, {
tags: {computed: 'new-trace-engine'},
level: 'error',
});
}
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
}

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

View check run for this annotation

Codecov / codecov/patch

core/computed/metrics/cumulative-layout-shift.js#L189-L200

Added lines #L189 - L200 were not covered by tests
}

if (!useNewTraceEngine) {
cumulativeLayoutShift = CumulativeLayoutShift.calculate(allFrameShiftEvents);
cumulativeLayoutShiftMainFrame = CumulativeLayoutShift.calculate(mainFrameShiftEvents);
}

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