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

fix: normalize all times to navStart, remove traceviewer model #2347

Merged
merged 21 commits into from
Jun 6, 2017
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
5 changes: 2 additions & 3 deletions lighthouse-core/audits/consistently-interactive.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,11 @@ class ConsistentlyInteractiveMetric extends Audit {
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const computedArtifacts = [
artifacts.requestNetworkRecords(devtoolsLog),
artifacts.requestTracingModel(trace),
artifacts.requestTraceOfTab(trace),
];

return Promise.all(computedArtifacts)
.then(([networkRecords, traceModel, traceOfTab]) => {
.then(([networkRecords, traceOfTab]) => {
if (!traceOfTab.timestamps.firstMeaningfulPaint) {
throw new Error('No firstMeaningfulPaint found in trace.');
}
Expand All @@ -221,7 +220,7 @@ class ConsistentlyInteractiveMetric extends Audit {
throw new Error('No domContentLoaded found in trace.');
}

const longTasks = TracingProcessor.getMainThreadTopLevelEvents(traceModel, trace)
const longTasks = TracingProcessor.getMainThreadTopLevelEvents(traceOfTab)
.filter(event => event.duration >= 50);
const quietPeriodInfo = this.findOverlappingQuietPeriods(longTasks, networkRecords,
traceOfTab);
Expand Down
15 changes: 5 additions & 10 deletions lighthouse-core/audits/estimated-input-latency.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ class EstimatedInputLatency extends Audit {
};
}

static calculate(tabTrace, model, trace) {
static calculate(tabTrace) {
const startTime = tabTrace.timings.firstMeaningfulPaint;
if (!startTime) {
throw new Error('No firstMeaningfulPaint event found in trace');
}

const latencyPercentiles = TracingProcessor.getRiskToResponsiveness(model, trace, startTime);
const latencyPercentiles = TracingProcessor.getRiskToResponsiveness(tabTrace, startTime);
const ninetieth = latencyPercentiles.find(result => result.percentile === 0.9);
const rawValue = parseFloat(ninetieth.time.toFixed(1));

Expand All @@ -57,7 +57,7 @@ class EstimatedInputLatency extends Audit {

return {
score: Math.round(score),
optimalValue: this.meta.optimalValue,
optimalValue: EstimatedInputLatency.meta.optimalValue,
rawValue,
displayValue: Util.formatMilliseconds(rawValue, 1),
extendedInfo: {
Expand All @@ -76,13 +76,8 @@ class EstimatedInputLatency extends Audit {
static audit(artifacts) {
const trace = artifacts.traces[this.DEFAULT_PASS];

const pending = [
artifacts.requestTraceOfTab(trace),
artifacts.requestTracingModel(trace)
];
return Promise.all(pending).then(([tabTrace, model]) => {
return EstimatedInputLatency.calculate(tabTrace, model, trace);
});
return artifacts.requestTraceOfTab(trace)
.then(EstimatedInputLatency.calculate);
}
}

Expand Down
9 changes: 4 additions & 5 deletions lighthouse-core/audits/time-to-interactive.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class TTIMetric extends Audit {

// Get our expected latency for the time window
const latencies = TracingProcessor.getRiskToResponsiveness(
data.model, data.trace, startTime, endTime, percentiles);
data.tabTrace, startTime, endTime, percentiles);
const estLatency = latencies[0].time;
foundLatencies.push({
estLatency: estLatency,
Expand Down Expand Up @@ -159,10 +159,9 @@ class TTIMetric extends Audit {
debugString = `Trace error: ${err.message}`;
return null;
}),
artifacts.requestTraceOfTab(trace),
artifacts.requestTracingModel(trace)
artifacts.requestTraceOfTab(trace)
];
return Promise.all(pending).then(([speedline, tabTrace, model]) => {
return Promise.all(pending).then(([speedline, tabTrace]) => {
// frame monotonic timestamps from speedline are in ms (ts / 1000), so we'll match
// https://github.com/pmdartus/speedline/blob/123f512632a/src/frame.js#L86
const fMPtsInMS = tabTrace.timestamps.firstMeaningfulPaint;
Expand All @@ -188,7 +187,7 @@ class TTIMetric extends Audit {
}

const times = {fmpTiming, visuallyReadyTiming, traceEndTiming};
const data = {tabTrace, model, trace};
const data = {tabTrace, trace};
Copy link
Member

Choose a reason for hiding this comment

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

looks like only data.tabTrace is being used above, not data.trace, so can maybe drop data in favor of just tabTrace? (also need to update the jsdocs still referring to model)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or we can just kill the audit entirely :)

Copy link
Member

Choose a reason for hiding this comment

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

haha, good point. I'm fine with leaving as is and deleting it next :)

const timeToInteractive = TTIMetric.findTTIAlpha(times, data);
const timeToInteractiveB = TTIMetric.findTTIAlphaFMPOnly(times, data);
const timeToInteractiveC = TTIMetric.findTTIAlphaFMPOnly5s(times, data);
Expand Down
19 changes: 18 additions & 1 deletion lighthouse-core/closure/typedefs/ComputedArtifacts.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,31 @@
* @externs
*/

/** @typedef
* {{
* navigationStart: number,
* firstPaint: number,
* firstContentfulPaint: number,
* firstMeaningfulPaint: number,
* traceEnd: number,
* onLoad: number,
* domContentLoaded: number,
* }}
*/
let TraceTimes;

/** @typedef
{{
timings: !TraceTimes,
timestamps: !TraceTimes,
processEvents: !Array<!TraceEvent>,
mainThreadEvents: !Array<!TraceEvent>,
startedInPageEvt: !TraceEvent,
navigationStartEvt: !TraceEvent,
firstPaintEvt: TraceEvent,
firstContentfulPaintEvt: TraceEvent,
firstMeaningfulPaintEvt: TraceEvent
firstMeaningfulPaintEvt: TraceEvent,
onLoadEvt: TraceEvent,
}} */
let TraceOfTabArtifact;

Expand Down
13 changes: 4 additions & 9 deletions lighthouse-core/gather/computed/first-interactive.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,10 @@ class FirstInteractive extends ComputedArtifact {
}

/**
* @param {!Trace} trace
* @param {!tr.Model} traceModel
* @param {!TraceOfTabArtifact} traceOfTab
* @return {{timeInMs: number, timestamp: number}}
*/
computeWithArtifacts(trace, traceModel, traceOfTab) {
computeWithArtifacts(traceOfTab) {
const navStart = traceOfTab.timestamps.navigationStart;
const FMP = traceOfTab.timings.firstMeaningfulPaint;
const DCL = traceOfTab.timings.domContentLoaded;
Expand All @@ -183,7 +181,7 @@ class FirstInteractive extends ComputedArtifact {
throw new Error(`No ${FMP ? 'domContentLoaded' : 'firstMeaningfulPaint'} event in trace`);
}

const longTasksAfterFMP = TracingProcessor.getMainThreadTopLevelEvents(traceModel, trace, FMP)
const longTasksAfterFMP = TracingProcessor.getMainThreadTopLevelEvents(traceOfTab, FMP)
.filter(evt => evt.duration >= LONG_TASK_THRESHOLD);
const firstInteractive = FirstInteractive.findQuietWindow(FMP, traceEnd, longTasksAfterFMP);

Expand All @@ -200,11 +198,8 @@ class FirstInteractive extends ComputedArtifact {
* @return {{timeInMs: number, timestamp: number}}
*/
compute_(trace, artifacts) {
return Promise.all([
artifacts.requestTracingModel(trace),
artifacts.requestTraceOfTab(trace),
]).then(([traceModel, traceOfTab]) => {
return this.computeWithArtifacts(trace, traceModel, traceOfTab);
return artifacts.requestTraceOfTab(trace).then(traceOfTab => {
return this.computeWithArtifacts(traceOfTab);
});
}
}
Expand Down
10 changes: 7 additions & 3 deletions lighthouse-core/gather/computed/trace-of-tab.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class TraceOfTab extends ComputedArtifact {

/**
* @param {{traceEvents: !Array}} trace
* @return {!{processEvents: !Array<TraceEvent>, startedInPageEvt: TraceEvent, navigationStartEvt: TraceEvent, firstContentfulPaintEvt: TraceEvent, firstMeaningfulPaintEvt: TraceEvent}}
* @return {!TraceOfTabArtifact}
Copy link
Member

Choose a reason for hiding this comment

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

TraceOfTabArtifact is really out of date. Mind updating it? :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

*/
compute_(trace) {
// Parse the trace for our key events and sort them by timestamp.
Expand Down Expand Up @@ -86,6 +86,9 @@ class TraceOfTab extends ComputedArtifact {
.filter(e => e.pid === startedInPageEvt.pid)
.sort((event0, event1) => event0.ts - event1.ts);

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

const traceEnd = trace.traceEvents.reduce((max, evt) => {
return max.ts > evt.ts ? max : evt;
});
Expand All @@ -95,7 +98,7 @@ class TraceOfTab extends ComputedArtifact {
firstPaint,
firstContentfulPaint,
firstMeaningfulPaint,
traceEnd,
traceEnd: {ts: traceEnd.ts + (traceEnd.dur || 0)},
onLoad,
domContentLoaded,
};
Expand All @@ -112,7 +115,8 @@ class TraceOfTab extends ComputedArtifact {
timings,
timestamps,
processEvents,
Copy link
Member

Choose a reason for hiding this comment

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

mainThreadEvents,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

startedInPageEvt: startedInPageEvt,
mainThreadEvents,
startedInPageEvt,
navigationStartEvt: navigationStart,
firstPaintEvt: firstPaint,
firstContentfulPaintEvt: firstContentfulPaint,
Expand Down
29 changes: 0 additions & 29 deletions lighthouse-core/gather/computed/tracing-model.js

This file was deleted.

Loading