Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

core(root-causes): grab from trace insights rather than use protocol #16352

Merged
merged 10 commits into from
Feb 24, 2025
Merged

core(root-causes): grab from trace insights rather than use protocol #16352

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
42fd3af
core(root-causes): grab from trace insights rather than use protocol
connorjclark Feb 24, 2025
3b77277
oops
connorjclark Feb 24, 2025
32dad93
Merge remote-tracking branch 'origin/main' into root-causes-te
connorjclark Feb 24, 2025
102b236
fix smoke
connorjclark Feb 24, 2025
55ae6e0
remove unfixable test
connorjclark Feb 24, 2025
b5217ca
update
connorjclark Feb 24, 2025
8f6b64a
fix getTraceEngineElements
connorjclark Feb 24, 2025
79f8301
rm artifact
connorjclark Feb 24, 2025
1a987ec
Merge remote-tracking branch 'origin/main' into root-causes-te
connorjclark Feb 24, 2025
9858043
typesssss
connorjclark Feb 24, 2025
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
6 changes: 0 additions & 6 deletions cli/test/smokehouse/test-definitions/perf-trace-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,6 @@ const expectations = {
nodeLabel: `Please don't move me`,
},
},
{
traceEventType: 'layout-shift',
node: {
nodeLabel: 'section > img',
},
},
{
traceEventType: 'animation',
node: {
Expand Down
12 changes: 4 additions & 8 deletions cli/test/smokehouse/test-definitions/shift-attribution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,11 @@ const expectations = {
_includes: [
{
node: {selector: 'body > div#blue'},
subItems: {items: [{cause: /font/, extra: {value: /Regular\.ttf/}}]},
subItems: {items: [
{cause: /Media/, extra: {selector: 'body > img'}},
{cause: /font/, extra: {value: /Regular\.ttf/}},
]},
},
// iframe root causes are disabled due to performance issues
// https://github.com/GoogleChrome/lighthouse/issues/15869
// {
// node: {selector: 'body > div#blue'},
// // TODO: We can't get nodes from non-main frames yet. See runRootCauseAnalysis.
// subItems: {items: [{cause: /iframe/, extra: undefined}]},
Copy link
Member

Choose a reason for hiding this comment

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

We should get an iframe root cause now. The iframe root causes were only disabled for the old root causes engine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not there. Keep in mind the implementations of these differ.

Copy link
Member

Choose a reason for hiding this comment

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

What implementation difference? If this isn't showing up then maybe we have a bug in the TE?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i don't know. it'd be a lot of work to understand this and I think I can't prioritize looking into it.

#16354

// },
{
node: {selector: 'body > div#blue'},
subItems: {items: [{cause: /Media/, extra: {selector: 'body > img'}}]},
Expand Down
25 changes: 6 additions & 19 deletions core/audits/layout-shifts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ const UIStrings = {
rootCauseFontChanges: 'Web font loaded',
/** A possible reason why that the layout shift occured. */
rootCauseInjectedIframe: 'Injected iframe',
/** A possible reason why that the layout shift occured. */
rootCauseRenderBlockingRequest: 'A late network request adjusted the page layout',
/** Label shown per-audit to show how many layout shifts are present. The `{# shifts found}` placeholder will be replaced with the number of layout shifts. */
displayValueShiftsFound: `{shiftCount, plural, =1 {1 layout shift found} other {# layout shifts found}}`,
};
Expand Down Expand Up @@ -82,41 +80,30 @@ class LayoutShifts extends Audit {
const biggestImpactElement = traceElements.find(t => t.nodeId === biggestImpactNodeId);

// Turn root causes into sub-items.
const index = layoutShiftEvents.indexOf(event);
const rootCauses = artifacts.RootCauses.layoutShifts[index];
const rootCauses = artifacts.RootCauses.layoutShifts.get(event);
/** @type {SubItem[]} */
const subItems = [];
if (rootCauses) {
for (const cause of rootCauses.unsizedMedia) {
for (const backendNodeId of rootCauses.unsizedImages) {
const element = artifacts.TraceElements.find(
t => t.traceEventType === 'layout-shift' && t.nodeId === cause.node.backendNodeId);
t => t.traceEventType === 'trace-engine' && t.nodeId === backendNodeId);
subItems.push({
extra: element ? Audit.makeNodeItem(element.node) : undefined,
cause: str_(UIStrings.rootCauseUnsizedMedia),
});
}
for (const cause of rootCauses.fontChanges) {
const url = cause.request.args.data.url;
for (const request of rootCauses.fontRequests) {
const url = request.args.data.url;
subItems.push({
extra: {type: 'url', value: url},
cause: str_(UIStrings.rootCauseFontChanges),
});
}
for (const cause of rootCauses.iframes) {
const element = artifacts.TraceElements.find(
t => t.traceEventType === 'layout-shift' && t.nodeId === cause.iframe.backendNodeId);
if (rootCauses.iframeIds.length) {
subItems.push({
extra: element ? Audit.makeNodeItem(element.node) : undefined,
cause: str_(UIStrings.rootCauseInjectedIframe),
});
}
for (const cause of rootCauses.renderBlockingRequests) {
const url = cause.request.args.data.url;
subItems.push({
extra: {type: 'url', value: url},
cause: str_(UIStrings.rootCauseRenderBlockingRequest),
});
}
}

items.push({
Expand Down
117 changes: 8 additions & 109 deletions core/gather/gatherers/root-causes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import BaseGatherer from '../base-gatherer.js';
import Trace from './trace.js';
import * as TraceEngine from '../../lib/trace-engine.js';
import {TraceEngineResult} from '../../computed/trace-engine-result.js';

class RootCauses extends BaseGatherer {
Expand All @@ -20,125 +19,25 @@ class RootCauses extends BaseGatherer {
};

/**
* @param {LH.Gatherer.Driver} driver
* @param {LH.Artifacts.TraceEngineResult['data']} traceParsedData
* @param {LH.Gatherer.Context<'Trace'>} context
* @return {Promise<LH.Artifacts.TraceEngineRootCauses>}
*/
static async runRootCauseAnalysis(driver, traceParsedData) {
await driver.defaultSession.sendCommand('DOM.enable');
await driver.defaultSession.sendCommand('CSS.enable');

// DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve
// nodeIds if the DOM domain was enabled before this gatherer, invoke it to be safe.
await driver.defaultSession.sendCommand('DOM.getDocument', {depth: -1, pierce: true});

/** @type {import('@paulirish/trace_engine').RootCauses.RootCauses.RootCauseProtocolInterface} */
const protocolInterface = {
/** @param {string} url */
// eslint-disable-next-line no-unused-vars
getInitiatorForRequest(url) {
return null;
},
/** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.BackendNodeId[]} backendNodeIds */
async pushNodesByBackendIdsToFrontend(backendNodeIds) {
const response = await driver.defaultSession.sendCommand(
'DOM.pushNodesByBackendIdsToFrontend', {backendNodeIds});
const nodeIds =
/** @type {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId[]} */(
response.nodeIds);
return nodeIds;
},
/** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId} nodeId */
async getNode(nodeId) {
try {
const response = await driver.defaultSession.sendCommand('DOM.describeNode', {nodeId});
// This always zero, so let's fix it here.
// https://bugs.chromium.org/p/chromium/issues/detail?id=1515175
response.node.nodeId = nodeId;
const node =
/** @type {import('@paulirish/trace_engine/generated/protocol.js').DOM.Node} */(
response.node);
return node;
} catch (err) {
if (err.message.includes('Could not find node with given id')) {
// TODO: when injecting an iframe, the engine gets the node of that frame's document element.
// But we don't have a way to access that frame. We just have our default session.
// Ex:
// node cli http://localhost:10503/shift-attribution.html --quiet --only-audits layout-shifts
// To fix we must:
// - Change trace engine `getNode` protocol interface to also give frame id
// - Expand our driver.targetManager to know how to talk to a session connected to a specific frame
// When this is fixed, remove this try/catch.
// Note: this could be buggy by giving the wrong node detail if a node id meant for a non-main frame
// happens to match one from the main frame ... which is pretty likely...
// TODO: fix trace engine type to allow returning null.
return /** @type {any} */(null);
}
throw err;
}
},
/** @param {number} nodeId */
async getComputedStyleForNode(nodeId) {
try {
const response = await driver.defaultSession.sendCommand(
'CSS.getComputedStyleForNode', {nodeId});
return response.computedStyle;
} catch {
return [];
}
},
/** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId} nodeId */
async getMatchedStylesForNode(nodeId) {
try {
const response = await driver.defaultSession.sendCommand(
'CSS.getMatchedStylesForNode', {nodeId});
return {...response, getError() {}};
} catch (err) {
return /** @type {any} */({getError() {
return err.toString();
}});
}
},
/** @param {string} url */
// @ts-expect-error not using, dont care about type error.
// eslint-disable-next-line no-unused-vars
async fontFaceForSource(url) {
return null;
},
};
async getArtifact(context) {
const trace = context.dependencies.Trace;
const traceEngineResult = await TraceEngineResult.request({trace}, context);

/** @type {LH.Artifacts.TraceEngineRootCauses} */
const rootCauses = {
layoutShifts: {},
layoutShifts: new Map(),
};
const rootCausesEngine = new TraceEngine.RootCauses(protocolInterface);
const layoutShiftEvents = traceParsedData.LayoutShifts.clusters.flatMap(c => c.events);
for (const event of layoutShiftEvents) {
const r = await rootCausesEngine.layoutShifts.rootCausesForEvent(traceParsedData, event);
if (!r) continue;

for (const cause of r.fontChanges) {
// TODO: why isn't trace engine unwrapping this promise ...
cause.fontFace = await cause.fontFace;
for (const insightSet of traceEngineResult.insights.values()) {
for (const [shift, reasons] of insightSet.model.CLSCulprits.shifts) {
rootCauses.layoutShifts.set(shift, reasons);
}
rootCauses.layoutShifts[layoutShiftEvents.indexOf(event)] = r;
}

await driver.defaultSession.sendCommand('DOM.disable');
await driver.defaultSession.sendCommand('CSS.disable');

return rootCauses;
}

/**
* @param {LH.Gatherer.Context<'Trace'>} context
* @return {Promise<LH.Artifacts.TraceEngineRootCauses>}
*/
async getArtifact(context) {
const trace = context.dependencies.Trace;
const traceEngineResult = await TraceEngineResult.request({trace}, context);
return RootCauses.runRootCauseAnalysis(context.driver, traceEngineResult.data);
}
}

export default RootCauses;
42 changes: 25 additions & 17 deletions core/gather/gatherers/trace-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,23 @@ class TraceElements extends BaseGatherer {
seen.add(obj);

if (obj && typeof obj === 'object' && !Array.isArray(obj)) {
Object.keys(obj).forEach(key => {
if (typeof obj[key] === 'object') {
recursiveObjectEnumerate(obj[key], cb, seen);
} else {
cb(obj, key);
if (obj instanceof Map) {
for (const [key, val] of obj) {
if (typeof val === 'object') {
recursiveObjectEnumerate(val, cb, seen);
} else {
cb(val, key);
}
}
});
} else {
Object.keys(obj).forEach(key => {
if (typeof obj[key] === 'object') {
recursiveObjectEnumerate(obj[key], cb, seen);
} else {
cb(obj[key], key);
}
});
}
} else if (Array.isArray(obj)) {
obj.forEach(item => {
if (typeof item === 'object' || Array.isArray(item)) {
Expand All @@ -121,12 +131,18 @@ class TraceElements extends BaseGatherer {

/** @type {number[]} */
const nodeIds = [];
recursiveObjectEnumerate(insightSet.model, (obj, key) => {
if (typeof obj[key] === 'number' && (key === 'nodeId' || key === 'node_id')) {
nodeIds.push(obj[key]);
recursiveObjectEnumerate(insightSet.model, (val, key) => {
const keys = ['nodeId', 'node_id'];
if (typeof val === 'number' && keys.includes(key)) {
nodeIds.push(val);
}
}, new Set());

// TODO: would be better if unsizedImages was `Array<{nodeId}>`.
for (const shift of insightSet.model.CLSCulprits.shifts.values()) {
nodeIds.push(...shift.unsizedImages);
}

return [...new Set(nodeIds)].map(id => ({nodeId: id}));
}

Expand Down Expand Up @@ -212,14 +228,6 @@ class TraceElements extends BaseGatherer {
nodeIds.push(biggestImpactedNodeId);
}

const index = layoutShiftEvents.indexOf(event);
const shiftRootCauses = rootCauses.layoutShifts[index];
if (shiftRootCauses) {
for (const cause of shiftRootCauses.unsizedMedia) {
nodeIds.push(cause.node.backendNodeId);
}
}

return nodeIds.map(nodeId => ({nodeId}));
});
}
Expand Down
78 changes: 2 additions & 76 deletions core/test/audits/layout-shifts-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('Performance: layout-shifts audit', () => {
snippet: '<h1 class="test-class">',
},
}],
RootCauses: {layoutShifts: {}},
RootCauses: {layoutShifts: new Map()},
};

const auditResult = await LayoutShiftsAudit.audit(artifacts, {computedCache: new Map()});
Expand All @@ -71,80 +71,6 @@ describe('Performance: layout-shifts audit', () => {
expect(auditResult.details.items[1].score).toEqual(0.1);
});

it('correctly surfaces layout shifts with root causes', async () => {
const trace = createTestTrace({});
trace.traceEvents.push({
args: {
data: {
had_recent_input: false,
is_main_frame: true,
weighted_score_delta: 0.3,
impacted_nodes: [{
node_id: 1,
old_rect: [0, 0, 1, 1],
new_rect: [0, 0, 2, 2],
}],
},
frame: 'ROOT_FRAME',
},
name: 'LayoutShift',
cat: 'loading',
}, {
args: {
data: {
had_recent_input: false,
is_main_frame: true,
weighted_score_delta: 0.1,
impacted_nodes: [{
node_id: 1,
old_rect: [0, 0, 1, 1],
new_rect: [0, 0, 2, 2],
}],
},
frame: 'ROOT_FRAME',
},
name: 'LayoutShift',
cat: 'loading',
});

const artifacts = {
traces: {defaultPass: trace},
TraceElements: [{
traceEventType: 'layout-shift',
nodeId: 1,
node: {
devtoolsNodePath: '1,HTML,3,BODY,5,DIV,0,HEADER',
selector: 'div.l-header > div.chorus-emc__content',
nodeLabel: 'My Test Label',
snippet: '<h1 class="test-class">',
},
}],
RootCauses: {layoutShifts: {
0: {
unsizedMedia: [],
fontChanges: [{request: {args: {data: {url: 'lol.com'}}}}],
iframes: [],
renderBlockingRequests: [],
},
}},
};

const auditResult = await LayoutShiftsAudit.audit(artifacts, {computedCache: new Map()});
expect(auditResult.score).toEqual(0);
expect(auditResult.displayValue).toBeDisplayString('2 layout shifts found');
expect(auditResult.metricSavings).toEqual({CLS: 0.4});
expect(auditResult.details.items).toHaveLength(2);
expect(auditResult.details.items[0]).toHaveProperty('node');
expect(auditResult.details.items[0].node).toHaveProperty('type', 'node');
expect(auditResult.details.items[0].score).toEqual(0.3);
expect(auditResult.details.items[0].subItems.items[0].cause)
.toBeDisplayString('Web font loaded');
expect(auditResult.details.items[1]).toHaveProperty('node');
expect(auditResult.details.items[1].node).toHaveProperty('type', 'node');
expect(auditResult.details.items[1].score).toEqual(0.1);
expect(auditResult.details.items[1].subItems).toBeUndefined();
});

it('correctly surfaces many layout shifts', async () => {
const trace = createTestTrace({});
const traceElements = [];
Expand Down Expand Up @@ -183,7 +109,7 @@ describe('Performance: layout-shifts audit', () => {
const artifacts = {
traces: {defaultPass: trace},
TraceElements: traceElements,
RootCauses: {layoutShifts: {}},
RootCauses: {layoutShifts: new Map()},
};

const auditResult = await LayoutShiftsAudit.audit(artifacts, {computedCache: new Map()});
Expand Down
Loading
Loading