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(responsiveness): use raw trace event #13970

Merged
merged 1 commit into from
May 6, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,21 @@ class ExperimentalInteractionToNextPaint extends Audit {

const trace = artifacts.traces[Audit.DEFAULT_PASS];
const metricData = {trace, settings};
const metricResult = await ComputedResponsivenes.request(metricData, context);
const responsivenessEvent = await ComputedResponsivenes.request(metricData, context);

// TODO: include the no-interaction state in the report instead of using n/a.
if (metricResult === null) {
if (responsivenessEvent === null) {
return {score: null, notApplicable: true};
}

const timing = responsivenessEvent.args.data.maxDuration;

return {
score: Audit.computeLogNormalScore({p10: context.options.p10, median: context.options.median},
metricResult.timing),
numericValue: metricResult.timing,
timing),
numericValue: timing,
numericUnit: 'millisecond',
displayValue: str_(i18n.UIStrings.ms, {timeInMs: metricResult.timing}),
displayValue: str_(i18n.UIStrings.ms, {timeInMs: timing}),
};
}
}
Expand Down
27 changes: 14 additions & 13 deletions lighthouse-core/computed/metrics/responsiveness.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,50 +11,51 @@
* user input in the provided trace).
*/

/** @typedef {LH.Trace.CompleteEvent & {name: 'Responsiveness.Renderer.UserInteraction', args: {frame: string, data: {interactionType: 'drag'|'keyboard'|'tapOrClick', maxDuration: number}}}} ResponsivenessEvent */
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: did you consider defining this in .d.ts (for ergonomic reasons)?

Copy link
Member Author

Choose a reason for hiding this comment

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

nit: did you consider defining this in .d.ts (for ergonomic reasons)?

maybe if it spreads, but so far it's really just needed in this one file and the two files that call it are fine with the implicit type from the function return.

There's also the multi-line jsdoc format, it's just unfortunate that vscode doesn't do better syntax highlighting for it (probably should file that as an issue at some point).


const makeComputedArtifact = require('../computed-artifact.js');
const ProcessedTrace = require('../processed-trace.js');

class Responsiveness {
/**
* @param {LH.Artifacts.ProcessedTrace} processedTrace
* @return {{timing: number}|null}
* @return {ResponsivenessEvent|null}
*/
static getHighPercentileResponsiveness(processedTrace) {
const durations = processedTrace.frameTreeEvents
const responsivenessEvents = processedTrace.frameTreeEvents
// https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/timing/responsiveness_metrics.cc;l=146-150;drc=a1a2302f30b0a58f7669a41c80acdf1fa11958dd
.filter(e => e.name === 'Responsiveness.Renderer.UserInteraction')
.map(evt => evt.args.data?.maxDuration)
.filter(/** @return {duration is number} */duration => duration !== undefined)
.sort((a, b) => b - a);
.filter(/** @return {e is ResponsivenessEvent} */ e => {
return e.name === 'Responsiveness.Renderer.UserInteraction';
}).sort((a, b) => b.args.data.maxDuration - a.args.data.maxDuration);

// If there were no interactions with the page, the metric is N/A.
if (durations.length === 0) {
if (responsivenessEvents.length === 0) {
return null;
}

// INP is the "nearest-rank"/inverted_cdf 98th percentile, except Chrome only
// keeps the 10 worst events around, so it can never be more than the 10th from
// last array element. To keep things simpler, sort desc and pick from front.
// See https://source.chromium.org/chromium/chromium/src/+/main:components/page_load_metrics/browser/responsiveness_metrics_normalization.cc;l=45-59;drc=cb0f9c8b559d9c7c3cb4ca94fc1118cc015d38ad
const index = Math.min(9, Math.floor(durations.length / 50));
const index = Math.min(9, Math.floor(responsivenessEvents.length / 50));

return {
timing: durations[index],
};
return responsivenessEvents[index];
}

/**
* @param {{trace: LH.Trace, settings: Immutable<LH.Config.Settings>}} data
* @param {LH.Artifacts.ComputedContext} context
* @return {Promise<LH.Artifacts.Metric|null>}
* @return {Promise<ResponsivenessEvent|null>}
*/
static async compute_(data, context) {
if (data.settings.throttlingMethod === 'simulate') {
throw new Error('Responsiveness currently unsupported by simulated throttling');
}

const processedTrace = await ProcessedTrace.request(data.trace, context);
return Responsiveness.getHighPercentileResponsiveness(processedTrace);
const event = Responsiveness.getHighPercentileResponsiveness(processedTrace);

return JSON.parse(JSON.stringify(event));
}
}

Expand Down
20 changes: 10 additions & 10 deletions lighthouse-core/test/computed/metrics/responsiveness-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ describe('Metric: Responsiveness', () => {
settings: {throttlingMethod: 'provided'},
};
const context = {computedCache: new Map()};
const result = await Responsiveness.request(metricInputData, context);
expect(result).toEqual(null);
const event = await Responsiveness.request(metricInputData, context);
expect(event).toEqual(null);
});

it('should select the 98th percentile interaction', async () => {
Expand All @@ -100,8 +100,8 @@ describe('Metric: Responsiveness', () => {
}

const context = {computedCache: new Map()};
const result = await Responsiveness.request(metricInputData, context);
assert.equal(result.timing, expectedTiming, `error at ${eventCount} events`);
const event = await Responsiveness.request(metricInputData, context);
assert.equal(event.args.data.maxDuration, expectedTiming, `error at ${eventCount} events`);
}
});

Expand Down Expand Up @@ -129,8 +129,8 @@ describe('Metric: Responsiveness', () => {
settings: {throttlingMethod: 'provided'},
};
const context = {computedCache: new Map()};
const result = await Responsiveness.request(metricInputData, context);
expect(result).toEqual({timing: 49});
const event = await Responsiveness.request(metricInputData, context);
expect(event.args.data).toMatchObject({maxDuration: 49});
});

it('should throw on attempting with a simulated timespan', async () => {
Expand All @@ -148,8 +148,8 @@ describe('Metric: Responsiveness', () => {
settings: {throttlingMethod: 'provided'},
};
const context = {computedCache: new Map()};
const result = await Responsiveness.request(metricInputData, context);
expect(result).toEqual({timing: 392});
const event = await Responsiveness.request(metricInputData, context);
expect(event.args.data).toMatchObject({maxDuration: 392});
});

it('evaluates from a real trace with no interaction events', async () => {
Expand All @@ -158,7 +158,7 @@ describe('Metric: Responsiveness', () => {
settings: {throttlingMethod: 'provided'},
};
const context = {computedCache: new Map()};
const result = await Responsiveness.request(metricInputData, context);
expect(result).toEqual(null);
const event = await Responsiveness.request(metricInputData, context);
expect(event).toEqual(null);
});
});
17 changes: 17 additions & 0 deletions types/artifacts.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,23 @@ export interface TraceEvent {
};
}

declare module Trace {
/**
* Base event of a `ph: 'X'` 'complete' event. Extend with `name` and `args` as
* needed.
*/
interface CompleteEvent {
Copy link
Member Author

@brendankenny brendankenny May 5, 2022

Choose a reason for hiding this comment

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

kind of harkens back to your trace types, @connorjclark :)

I kind of like this pattern of building up the event type compared to the Omit<>+build pattern used here:

/** @typedef {Omit<LH.TraceEvent, 'name'|'args'> & {name: 'FrameCommittedInBrowser', args: {data: {frame: string, url: string, parent?: string}}}} FrameCommittedEvent */
/** @typedef {Omit<LH.TraceEvent, 'name'|'args'> & {name: 'largestContentfulPaint::Invalidate'|'largestContentfulPaint::Candidate', args: {data?: {size?: number}, frame: string}}} LCPEvent */
/** @typedef {Omit<LH.TraceEvent, 'name'|'args'> & {name: 'largestContentfulPaint::Candidate', args: {data: {size: number}, frame: string}}} LCPCandidateEvent */

(where you still have some random optional properties from the LH.TraceEvent frankenstein)

ph: 'X';
cat: string;
pid: number;
tid: number;
dur: number;
ts: number;
tdur: number;
tts: number;
}
}

/**
* A record of DevTools Debugging Protocol events.
*/
Expand Down