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(tracehouse): split timeOrigin determination out of computeTraceOfTab #11253

Merged
merged 9 commits into from
Oct 22, 2020
9 changes: 9 additions & 0 deletions lighthouse-core/computed/trace-of-tab.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ class LHTraceProcessor extends TraceProcessor {
return new LHError(LHError.errors.NO_NAVSTART);
}

/**
* This isn't ever actually used, but might one day be.
Copy link
Member

Choose a reason for hiding this comment

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

lol if this is true (and there aren't concrete FR plans for it), why are we refactoring and adding 'firstResourceSendRequest'? Why not refactor but have 'lastNavigationStart' be the only option right now?

Copy link
Member

Choose a reason for hiding this comment

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

well, looking at the excellent comment on computeTimeOrigin, maybe the simpler thing is to change this to something more concrete like

Suggested change
* This isn't ever actually used, but might one day be.
* This isn't currently used, but will be when the time origin of trace processing is changed.
* @see {TraceProcessor.computeTimeOrigin}

(the @see doesn't actually work like for that yet, but will soon(!), though if the thing isn't imported, I don't know if you actually have to do something like @see {import('../../path/to/trace-processor.js').computeTimeOrigin} or what)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why are we [...] adding 'firstResourceSendRequest'?

firstResourceSendRequest won't be used for FR, but will be used for the #8984 line of work that will continue amidst FR.

Why not refactor but have 'lastNavigationStart' be the only option right now?

That's another option but a motivating example to validate the split seemed worthwhile to pair with the refactor given its simplicity and test uninterestingness if there's only one option :)

maybe the simpler thing is to change this to something more concrete like

sg 👍

* @see https://github.com/GoogleChrome/lighthouse/pull/11253#discussion_r507985527
* @return {Error}
*/
static createNoResourceSendRequestError() {
return new LHError(LHError.errors.NO_RESOURCE_SEND);
}

/**
* @return {Error}
*/
Expand Down
5 changes: 5 additions & 0 deletions lighthouse-core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,11 @@ const ERRORS = {
message: UIStrings.badTraceRecording,
lhrRuntimeError: true,
},
NO_RESOURCE_SEND: {
Copy link
Member

Choose a reason for hiding this comment

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

NO_FIRST_RESOURCE? NO_FIRST_RESOURCE_SEND?

Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be added to the proto, too, since it's a lhrRuntimeError? Again it's a bit premature, but we might easily forget to do it when the error is live.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does this need to be added to the proto, too, since it's a lhrRuntimeError? Again it's a bit premature, but we might easily forget to do it when the error is live.

Yeah sg might as well do it now too.

re: NO_FIRST_RESOURCE_SEND, any particular reason you'd like to see the addition of FIRST? It seemed like unnecessarily specific information to add to me that might lead to narrower usage later. The only way there can be no first resource request is if there were none at all :)

Copy link
Member

Choose a reason for hiding this comment

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

re: NO_FIRST_RESOURCE_SEND, any particular reason you'd like to see the addition of FIRST? It seemed like unnecessarily specific information to add to me that might lead to narrower usage later. The only way there can be no first resource request is if there were none at all :)

it's just short and I was wondering if there's more context we can give it. That's a very good point, though :)

"Resource send" is kind of confusing as representing the request for a resource, though (ResourceSendRequest does make sense with the other trace event names next to it). Maybe NO_RESOURCE_REQUEST? A slight step away from the trace event name but slightly more understandable on its own.

I'll stop bikeshedding after this :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NO_RESOURCE_REQUEST

sold :)

code: 'NO_RESOURCE_SEND',
message: UIStrings.badTraceRecording,
lhrRuntimeError: true,
},
NO_NAVSTART: {
code: 'NO_NAVSTART',
message: UIStrings.badTraceRecording,
Expand Down
188 changes: 152 additions & 36 deletions lighthouse-core/lib/tracehouse/trace-processor.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

/** @typedef {Omit<LH.Artifacts.TraceTimes, 'firstContentfulPaint'> & {firstContentfulPaint?: number}} TraceTimesWithoutFCP */
/** @typedef {Omit<LH.Artifacts.TraceOfTab, 'firstContentfulPaintEvt'|'timings'|'timestamps'> & {timings: TraceTimesWithoutFCP, timestamps: TraceTimesWithoutFCP, firstContentfulPaintEvt?: LH.Artifacts.TraceOfTab['firstContentfulPaintEvt']}} TraceOfTabWithoutFCP */
/** @typedef {Omit<TraceOfTabWithoutFCP, 'frames'|'processEvents'|'mainThreadEvents'|'mainFrameIds'>} FrameTimings */
/** @typedef {'lastNavigationStart'|'firstResourceSendRequest'} TimeOriginDeterminationMethod */
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved

const log = require('lighthouse-logger');

Expand All @@ -43,6 +45,13 @@ class TraceProcessor {
return new Error('No navigationStart event found');
}

/**
* @return {Error}
*/
static createNoResourceSendRequestError() {
return new Error('No ResourceSendRequest event found');
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* @return {Error}
*/
Expand Down Expand Up @@ -471,11 +480,14 @@ class TraceProcessor {

/**
* Finds key trace events, identifies main process/thread, and returns timings of trace events
* in milliseconds since navigation start in addition to the standard microsecond monotonic timestamps.
* in milliseconds since the time origin in addition to the standard microsecond monotonic timestamps.
* @param {LH.Trace} trace
* @param {{timeOriginDeterminationMethod?: TimeOriginDeterminationMethod}} [options]
* @return {TraceOfTabWithoutFCP}
*/
static computeTraceOfTab(trace) {
static computeTraceOfTab(trace, options) {
const {timeOriginDeterminationMethod = 'lastNavigationStart'} = options || {};

// Parse the trace for our key events and sort them by timestamp. Note: sort
// *must* be stable to keep events correctly nested.
const keyEvents = this.filteredTraceSort(trace.traceEvents, e => {
Expand All @@ -491,10 +503,140 @@ class TraceProcessor {
// Filter to just events matching the frame ID, just to make sure.
const frameEvents = keyEvents.filter(e => e.args.frame === mainFrameIds.frameId);

// Our navStart will be the last frame navigation in the trace
const navigationStart = frameEvents.filter(this._isNavigationStartOfInterest).pop();
if (!navigationStart) throw this.createNoNavstartError();
const timeOriginEvt = navigationStart;
// Compute our time origin to use for all relative timings.
const timeOriginEvt = this.computeTimeOrigin(
{keyEvents, frameEvents, mainFrameIds},
timeOriginDeterminationMethod
);

// Compute the key frame timings for the main frame.
const frameTimings = this.computeKeyTimingsForFrame(frameEvents, {timeOriginEvt});

// Subset all trace events to just our tab's process (incl threads other than main)
// stable-sort events to keep them correctly nested.
const processEvents = TraceProcessor
.filteredTraceSort(trace.traceEvents, e => e.pid === mainFrameIds.pid);

const mainThreadEvents = processEvents
.filter(e => e.tid === mainFrameIds.tid);

const frames = keyEvents
.filter(evt => evt.name === 'FrameCommittedInBrowser')
.map(evt => evt.args.data)
.filter(/** @return {data is {frame: string, url: string}} */ data => {
return Boolean(data && data.frame && data.url);
});

// Ensure our traceEnd reflects all page activity.
const traceEnd = this.computeTraceEnd(trace.traceEvents, timeOriginEvt);
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to just pass trace.traceEvents into computeKeyTimingsForFrame and do traceEnd for real in there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As in pass both frameEvents and all trace events?

Primary reason I did not do that is it violates the veil of abstraction that computeKeyTimingsForFrame well, computes the key timings for a frame :)

If the double computation is problematic I'd prefer to modify the return type of computeKeyTimingsForFrame to omit traceEnd entirely as a proper fix. The duplication of all the types to avoid a double computation didn't seem to be worth the tradeoff in confusion initially since the trace end computation is cheap (~0.5ms/MB of trace). Prefer this approach?

Copy link
Member

Choose a reason for hiding this comment

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

If the double computation is problematic

It's not fatally problematic, no more so than breaking the veil :) And definitely not worried about the computation cost. I was just curious about your thinking on which was less confusing, passing in both sets of events or fake computing it and then doing it for real. Breaking the type down might be worse but it's possible it would be ok...e.g. with an inferred return type on computeKeyTimingsForFrame, maybe?

I'm also trying to be more literal in my review comments. I was 95% sure you had already considered this question so I was 95% really just asking, and 5% suggesting considering it and then just asking as a consideration follow up :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Breaking the type down might be worse but it's possible it would be ok...e.g. with an inferred return type on computeKeyTimingsForFrame, maybe?

I like it SG!

Copy link
Member

Choose a reason for hiding this comment

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

If the double computation is problematic I'd prefer to modify the return type of computeKeyTimingsForFrame to omit traceEnd entirely as a proper fix.

Just noticed FrameTimings is a new typedef, and yeah, this would be the proper fix and it would be great if it's a simple modification to the Omit, but I don't know if there's an easy way to do that one level deep, so don't worry about it if there's not.


// This could be much more concise with object spread, but the consensus is that explicitness is
// preferred over brevity here.
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
return {
frames,
mainThreadEvents,
processEvents,
mainFrameIds,
timings: {
timeOrigin: frameTimings.timings.timeOrigin,
firstPaint: frameTimings.timings.firstPaint,
firstContentfulPaint: frameTimings.timings.firstContentfulPaint,
firstMeaningfulPaint: frameTimings.timings.firstMeaningfulPaint,
largestContentfulPaint: frameTimings.timings.largestContentfulPaint,
traceEnd: traceEnd.timing,
load: frameTimings.timings.load,
domContentLoaded: frameTimings.timings.domContentLoaded,
},
timestamps: {
timeOrigin: frameTimings.timestamps.timeOrigin,
firstPaint: frameTimings.timestamps.firstPaint,
firstContentfulPaint: frameTimings.timestamps.firstContentfulPaint,
firstMeaningfulPaint: frameTimings.timestamps.firstMeaningfulPaint,
largestContentfulPaint: frameTimings.timestamps.largestContentfulPaint,
traceEnd: traceEnd.timestamp,
load: frameTimings.timestamps.load,
domContentLoaded: frameTimings.timestamps.domContentLoaded,
},
timeOriginEvt: frameTimings.timeOriginEvt,
firstPaintEvt: frameTimings.firstPaintEvt,
firstContentfulPaintEvt: frameTimings.firstContentfulPaintEvt,
firstMeaningfulPaintEvt: frameTimings.firstMeaningfulPaintEvt,
largestContentfulPaintEvt: frameTimings.largestContentfulPaintEvt,
loadEvt: frameTimings.loadEvt,
domContentLoadedEvt: frameTimings.domContentLoadedEvt,
fmpFellBack: frameTimings.fmpFellBack,
lcpInvalidated: frameTimings.lcpInvalidated,
};
}

/**
* Computes the last observable timestamp in a set of trace events.
*
* @param {Array<LH.TraceEvent>} events
* @param {LH.TraceEvent} timeOriginEvt
* @return {{timing: number, timestamp: number}}
*/
static computeTraceEnd(events, timeOriginEvt) {
let maxTs = -Infinity;
for (const event of events) {
maxTs = Math.max(event.ts + (event.dur || 0), maxTs);
}

return {timestamp: maxTs, timing: (maxTs - timeOriginEvt.ts) / 1000};
}

/**
* Computes the time origin using the specified method.
*
* - firstResourceSendRequest
* Uses the time that the very first network request is sent in the main frame.
* Eventually should be used in place of lastNavigationStart as the default for navigations.
* This method includes the cost of all redirects when evaluating a navigation (which matches lantern behavior).
* The only difference between firstResourceSendRequest and the first `navigationStart` is
* the unload time of `about:blank` (which is a Lighthouse implementation detail and shouldn't be included).
*
* - lastNavigationStart
* Uses the time of the last `navigationStart` event in the main frame.
* The historical time origin of Lighthouse from 2016-Present.
* This method excludes the cost of client-side redirects when evaluating a navigation.
* Can also be skewed by several hundred milliseconds or even seconds when the browser takes a long
* time to unload `about:blank`.
*
* @param {{keyEvents: Array<LH.TraceEvent>, frameEvents: Array<LH.TraceEvent>, mainFrameIds: {frameId: string}}} traceEventSubsets
* @param {TimeOriginDeterminationMethod} method
* @return {LH.TraceEvent}
*/
static computeTimeOrigin(traceEventSubsets, method) {
switch (method) {
case 'firstResourceSendRequest': {
// Our time origin will be the timestamp of the first request that's sent in the frame.
const fetchStart = traceEventSubsets.keyEvents.find(event => {
if (event.name !== 'ResourceSendRequest') return false;
const data = event.args.data || {};
return data.frame === traceEventSubsets.mainFrameIds.frameId;
});
if (!fetchStart) throw this.createNoResourceSendRequestError();
return fetchStart;
}
case 'lastNavigationStart': {
// Our time origin will be the last frame navigation in the trace
const frameEvents = traceEventSubsets.frameEvents;
const navigationStart = frameEvents.filter(this._isNavigationStartOfInterest).pop();
if (!navigationStart) throw this.createNoNavstartError();
return navigationStart;
}
}
}

/**
* Computes timings of trace events of key trace events in milliseconds since the time origin
* in addition to the standard microsecond monotonic timestamps.
* @param {Array<LH.TraceEvent>} frameEvents
* @param {{timeOriginEvt: LH.TraceEvent}} options
* @return {FrameTimings}
*/
static computeKeyTimingsForFrame(frameEvents, options) {
const {timeOriginEvt} = options;

// Find our first paint of this frame
const firstPaint = frameEvents.find(e => e.name === 'firstPaint' && e.ts > timeOriginEvt.ts);
Expand Down Expand Up @@ -553,20 +695,7 @@ class TraceProcessor {
e => e.name === 'domContentLoadedEventEnd' && e.ts > timeOriginEvt.ts
);

// subset all trace events to just our tab's process (incl threads other than main)
// stable-sort events to keep them correctly nested.
const processEvents = TraceProcessor
.filteredTraceSort(trace.traceEvents, e => e.pid === mainFrameIds.pid);

const mainThreadEvents = processEvents
.filter(e => e.tid === mainFrameIds.tid);

// traceEnd must exist since at least timeOrigin event was verified as existing.
const traceEnd = trace.traceEvents.reduce((max, evt) => {
return max.ts > evt.ts ? max : evt;
});
const fakeEndOfTraceEvt = {ts: traceEnd.ts + (traceEnd.dur || 0)};

const traceEndEvt = this.computeTraceEnd(frameEvents, timeOriginEvt);
/** @param {{ts: number}=} event */
const getTimestamp = (event) => event && event.ts;
/** @type {TraceTimesWithoutFCP} */
Expand All @@ -576,41 +705,28 @@ class TraceProcessor {
firstContentfulPaint: getTimestamp(firstContentfulPaint),
firstMeaningfulPaint: getTimestamp(firstMeaningfulPaint),
largestContentfulPaint: getTimestamp(largestContentfulPaint),
traceEnd: fakeEndOfTraceEvt.ts,
traceEnd: traceEndEvt.timestamp,
load: getTimestamp(load),
domContentLoaded: getTimestamp(domContentLoaded),
};

/** @param {number} ts */
const getTiming = (ts) => (ts - timeOriginEvt.ts) / 1000;
/** @param {number=} ts */
const maybeGetTiming = (ts) => ts === undefined ? undefined : getTiming(ts);
const maybeGetTiming = (ts) => ts === undefined ? undefined : (ts - timeOriginEvt.ts) / 1000;
/** @type {TraceTimesWithoutFCP} */
const timings = {
timeOrigin: 0,
firstPaint: maybeGetTiming(timestamps.firstPaint),
firstContentfulPaint: maybeGetTiming(timestamps.firstContentfulPaint),
firstMeaningfulPaint: maybeGetTiming(timestamps.firstMeaningfulPaint),
largestContentfulPaint: maybeGetTiming(timestamps.largestContentfulPaint),
traceEnd: getTiming(timestamps.traceEnd),
traceEnd: traceEndEvt.timing,
load: maybeGetTiming(timestamps.load),
domContentLoaded: maybeGetTiming(timestamps.domContentLoaded),
};

const frames = keyEvents
.filter(evt => evt.name === 'FrameCommittedInBrowser')
.map(evt => evt.args.data)
.filter(/** @return {data is {frame: string, url: string}} */ data => {
return Boolean(data && data.frame && data.url);
});

return {
timings,
timestamps,
processEvents,
mainThreadEvents,
mainFrameIds,
frames,
timeOriginEvt: timeOriginEvt,
firstPaintEvt: firstPaint,
firstContentfulPaintEvt: firstContentfulPaint,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ Object {
"observedTimeOrigin": 0,
"observedTimeOriginTs": 713037023064,
"observedTraceEnd": 7416,
"observedTraceEndTs": 713044438781,
"observedTraceEndTs": 713044439102,
"speedIndex": 3681,
"speedIndexTs": undefined,
"totalBlockingTime": 1167,
Expand Down
36 changes: 35 additions & 1 deletion lighthouse-core/test/lib/tracehouse/trace-processor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,13 @@ describe('TraceProcessor', () => {
});
});

describe('computeTraceEnd', () => {
it('computes the last timestamp within the bounds of the trace', () => {
const events = [{ts: 1000}, {ts: 999, dur: 1001}];
expect(TraceProcessor.computeTraceEnd(events, {ts: 0})).toEqual({timestamp: 2000, timing: 2});
});
});

describe('computeTraceOfTab', () => {
it('gathers the events from the tab\'s process', () => {
const trace = TraceProcessor.computeTraceOfTab(lateTracingStartedTrace);
Expand Down Expand Up @@ -283,7 +290,34 @@ describe('TraceProcessor', () => {
assert.equal(Math.round(trace.timestamps.firstPaint), 29343620997);
assert.equal(Math.round(trace.timestamps.firstContentfulPaint), 29343621005);
assert.equal(Math.round(trace.timestamps.firstMeaningfulPaint), 29344070867);
assert.equal(Math.round(trace.timestamps.traceEnd), 29344190223);
assert.equal(Math.round(trace.timestamps.traceEnd), 29344190232);
});

describe('timeOriginDeterminationMethod', () => {
it('supports lastNavigationStart', () => {
const trace = TraceProcessor.computeTraceOfTab(lcpTrace);
expect(trace.timings).toMatchObject({
largestContentfulPaint: 1121.711,
load: 2159.007,
traceEnd: 7416.038,
});

expect(trace.timestamps.timeOrigin).toEqual(713037023064);
});

it('supports firstResourceSendRequest', () => {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
const trace = TraceProcessor.computeTraceOfTab(lcpTrace, {
timeOriginDeterminationMethod: 'firstResourceSendRequest',
});

expect(trace.timings).toMatchObject({
largestContentfulPaint: 812.683,
load: 1849.979,
traceEnd: 7107.01,
});

expect(trace.timestamps.timeOrigin).toEqual(713037332092);
});
});

describe('finds correct FMP', () => {
Expand Down