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: refactor NetworkRequest time properties #14311

Closed
wants to merge 35 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
0b2e77b
misc: document where network timings come from
connorjclark Jul 20, 2022
7b912a5
update
connorjclark Jul 20, 2022
057d1da
update
connorjclark Jul 20, 2022
0a33be4
update
connorjclark Jul 20, 2022
02aae47
update
connorjclark Jul 20, 2022
6f7bee2
pr
connorjclark Aug 19, 2022
9bf0683
update
connorjclark Aug 19, 2022
0227595
Merge remote-tracking branch 'origin/master' into request-notes-cdp
connorjclark Aug 19, 2022
3077f90
typo
connorjclark Aug 19, 2022
d83d201
this ck is fn raw!!
connorjclark Aug 19, 2022
ab2885a
wip
connorjclark Aug 20, 2022
39016e1
wip
connorjclark Aug 22, 2022
237dec1
Merge remote-tracking branch 'origin/master' into network-request-tim…
connorjclark Aug 22, 2022
71705e4
rename
connorjclark Aug 22, 2022
f80540b
jsons
connorjclark Aug 22, 2022
83313ed
fix some tests
connorjclark Aug 22, 2022
ee62cf3
fix all audit tests
connorjclark Aug 22, 2022
cf84651
fix computed tests
connorjclark Aug 22, 2022
1200ecf
tags blocking artifact renames
connorjclark Aug 22, 2022
9be7ba1
update
connorjclark Aug 22, 2022
f76c90d
fix offscreen img
connorjclark Aug 23, 2022
4ec8be3
Merge remote-tracking branch 'origin/master' into network-request-tim…
connorjclark Aug 24, 2022
9c9af6e
internalNetworkRequestTime -> networkRequestTime
connorjclark Oct 18, 2022
bde4695
Merge remote-tracking branch 'origin/main' into network-request-timin…
connorjclark Oct 18, 2022
58a4d27
Apply suggestions from code review
connorjclark Oct 18, 2022
817326e
nits
connorjclark Oct 18, 2022
223eafd
re-renames
connorjclark Oct 28, 2022
39edc02
uh ok
connorjclark Oct 28, 2022
1f0dbfa
keys
connorjclark Oct 28, 2022
5502fb1
Merge remote-tracking branch 'origin/main' into network-request-timin…
connorjclark Nov 4, 2022
1415f79
partial review pass
connorjclark Nov 4, 2022
9410216
add new props to lr fetch timings test
connorjclark Nov 4, 2022
798064e
update simulator test
connorjclark Nov 4, 2022
49a9555
reorder
connorjclark Nov 4, 2022
3f8dd5c
Merge remote-tracking branch 'origin/main' into network-request-timin…
connorjclark Nov 28, 2022
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
6 changes: 3 additions & 3 deletions core/audits/byte-efficiency/offscreen-images.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const IGNORE_THRESHOLD_IN_BYTES = 2048;
const IGNORE_THRESHOLD_IN_PERCENT = 75;
const IGNORE_THRESHOLD_IN_MS = 50;

/** @typedef {{node: LH.Audit.Details.NodeValue, url: string, requestStartTime: number, totalBytes: number, wastedBytes: number, wastedPercent: number}} WasteResult */
/** @typedef {{node: LH.Audit.Details.NodeValue, url: string, rendererStartTime: number, totalBytes: number, wastedBytes: number, wastedPercent: number}} WasteResult */

class OffscreenImages extends ByteEfficiencyAudit {
/**
Expand Down Expand Up @@ -102,7 +102,7 @@ class OffscreenImages extends ByteEfficiencyAudit {
return {
node: ByteEfficiencyAudit.makeNodeItem(image.node),
url,
requestStartTime: networkRecord.startTime,
rendererStartTime: networkRecord.rendererStartTime,
totalBytes,
wastedBytes,
wastedPercent: 100 * wastedRatio,
Expand Down Expand Up @@ -151,7 +151,7 @@ class OffscreenImages extends ByteEfficiencyAudit {
return images.filter(image => {
if (image.wastedBytes < IGNORE_THRESHOLD_IN_BYTES) return false;
if (image.wastedPercent < IGNORE_THRESHOLD_IN_PERCENT) return false;
return image.requestStartTime < interactiveTimestamp / 1e6 - IGNORE_THRESHOLD_IN_MS / 1000;
return image.rendererStartTime < interactiveTimestamp / 1000 - IGNORE_THRESHOLD_IN_MS;
});
}

Expand Down
2 changes: 1 addition & 1 deletion core/audits/byte-efficiency/render-blocking-resources.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class RenderBlockingResources extends Audit {
const deferredNodeIds = new Set();
for (const resource of artifacts.TagsBlockingFirstPaint) {
// Ignore any resources that finished after observed FCP (they're clearly not render-blocking)
if (resource.endTime * 1000 > fcpTsInMs) continue;
if (resource.networkEndTime > fcpTsInMs) continue;
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 make sense to use mainThreadEndTime here?

// TODO: beacon to Sentry, https://github.com/GoogleChrome/lighthouse/issues/7041
if (!nodesByUrl[resource.tag.url]) continue;

Expand Down
91 changes: 68 additions & 23 deletions core/audits/critical-request-chains.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,26 +41,70 @@ class CriticalRequestChains extends Audit {
};
}

/** @typedef {{depth: number, id: string, chainDuration: number, chainTransferSize: number, node: LH.Audit.Details.SimpleCriticalRequestNode[string]}} CrcNodeInfo */
// TODO: if we are OK with changing the audit details we can easily delete all this duplication.
// Only difference between these trees is the shape of the network request object.
/** @typedef {{depth: number, id: string, chainDuration: number, chainTransferSize: number, node: LH.Artifacts.CriticalRequestNode}} CrcNodeInfo */
/** @typedef {{depth: number, id: string, chainDuration: number, chainTransferSize: number, node: LH.Audit.Details.SimpleCriticalRequestNode}} CrcNodeInfo2 */

/**
* @param {LH.Audit.Details.SimpleCriticalRequestNode} tree
* @param {LH.Artifacts.CriticalRequestTree} tree
* @param {function(CrcNodeInfo): void} cb
*/
static _traverse(tree, cb) {
static _traverseArtifact(tree, cb) {
/**
* @param {LH.Audit.Details.SimpleCriticalRequestNode} node
* @param {LH.Artifacts.CriticalRequestTree} tree
* @param {number} depth
* @param {number=} startTime
* @param {number=} transferSize
*/
function walk(node, depth, startTime, transferSize = 0) {
const children = Object.keys(node);
function walk(tree, depth, startTime, transferSize = 0) {
const children = Object.keys(tree);
if (children.length === 0) {
return;
}
children.forEach(id => {
const child = node[id];
const child = tree[id];
if (!startTime) {
startTime = child.request.rendererStartTime;
}

// Call the callback with the info for this child.
cb({
depth,
id,
node: child,
chainDuration: (child.request.networkEndTime - startTime) / 1000,
chainTransferSize: (transferSize + child.request.transferSize),
});

// Carry on walking.
if (child.children) {
walk(child.children, depth + 1, startTime);
}
}, '');
}

walk(tree, 0);
}

/**
* @param {LH.Audit.Details.SimpleCriticalRequestTree} tree
* @param {function(CrcNodeInfo2): void} cb
*/
static _traverseDetails(tree, cb) {
/**
* @param {LH.Audit.Details.SimpleCriticalRequestTree} tree
* @param {number} depth
* @param {number=} startTime
* @param {number=} transferSize
*/
function walk(tree, depth, startTime, transferSize = 0) {
const children = Object.keys(tree);
if (children.length === 0) {
return;
}
children.forEach(id => {
const child = tree[id];
if (!startTime) {
startTime = child.request.startTime;
}
Expand All @@ -70,7 +114,7 @@ class CriticalRequestChains extends Audit {
depth,
id,
node: child,
chainDuration: (child.request.endTime - startTime) * 1000,
chainDuration: child.request.endTime - startTime,
chainTransferSize: (transferSize + child.request.transferSize),
});

Expand All @@ -86,7 +130,7 @@ class CriticalRequestChains extends Audit {

/**
* Get stats about the longest initiator chain (as determined by time duration)
* @param {LH.Audit.Details.SimpleCriticalRequestNode} tree
* @param {LH.Audit.Details.SimpleCriticalRequestTree} tree
* @return {{duration: number, length: number, transferSize: number}}
*/
static _getLongestChain(tree) {
Expand All @@ -95,7 +139,7 @@ class CriticalRequestChains extends Audit {
length: 0,
transferSize: 0,
};
CriticalRequestChains._traverse(tree, opts => {
CriticalRequestChains._traverseDetails(tree, opts => {
const duration = opts.chainDuration;
if (duration > longest.duration) {
longest.duration = duration;
Expand All @@ -109,23 +153,23 @@ class CriticalRequestChains extends Audit {
}

/**
* @param {LH.Artifacts.CriticalRequestNode} tree
* @return {LH.Audit.Details.SimpleCriticalRequestNode}
* @param {LH.Artifacts.CriticalRequestTree} tree
* @return {LH.Audit.Details.SimpleCriticalRequestTree}
*/
static flattenRequests(tree) {
/** @type {LH.Audit.Details.SimpleCriticalRequestNode} */
/** @type {LH.Audit.Details.SimpleCriticalRequestTree} */
const flattendChains = {};
/** @type {Map<string, LH.Audit.Details.SimpleCriticalRequestNode[string]>} */
/** @type {Map<string, LH.Audit.Details.SimpleCriticalRequestNode>} */
const chainMap = new Map();

/** @param {CrcNodeInfo} opts */
function flatten(opts) {
const request = opts.node.request;
const simpleRequest = {
url: request.url,
startTime: request.startTime,
endTime: request.endTime,
responseReceivedTime: request.responseReceivedTime,
startTime: request.rendererStartTime,
endTime: request.rendererEndTime,
responseReceivedTime: request.responseHeadersEndTime,
transferSize: request.transferSize,
};

Expand All @@ -142,7 +186,7 @@ class CriticalRequestChains extends Audit {
if (opts.node.children) {
for (const chainId of Object.keys(opts.node.children)) {
// Note: cast should be Partial<>, but filled in when child node is traversed.
const childChain = /** @type {LH.Audit.Details.SimpleCriticalRequestNode[string]} */ ({
const childChain = /** @type {LH.Audit.Details.SimpleCriticalRequestNode} */ ({
request: {},
});
chainMap.set(chainId, childChain);
Expand All @@ -155,7 +199,7 @@ class CriticalRequestChains extends Audit {
chainMap.set(opts.id, chain);
}

CriticalRequestChains._traverse(tree, flatten);
CriticalRequestChains._traverseArtifact(tree, flatten);

return flattendChains;
}
Expand All @@ -171,16 +215,17 @@ class CriticalRequestChains extends Audit {
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const URL = artifacts.URL;
const chains = await ComputedChains.request({devtoolsLog, trace, URL}, context);

let chainCount = 0;
/**
* @param {LH.Audit.Details.SimpleCriticalRequestNode} node
* @param {LH.Audit.Details.SimpleCriticalRequestTree} tree
* @param {number} depth
*/
function walk(node, depth) {
const childIds = Object.keys(node);
function walk(tree, depth) {
const childIds = Object.keys(tree);

childIds.forEach(id => {
const child = node[id];
const child = tree[id];
if (child.children) {
walk(child.children, depth + 1);
} else {
Expand Down
2 changes: 1 addition & 1 deletion core/audits/font-display.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ class FontDisplay extends Audit {
.map(record => {
// In reality the end time should be calculated with paint time included
// all browsers wait 3000ms to block text so we make sure 3000 is our max wasted time
const wastedMs = Math.min((record.endTime - record.startTime) * 1000, 3000);
const wastedMs = Math.min(record.rendererEndTime - record.rendererStartTime, 3000);

return {
url: record.url,
Expand Down
27 changes: 17 additions & 10 deletions core/audits/network-requests.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ class NetworkRequests extends Audit {
static async audit(artifacts, context) {
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
const records = await NetworkRecords.request(devtoolsLog, context);
const earliestStartTime = records.reduce(
(min, record) => Math.min(min, record.startTime),
const earliestMainThreadStartTime = records.reduce(
(min, record) => Math.min(min, record.rendererStartTime),
Infinity
);

Expand All @@ -47,8 +47,8 @@ class NetworkRequests extends Audit {
}

/** @param {number} time */
const timeToMs = time => time < earliestStartTime || !Number.isFinite(time) ?
undefined : (time - earliestStartTime) * 1000;
const normalizeTime = time => time < earliestMainThreadStartTime || !Number.isFinite(time) ?
undefined : time - earliestMainThreadStartTime;

const results = records.map(record => {
const endTimeDeltaMs = record.lrStatistics?.endTimeDeltaMs;
Expand All @@ -64,8 +64,11 @@ class NetworkRequests extends Audit {
return {
url: UrlUtils.elideDataURI(record.url),
protocol: record.protocol,
startTime: timeToMs(record.startTime),
endTime: timeToMs(record.endTime),
rendererStartTime: normalizeTime(record.rendererStartTime),
networkRequestTime: normalizeTime(record.networkRequestTime),
responseHeadersEndTime: normalizeTime(record.responseHeadersEndTime),
networkEndTime: normalizeTime(record.networkEndTime),
rendererEndTime: normalizeTime(record.rendererEndTime),
finished: record.finished,
transferSize: record.transferSize,
resourceSize: record.resourceSize,
Expand All @@ -87,8 +90,12 @@ class NetworkRequests extends Audit {
const headings = [
{key: 'url', valueType: 'url', label: 'URL'},
{key: 'protocol', valueType: 'text', label: 'Protocol'},
{key: 'startTime', valueType: 'ms', granularity: 1, label: 'Start Time'},
{key: 'endTime', valueType: 'ms', granularity: 1, label: 'End Time'},
{key: 'rendererStartTime', valueType: 'ms', granularity: 1, label: 'Renderer Start Time'},
{key: 'networkRequestTime', valueType: 'ms', granularity: 1, label: 'Network Request Time'},
// eslint-disable-next-line max-len
{key: 'responseHeadersEndTime', valueType: 'ms', granularity: 1, label: 'Response Headers End Time'},
{key: 'networkEndTime', valueType: 'ms', granularity: 1, label: 'Network End Time'},
{key: 'rendererEndTime', valueType: 'ms', granularity: 1, label: 'Renderer End Time'},
{
key: 'transferSize',
valueType: 'bytes',
Expand All @@ -111,8 +118,8 @@ class NetworkRequests extends Audit {
const tableDetails = Audit.makeTableDetails(headings, results);

// Include starting timestamp to allow syncing requests with navStart/metric timestamps.
const networkStartTimeTs = Number.isFinite(earliestStartTime) ?
earliestStartTime * 1_000_000 : undefined;
const networkStartTimeTs = Number.isFinite(earliestMainThreadStartTime) ?
earliestMainThreadStartTime * 1000 : undefined;
tableDetails.debugData = {
type: 'debugdata',
networkStartTimeTs,
Expand Down
5 changes: 3 additions & 2 deletions core/audits/redirects.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,10 @@ class Redirects extends Audit {
}

const lanternTimingDeltaMs = redirectedTiming.startTime - initialTiming.startTime;
const observedTimingDeltaS = redirectedRequest.startTime - initialRequest.startTime;
const observedTimingDelta =
redirectedRequest.rendererStartTime - initialRequest.rendererStartTime;
const wastedMs = settings.throttlingMethod === 'simulate' ?
lanternTimingDeltaMs : observedTimingDeltaS * 1000;
lanternTimingDeltaMs : observedTimingDelta;
totalWastedMs += wastedMs;

tableRows.push({
Expand Down
24 changes: 12 additions & 12 deletions core/audits/uses-rel-preconnect.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {LanternLargestContentfulPaint} from '../computed/metrics/lantern-largest
// around for 10s. Meaning, the time delta between processing preconnect a request should be <10s,
// otherwise it's wasted. We add a 5s margin so we are sure to capture all key requests.
// @see https://github.com/GoogleChrome/lighthouse/issues/3106#issuecomment-333653747
const PRECONNECT_SOCKET_MAX_IDLE = 15;
const PRECONNECT_SOCKET_MAX_IDLE = 15_000;

const IGNORE_THRESHOLD_IN_MS = 50;

Expand Down Expand Up @@ -90,13 +90,14 @@ class UsesRelPreconnectAudit extends Audit {
}

/**
* Check is the connection has started before the socket idle time
* Check if the connection has started before the socket idle time
* @param {LH.Artifacts.NetworkRequest} record
* @param {LH.Artifacts.NetworkRequest} mainResource
* @return {boolean}
*/
static socketStartTimeIsBelowThreshold(record, mainResource) {
return Math.max(0, record.startTime - mainResource.endTime) < PRECONNECT_SOCKET_MAX_IDLE;
static connectionGoesUnusedForTooLong(record, mainResource) {
const delta = Math.max(0, record.networkRequestTime - mainResource.networkEndTime);
return delta < PRECONNECT_SOCKET_MAX_IDLE;
}

/**
Expand Down Expand Up @@ -151,7 +152,7 @@ class UsesRelPreconnectAudit extends Audit {
// Filter out all resources where origins are already resolved.
UsesRelPreconnectAudit.hasAlreadyConnectedToOrigin(record) ||
// Make sure the requests are below the PRECONNECT_SOCKET_MAX_IDLE (15s) mark.
!UsesRelPreconnectAudit.socketStartTimeIsBelowThreshold(record, mainResource)
!UsesRelPreconnectAudit.connectionGoesUnusedForTooLong(record, mainResource)
) {
return;
}
Expand All @@ -169,11 +170,10 @@ class UsesRelPreconnectAudit extends Audit {
/** @type {Array<{url: string, wastedMs: number}>}*/
let results = [];
origins.forEach(records => {
// Sometimes requests are done simultaneous and the connection has not been made
// chrome will try to connect for each network record, we get the first record
const firstRecordOfOrigin = records.reduce((firstRecord, record) => {
return (record.startTime < firstRecord.startTime) ? record : firstRecord;
});
// We just need a single record, let's grab the earliest there is.
const firstRecordOfOrigin = records.reduce((firstRecord, record) =>
record.rendererStartTime < firstRecord.rendererStartTime ? record : firstRecord
);

// Skip the origin if we don't have timing information
if (!firstRecordOfOrigin.timing) return;
Expand All @@ -189,8 +189,8 @@ class UsesRelPreconnectAudit extends Audit {
if (firstRecordOfOrigin.parsedURL.scheme === 'https') connectionTime = connectionTime * 2;

const timeBetweenMainResourceAndDnsStart =
firstRecordOfOrigin.startTime * 1000 -
mainResource.endTime * 1000 +
firstRecordOfOrigin.networkRequestTime -
mainResource.networkEndTime +
firstRecordOfOrigin.timing.dnsStart;

const wastedMs = Math.min(connectionTime, timeBetweenMainResourceAndDnsStart);
Expand Down
12 changes: 6 additions & 6 deletions core/computed/critical-request-chains.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,17 @@ class CriticalRequestChains {
* Create a tree of critical requests.
* @param {LH.Artifacts.NetworkRequest} mainResource
* @param {LH.Gatherer.Simulation.GraphNode} graph
* @return {LH.Artifacts.CriticalRequestNode}
* @return {LH.Artifacts.CriticalRequestTree}
*/
static extractChainsFromGraph(mainResource, graph) {
/** @type {LH.Artifacts.CriticalRequestNode} */
const rootNode = {};
/** @type {LH.Artifacts.CriticalRequestTree} */
const tree = {};

/**
* @param {LH.Artifacts.NetworkRequest[]} path
*/
function addChain(path) {
let currentNode = rootNode;
let currentNode = tree;

for (const record of path) {
if (!currentNode[record.requestId]) {
Expand Down Expand Up @@ -122,13 +122,13 @@ class CriticalRequestChains {
addChain(networkPath);
}, getNextNodes);

return rootNode;
return tree;
}

/**
* @param {{URL: LH.Artifacts['URL'], devtoolsLog: LH.DevtoolsLog, trace: LH.Trace}} data
* @param {LH.Artifacts.ComputedContext} context
* @return {Promise<LH.Artifacts.CriticalRequestNode>}
* @return {Promise<LH.Artifacts.CriticalRequestTree>}
*/
static async compute_(data, context) {
const mainResource = await MainResource.request(data, context);
Expand Down
Loading