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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 21 commits into from
Jun 6, 2017
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e791557
Remove traceviewer's tracingModel
paulirish May 5, 2017
8c2e789
Merge branch 'master' into rmtraceviewer
patrickhulce May 23, 2017
15d9424
revert user timings changes
patrickhulce May 23, 2017
2fb8850
fix remaining tests
patrickhulce May 23, 2017
25f157c
add trace with redirects
patrickhulce May 24, 2017
ea619df
use first navstart, not last
patrickhulce May 24, 2017
f32cccf
Revert "use first navstart, not last"
patrickhulce May 24, 2017
78036fc
Merge branch 'master' into rmtraceviewer
patrickhulce May 30, 2017
ef2eccd
reduce -> for..of (#2399)
paulirish May 30, 2017
184537b
feedback
patrickhulce May 30, 2017
1a0aacb
bug fix and test
patrickhulce May 31, 2017
9bfcdee
add second test
patrickhulce May 31, 2017
5f1a15f
Merge branch 'master' into rmtraceviewer
patrickhulce May 31, 2017
65de1eb
mainthread events
patrickhulce May 31, 2017
ba571ab
test updates
patrickhulce May 31, 2017
f62b2a4
add back colon traceg
patrickhulce May 31, 2017
d4c4605
fixed user timing test
patrickhulce May 31, 2017
df3c12e
use navstart as default
patrickhulce Jun 6, 2017
08797fc
Merge branch 'master' into rmtraceviewer
patrickhulce Jun 6, 2017
2bb290b
tasks -> events
patrickhulce Jun 6, 2017
71682ce
Merge branch 'master' into rmtraceviewer
patrickhulce Jun 6, 2017
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,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 @@ -223,7 +222,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
12 changes: 4 additions & 8 deletions lighthouse-core/audits/estimated-input-latency.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,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 Down Expand Up @@ -87,12 +87,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(tabTrace => {
return EstimatedInputLatency.calculate(tabTrace);
Copy link
Member

Choose a reason for hiding this comment

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

since it's static it would nice if this were able to be .then(EstimatedInputLatency.calculate), though that would require cleaning up our uses of this in here

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

});
}
}
Expand Down
9 changes: 4 additions & 5 deletions lighthouse-core/audits/time-to-interactive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,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 @@ -162,10 +162,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 @@ -191,7 +190,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
13 changes: 4 additions & 9 deletions lighthouse-core/gather/computed/first-interactive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,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 @@ -194,7 +192,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 @@ -211,11 +209,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
14 changes: 10 additions & 4 deletions lighthouse-core/gather/computed/trace-of-tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,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 @@ -98,7 +98,11 @@ class TraceOfTab extends ComputedArtifact {
.filter(e => e.pid === startedInPageEvt.pid)
.sort((event0, event1) => event0.ts - event1.ts);

const traceEnd = trace.traceEvents.reduce((max, evt) => {
const earliestTraceEvt = trace.traceEvents.reduce((min, evt) => {
Copy link
Member

Choose a reason for hiding this comment

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

let's add this guy:

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

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

return evt.ts === 0 || min.ts <= evt.ts ? min : evt;
}, {ts: Infinity});

const latestTraceEvt = trace.traceEvents.reduce((max, evt) => {
return max.ts > evt.ts ? max : evt;
});

Expand All @@ -107,7 +111,7 @@ class TraceOfTab extends ComputedArtifact {
firstPaint,
firstContentfulPaint,
firstMeaningfulPaint,
traceEnd,
traceEnd: latestTraceEvt,
Copy link
Member

@brendankenny brendankenny Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

traceEnd in metrics but latestTraceEvt in the top level. Should latestTraceEvt be traceEndEvt to match how the others pair up (timing name + Evt)?

(and don't feel strongly about it, but traceStartEvt may make more sense than earliestTraceEvt. e.g. new code in tracing-processor already does traceEnd = tabTrace.latestTraceEvt.ts)

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 had a very good explanation for why I named them this instead of traceEnd (that these were supposed to be just about the main thread, latestMainThreadEvt earliestMainThreadEvt and distinct from traceEnd) but the rest of the code betrays me. In fact, there are a number of things in here that I strongly feel like I accidentally dropped a stash...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah what do you think about using navStart and traceEnd timings instead of all this other nonsense entirely?

onLoad,
domContentLoaded,
};
Expand All @@ -124,7 +128,9 @@ 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,
earliestTraceEvt,
latestTraceEvt,
startedInPageEvt,
navigationStartEvt: navigationStart,
firstPaintEvt: firstPaint,
firstContentfulPaintEvt: firstContentfulPaint,
Expand Down
41 changes: 0 additions & 41 deletions lighthouse-core/gather/computed/tracing-model.js
View file
Open in desktop

This file was deleted.

Loading