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

feat: add consistently interactive audit #2023

Merged
merged 15 commits into from
May 10, 2017
Merged
Show file tree
Hide file tree
Changes from 5 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
247 changes: 247 additions & 0 deletions lighthouse-core/audits/consistently-interactive.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,247 @@
/**
* @license
* Copyright 2017 Google Inc. All rights reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

const Audit = require('./audit');
const TracingProcessor = require('../lib/traces/tracing-processor');
const Formatter = require('../report/formatter');

// Parameters (in ms) for log-normal CDF scoring. To see the curve:
// https://www.desmos.com/calculator/uti67afozh
const SCORING_POINT_OF_DIMINISHING_RETURNS = 1700;
const SCORING_MEDIAN = 10000;
const SCORING_TARGET = 5000;
Copy link
Member

Choose a reason for hiding this comment

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

remove

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


const REQUIRED_QUIET_WINDOW = 5000;
const ALLOWED_CONCURRENT_REQUESTS = 2;

const distribution = TracingProcessor.getLogNormalDistribution(
SCORING_MEDIAN,
SCORING_POINT_OF_DIMINISHING_RETURNS
);

/**
* @fileoverview This audit identifies the time the page is "consistently interactive".
* Looks for the first period of at least 5 seconds after FMP where both CPU and network were quiet,
* and returns the timestamp of the beginning of the CPU quiet period.
* @see https://docs.google.com/document/d/1GGiI9-7KeY3TPqS3YT271upUVimo-XiL5mwWorDUD4c/edit#
*/
class ConsistentlyInteractiveMetric extends Audit {
/**
* @return {!AuditMeta}
*/
static get meta() {
return {
category: 'Performance',
name: 'consistently-interactive',
description: 'Consistently Interactive (beta)',
helpText: 'The point at which most network resources have finished loading and the ' +
'CPU is idle for a prolonged period.',
optimalValue: SCORING_TARGET.toLocaleString() + 'ms',
Copy link
Member

Choose a reason for hiding this comment

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

remove

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

scoringMode: Audit.SCORING_MODES.NUMERIC,
requiredArtifacts: ['traces', 'networkRecords']
};
}

/**
* @param {!Array} networkRecords
Copy link
Member

Choose a reason for hiding this comment

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

{!Array<WebInspector.NetworkRequest>}

Copy link
Member

Choose a reason for hiding this comment

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

add a jsdoc method description?

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

* @param {{timestamps: {traceEnd: number}}} traceOfTab
* @return {!Array<{start: number, end: number}>}
*/
static _findNetworkQuietPeriods(networkRecords, traceOfTab) {
const traceEnd = traceOfTab.timestamps.traceEnd;

// First collect the timestamps of when requests start and end
const timeBoundaries = [];
networkRecords.forEach(record => {
const scheme = record.parsedURL && record.parsedURL.scheme;
if (scheme === 'data' || scheme === 'ws') {
Copy link
Member

Choose a reason for hiding this comment

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

might be overkill, but should we convert this to a ignoredNetworkSchemes array and pull it into a constant?

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, done

return;
}

// convert the network record timestamp to ms to line-up with traceOfTab
timeBoundaries.push({time: record.startTime * 1000, isStart: true});
timeBoundaries.push({time: record.endTime * 1000, isStart: false});
});

timeBoundaries.sort((a, b) => a.time - b.time);

let numInflightRequests = 0;
let quietPeriodStart = 0;
const quietPeriods = [];
timeBoundaries.forEach(boundary => {
Copy link
Member

Choose a reason for hiding this comment

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

hey a forEach! 😁 😍

if (boundary.isStart) {
// we've just started a new request. are we exiting a quiet period?
if (numInflightRequests === ALLOWED_CONCURRENT_REQUESTS) {
quietPeriods.push({start: quietPeriodStart, end: boundary.time});
quietPeriodStart = Infinity;
}
numInflightRequests++;
} else {
numInflightRequests--;
// we've just completed a request. are we entering a quiet period?
if (numInflightRequests <= ALLOWED_CONCURRENT_REQUESTS) {
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 use === and drop the Math.min and just use quietPeriodStart = boundary.time;?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

probably not? I can't think of one, so done :)

quietPeriodStart = Math.min(boundary.time, quietPeriodStart);
}
}
});

// Check if the trace ended in a quiet period
if (quietPeriodStart !== Infinity) {
Copy link
Member

Choose a reason for hiding this comment

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

would it be clearer to check numInflightRequests <= ALLOWED_CONCURRENT_REQUESTS as representing being in a quiet period?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep sgtm, this func was mostly just the network throughput computation modified to line up with deep's implementation, but it could be clearer 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

come to think of it, in this version the condition will never be true anyway...I'll have to look into how end times are handled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yikes, we've actually got a problem with network recorder :/ it only saves records on finish

Copy link
Member

Choose a reason for hiding this comment

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

hmm, good catch. That was for expedience at the time, but that was a very long time ago and we could probably do with some more nuanced data.

A quick fix would be to just look directly at the devtoolsLog for unclosed requests, but what are you thinking?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just fixed, adding the request on start instead and checking .finished

Copy link
Member

Choose a reason for hiding this comment

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

just fixed, adding the request on start instead and checking .finished

this is the right solution, but nothing else that depends on network records is checking for this as they all could assume that all requests were finished when they were written. They're often just taking all of them and filtering on whatever they need to look for. We need to do a survey of all the uses, fix where it makes sense, add unit tests like you added in a5d62b8 for CI, etc

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 think I've addressed all the usages here now, some places were already checking for it

quietPeriods.push({start: quietPeriodStart, end: traceEnd});
}

return quietPeriods;
}

/**
* @param {!Array<{start: number, end: number}>} longTasks
Copy link
Member

Choose a reason for hiding this comment

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

jsdoc description

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

* @param {{timestamps: {navigationStart: number, traceEnd: number}}} traceOfTab
* @return {!Array<{start: number, end: number}>}
*/
static _findCPUQuietPeriods(longTasks, traceOfTab) {
const navStartTsInMs = traceOfTab.timestamps.navigationStart;
const traceEndTsInMs = traceOfTab.timestamps.traceEnd;
if (longTasks.length === 0) {
return [{start: 0, end: traceEndTsInMs}];
}

const quietPeriods = [];
longTasks.forEach((task, index) => {
if (index === 0) {
quietPeriods.push({
start: 0,
end: task.start + navStartTsInMs,
});
}

if (index === longTasks.length - 1) {
quietPeriods.push({
start: task.end + navStartTsInMs,
end: traceEndTsInMs,
});
} else {
quietPeriods.push({
start: task.end + navStartTsInMs,
end: longTasks[index + 1].start + navStartTsInMs,
});
}
});

return quietPeriods;
}

/**
* @param {!Array<{start: number, end: number}>} longTasks
Copy link
Member

Choose a reason for hiding this comment

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

jsdoc description :)

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

* @param {{timestamps: {navigationStart: number, firstMeaningfulPaint: number,
Copy link
Member

Choose a reason for hiding this comment

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

networkRecords needs an @param

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

* traceEnd: number}}} traceOfTab
* @return {!Object}
Copy link
Member

Choose a reason for hiding this comment

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

would be great to have a type 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

*/
static findOverlappingQuietPeriods(longTasks, networkRecords, traceOfTab) {
const FMPTsInMs = traceOfTab.timestamps.firstMeaningfulPaint;

const isLongEnoughQuietPeriod = period =>
period.end > FMPTsInMs + REQUIRED_QUIET_WINDOW &&
period.end - period.start >= REQUIRED_QUIET_WINDOW;
const networkQuietPeriods = this._findNetworkQuietPeriods(networkRecords, traceOfTab)
.filter(isLongEnoughQuietPeriod);
const cpuQuietPeriods = this._findCPUQuietPeriods(longTasks, traceOfTab)
.filter(isLongEnoughQuietPeriod);

const cpuQueue = cpuQuietPeriods.slice();
const networkQueue = networkQuietPeriods.slice();
Copy link
Member

Choose a reason for hiding this comment

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

why the slices?

Copy link
Member

Choose a reason for hiding this comment

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

why the slices?

jk, they get passed back from findOverlappingQuietPeriods. Any reason to do that? I only see them used in one test (and if only used to expedite testing of _findCPUQuietPeriods and _findNetworkQuietPeriods, I personally think it's fine to call those directly as their static (albeit private))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

they're also returned in extended info


// We will check for a CPU quiet period contained within a Network quiet period or vice-versa
let cpuCandidate = cpuQueue.shift();
let networkCandidate = networkQueue.shift();
while (cpuCandidate && networkCandidate) {
if (cpuCandidate.start >= networkCandidate.start) {
// CPU starts later than network, it must be contained by network or we check the next network
Copy link
Member

Choose a reason for hiding this comment

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

maybe s/it/window? Was confused for a bit why cpuCandidate needed to be within networkCandidate

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

if (networkCandidate.end >= cpuCandidate.start + REQUIRED_QUIET_WINDOW) {
return {
cpuQuietPeriod: cpuCandidate,
networkQuietPeriod: networkCandidate,
cpuQuietPeriods,
networkQuietPeriods,
};
} else {
networkCandidate = networkQueue.shift();
}
} else {
// Network starts later than CPU, it must be contained by CPU or we check the next CPU
if (cpuCandidate.end >= networkCandidate.start + REQUIRED_QUIET_WINDOW) {
return {
cpuQuietPeriod: cpuCandidate,
networkQuietPeriod: networkCandidate,
cpuQuietPeriods,
networkQuietPeriods,
};
} else {
cpuCandidate = cpuQueue.shift();
}
}
}

const culprit = cpuCandidate ? 'Network' : 'CPU';
throw new Error(`${culprit} did not quiet for at least 5s before the end of the trace.`);
}

/**
* @param {!Artifacts} artifacts
* @return {!Promise<!AuditResult>}
*/
static audit(artifacts) {
const trace = artifacts.traces[Audit.DEFAULT_PASS];
const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS];
const computedTraceArtifacts = [
artifacts.requestTracingModel(trace),
artifacts.requestTraceOfTab(trace),
];

return Promise.all(computedTraceArtifacts)
.then(([traceModel, traceOfTab]) => {
if (!traceOfTab.timestamps.firstMeaningfulPaint) {
Copy link
Member

Choose a reason for hiding this comment

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

should probably throw on lack of DCL too (as in TTFI)

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(though in the spec I'm not actually sure this is true, it just seemed like the proper thing to do. I should raise it)

Copy link
Member

Choose a reason for hiding this comment

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

(though in the spec I'm not actually sure this is true, it just seemed like the proper thing to do. I should raise it)

wouldn't no FMP be a bug in Chrome/Lighthouse? Unless you mean something else

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 was talking about the fact that we're doing the max of DCL and the value. I wasn't sure that's in the spec or if I added that to be sure it doesn't happen before TTFI

throw new Error('No firstMeaningfulPaint found in trace.');
}

const longTasks = TracingProcessor.getMainThreadTopLevelEvents(traceModel, trace)
Copy link
Member

Choose a reason for hiding this comment

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

pass in FMP here to avoid computing stuff before then? (also would allow removal of FMP stuff in isLongEnoughQuietPeriod above)

Copy link
Member

Choose a reason for hiding this comment

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

though I guess _findNetworkQuietPeriods would then also need to find out and clip by FMP

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wouldn't really be able to remove anything since we still need to check that the period is 5s after FMP (even if long tasks are ignored before FMP) so it's just the saved long tasks we're iterating through, and IMO it makes the logic harder to parse that FMP has already been dealt with in CPU but we still have to deal with it in network, fine leaving?

Copy link
Member

Choose a reason for hiding this comment

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

fine leaving?

yeah, I guess it's just awkward one way or the other. SG

.filter(event => event.end - event.start >= 50);
Copy link
Member

Choose a reason for hiding this comment

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

event.duration

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

const quietPeriodInfo = this.findOverlappingQuietPeriods(longTasks, networkRecords,
traceOfTab);
const cpuQuietPeriod = quietPeriodInfo.cpuQuietPeriod;

const timestamp = Math.max(
cpuQuietPeriod.start,
traceOfTab.timestamps.firstMeaningfulPaint,
traceOfTab.timestamps.domContentLoaded
);
const timeInMs = timestamp - traceOfTab.timestamps.navigationStart;
const extendedInfo = Object.assign(quietPeriodInfo, {timestamp, timeInMs});
Copy link
Member

Choose a reason for hiding this comment

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

shape of quietPeriodInfo is difficult to track down. Can it be made more evident 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.

added typedef to findOverlappingQuietPeriods 👍

Copy link
Member

Choose a reason for hiding this comment

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

that is...not a terribly useful type definition :P If I wanted to use that extendedInfo I would have to check the param signatures of _findCPUQuietPeriods and _findNetworkQuietPeriods and read findOverlappingQuietPeriods to make sure there's only one code escape path. We don't have a great place for documenting artifact shapes, but it would be nice to always do it, even if in an awkward place

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added a TimePeriod typedef

/**
 * @typedef {{
 *     start: number,
 *     end: number,
 * }}
 */
let TimePeriod; // eslint-disable-line no-unused-vars


let score = 100 * distribution.computeComplementaryPercentile(timeInMs);
// Clamp the score to 0 <= x <= 100.
score = Math.min(100, score);
score = Math.max(0, score);
score = Math.round(score);

const displayValue = Math.round(timeInMs / 10) * 10;
return {
score,
rawValue: timeInMs,
displayValue: `${displayValue.toLocaleString()}ms`,
optimalValue: this.meta.optimalValue,
extendedInfo: {
value: extendedInfo,
formatter: Formatter.SUPPORTED_FORMATS.NULL,
}
};
});
}
}

module.exports = ConsistentlyInteractiveMetric;
2 changes: 2 additions & 0 deletions lighthouse-core/config/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ module.exports = {
"load-fast-enough-for-pwa",
"speed-index-metric",
"estimated-input-latency",
"consistently-interactive",
"first-interactive",
"time-to-interactive",
"user-timings",
Expand Down Expand Up @@ -617,6 +618,7 @@ module.exports = {
{"id": "speed-index-metric", "weight": 1},
{"id": "estimated-input-latency", "weight": 1},
{"id": "time-to-interactive", "weight": 5},
{"id": "consistently-interactive", "weight": 5},
{"id": "first-interactive", "weight": 5},
{"id": "link-blocking-first-paint", "weight": 0},
{"id": "script-blocking-first-paint", "weight": 0},
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const URL = require('../lib/url-shim');
const log = require('../lib/log.js');
const DevtoolsLog = require('./devtools-log');

const PAUSE_AFTER_LOAD = 500;
const PAUSE_AFTER_LOAD = 5000;

const _uniq = arr => Array.from(new Set(arr));

Expand Down
12 changes: 12 additions & 0 deletions lighthouse-core/lib/traces/pwmetrics-events.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,18 @@ class Metrics {
return safeGet(ttfiExt, 'value.timeInMs');
}
},
{
name: 'Time to Consistently Interactive (vBeta)',
id: 'ttci',
getTs: auditResults => {
const ttiExt = auditResults['consistently-interactive'].extendedInfo;
return safeGet(ttiExt, 'value.timestamp');
},
getTiming: auditResults => {
const ttiExt = auditResults['consistently-interactive'].extendedInfo;
return safeGet(ttiExt, 'value.timeInMs');
}
},
{
name: 'End of Trace',
id: 'eot',
Expand Down
Loading