Skip to content

core(network-request): switch to improved timing names #14721

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

Merged
merged 5 commits into from
Feb 1, 2023
Merged

core(network-request): switch to improved timing names #14721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
381d671
core(network-request): switch to improved timing names
brendankenny Jan 26, 2023
ff0de30
sample jsons
brendankenny Jan 27, 2023
8d8aa57
lint
brendankenny Jan 27, 2023
ab3e6bc
Merge branch 'main' into netnames
brendankenny Feb 1, 2023
e1a9ca4
Merge branch 'main' into netnames
brendankenny Feb 1, 2023
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
2 changes: 1 addition & 1 deletion core/audits/byte-efficiency/offscreen-images.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class OffscreenImages extends ByteEfficiencyAudit {
return {
node: ByteEfficiencyAudit.makeNodeItem(image.node),
url,
requestStartTime: networkRecord.startTime,
requestStartTime: networkRecord.networkRequestTime,
totalBytes,
wastedBytes,
wastedPercent: 100 * wastedRatio,
Expand Down
30 changes: 15 additions & 15 deletions core/audits/critical-request-chains.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,42 +41,42 @@ class CriticalRequestChains extends Audit {
};
}

/** @typedef {{depth: number, id: string, chainDuration: number, chainTransferSize: number, node: LH.Audit.Details.SimpleCriticalRequestNode[string]}} CrcNodeInfo */
/** @typedef {{depth: number, id: string, chainDuration: number, chainTransferSize: number, node: LH.Artifacts.CriticalRequestNode[string]}} CrcNodeInfo */
Copy link
Contributor Author

@brendankenny brendankenny Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This file has the only real changes. This was actually always LH.Artifacts.CriticalRequestNode because the input was the (computed) artifact, with node.request being an actual NetworkRequest, but previous to this PR that was actually structurally compatible with the simplified LH.Audit.Details.SimpleCriticalRequestNode:

lighthouse/types/lhr/audit-details.d.ts

Lines 32 to 42 in a2977ff

type SimpleCriticalRequestNode = {
[id: string]: {
request: {
url: string;
startTime: number;
endTime: number;
responseReceivedTime: number;
transferSize: number;
};
children?: SimpleCriticalRequestNode;
}

so the two types could be used interchangeably.

Since we don't want to change LH.Audit.Details.SimpleCriticalRequestNode, but node.request now has networkRequestTime, etc on it, this file needs to now actually differentiate between the two, and there needs to be a copy step from the new property names to the old property names on the audit details.

Nothing actually changes in the output (see sample_v2.json for an example).


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

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

// Carry on walking.
if (child.children) {
walk(child.children, depth + 1, startTime);
walk(child.children, depth + 1, networkRequestTime);
}
}, '');
}
Expand All @@ -86,7 +86,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.Artifacts.CriticalRequestNode} tree
* @return {{duration: number, length: number, transferSize: number}}
*/
static _getLongestChain(tree) {
Expand All @@ -96,7 +96,7 @@ class CriticalRequestChains extends Audit {
transferSize: 0,
};
CriticalRequestChains._traverse(tree, opts => {
const duration = opts.chainDuration * 1000;
const duration = opts.chainDuration;
if (duration > longest.duration) {
longest.duration = duration;
longest.transferSize = opts.chainTransferSize;
Expand All @@ -123,9 +123,9 @@ class CriticalRequestChains extends Audit {
const request = opts.node.request;
const simpleRequest = {
url: request.url,
startTime: request.startTime / 1000,
endTime: request.endTime / 1000,
responseReceivedTime: request.responseReceivedTime / 1000,
startTime: request.networkRequestTime / 1000,
endTime: request.networkEndTime / 1000,
responseReceivedTime: request.responseHeadersEndTime / 1000,
transferSize: request.transferSize,
};

Expand Down Expand Up @@ -199,7 +199,7 @@ class CriticalRequestChains extends Audit {
walk(initialNavChildren, 0);
}

const longestChain = CriticalRequestChains._getLongestChain(flattenedChains);
const longestChain = CriticalRequestChains._getLongestChain(chains);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT this never made sense to run on flattenedChains, since it recomputes the durations anyways, but it previously worked either way. Now it just needs to drop the * 1000 since request times are in milliseconds after #14567


return {
score: Number(chainCount === 0),
Expand Down
2 changes: 1 addition & 1 deletion core/audits/font-display.js
View file
Open in desktop
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, 3000);
const wastedMs = Math.min(record.networkEndTime - record.networkRequestTime, 3000);

return {
url: record.url,
Expand Down
8 changes: 4 additions & 4 deletions core/audits/network-requests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@ class NetworkRequests extends Audit {
url: UrlUtils.elideDataURI(record.url),
protocol: record.protocol,
rendererStartTime: normalizeTime(record.rendererStartTime),
startTime: normalizeTime(record.startTime),
endTime: normalizeTime(record.endTime),
networkRequestTime: normalizeTime(record.networkRequestTime),
networkEndTime: normalizeTime(record.networkEndTime),
finished: record.finished,
transferSize: record.transferSize,
resourceSize: record.resourceSize,
Expand All @@ -88,8 +88,8 @@ 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: 'networkRequestTime', valueType: 'ms', granularity: 1, label: 'Network Request Time'},
{key: 'networkEndTime', valueType: 'ms', granularity: 1, label: 'Network End Time'},
{
key: 'transferSize',
valueType: 'bytes',
Expand Down
3 changes: 2 additions & 1 deletion core/audits/redirects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ class Redirects extends Audit {
}

const lanternTimingDeltaMs = redirectedTiming.startTime - initialTiming.startTime;
const observedTimingDeltaMs = redirectedRequest.startTime - initialRequest.startTime;
const observedTimingDeltaMs = redirectedRequest.networkRequestTime -
initialRequest.networkRequestTime;
const wastedMs = settings.throttlingMethod === 'simulate' ?
lanternTimingDeltaMs : observedTimingDeltaMs;
totalWastedMs += wastedMs;
Expand Down
9 changes: 5 additions & 4 deletions core/audits/uses-rel-preconnect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ class UsesRelPreconnectAudit extends Audit {
* @return {boolean}
*/
static socketStartTimeIsBelowThreshold(record, mainResource) {
return Math.max(0, record.startTime - mainResource.endTime) < PRECONNECT_SOCKET_MAX_IDLE_IN_MS;
const timeSinceMainEnd = Math.max(0, record.networkRequestTime - mainResource.networkEndTime);
return timeSinceMainEnd < PRECONNECT_SOCKET_MAX_IDLE_IN_MS;
Comment on lines +98 to +99
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only other real code change, because it was kind of hard to read split over two lines. An intermediate variable seemed fine.

}

/**
Expand Down Expand Up @@ -169,7 +170,7 @@ class UsesRelPreconnectAudit extends Audit {
// 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;
return (record.networkRequestTime < firstRecord.networkRequestTime) ? record : firstRecord;
});

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

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

const wastedMs = Math.min(connectionTime, timeBetweenMainResourceAndDnsStart);
Expand Down
4 changes: 2 additions & 2 deletions core/gather/driver/network-monitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,9 @@ class NetworkMonitor extends NetworkMonitorEventEmitter {
if (request.protocol === 'ws' || request.protocol === 'wss') return;

// convert the network timestamp to ms
timeBoundaries.push({time: request.startTime * 1000, isStart: true});
timeBoundaries.push({time: request.networkRequestTime * 1000, isStart: true});
if (request.finished) {
timeBoundaries.push({time: request.endTime * 1000, isStart: false});
timeBoundaries.push({time: request.networkEndTime * 1000, isStart: false});
}
});

Expand Down
8 changes: 4 additions & 4 deletions core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ class TagsBlockingFirstPaint extends FRGatherer {
*/
static async findBlockingTags(driver, networkRecords) {
const firstRequestEndTime = networkRecords.reduce(
(min, record) => Math.min(min, record.endTime),
(min, record) => Math.min(min, record.networkEndTime),
Infinity
);
const tags = await driver.executionContext.evaluate(collectTagsThatBlockFirstPaint, {args: []});
Expand All @@ -167,7 +167,7 @@ class TagsBlockingFirstPaint extends FRGatherer {
const request = requests.get(tag.url);
if (!request || request.isLinkPreload) continue;

let endTime = request.endTime;
let endTime = request.networkEndTime;
let mediaChanges;

if (tag.tagName === 'LINK') {
Expand All @@ -180,7 +180,7 @@ class TagsBlockingFirstPaint extends FRGatherer {
if (timesResourceBecameNonBlocking.length > 0) {
const earliestNonBlockingTime = Math.min(...timesResourceBecameNonBlocking);
const lastTimeResourceWasBlocking = Math.max(
request.startTime,
request.networkRequestTime,
firstRequestEndTime + earliestNonBlockingTime / 1000
);
endTime = Math.min(endTime, lastTimeResourceWasBlocking);
Expand All @@ -194,7 +194,7 @@ class TagsBlockingFirstPaint extends FRGatherer {
result.push({
tag: {tagName, url, mediaChanges},
transferSize: request.transferSize,
startTime: request.startTime,
startTime: request.networkRequestTime,
endTime,
});

Expand Down
4 changes: 2 additions & 2 deletions core/lib/dependency-graph/network-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ class NetworkNode extends BaseNode {
* @return {number}
*/
get startTime() {
return this._record.startTime * 1000;
return this._record.networkRequestTime * 1000;
}

/**
* @return {number}
*/
get endTime() {
return this._record.endTime * 1000;
return this._record.networkEndTime * 1000;
}

/**
Expand Down
14 changes: 8 additions & 6 deletions core/lib/dependency-graph/simulator/network-analyzer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ class NetworkAnalyzer {
if (!Number.isFinite(timing.receiveHeadersEnd) || timing.receiveHeadersEnd < 0) return;

// Compute the amount of time downloading everything after the first congestion window took
const totalTime = record.endTime - record.startTime;
const totalTime = record.networkEndTime - record.networkRequestTime;
const downloadTimeAfterFirstByte = totalTime - timing.receiveHeadersEnd;
const numberOfRoundTrips = Math.log2(record.transferSize / INITIAL_CWD);

Expand Down Expand Up @@ -279,17 +279,19 @@ class NetworkAnalyzer {
const groupedByOrigin = NetworkAnalyzer.groupByOrigin(records);
for (const [_, originRecords] of groupedByOrigin.entries()) {
const earliestReusePossible = originRecords
.map(record => record.endTime)
.map(record => record.networkEndTime)
.reduce((a, b) => Math.min(a, b), Infinity);

for (const record of originRecords) {
connectionWasReused.set(
record.requestId,
record.startTime >= earliestReusePossible || record.protocol === 'h2'
record.networkRequestTime >= earliestReusePossible || record.protocol === 'h2'
);
}

const firstRecord = originRecords.reduce((a, b) => (a.startTime > b.startTime ? b : a));
const firstRecord = originRecords.reduce((a, b) => {
return a.networkRequestTime > b.networkRequestTime ? b : a;
});
connectionWasReused.set(firstRecord.requestId, false);
}

Expand Down Expand Up @@ -399,8 +401,8 @@ class NetworkAnalyzer {

// If we've made it this far, all the times we need should be valid (i.e. not undefined/-1).
totalBytes += record.transferSize;
boundaries.push({time: record.responseReceivedTime / 1000, isStart: true});
boundaries.push({time: record.endTime / 1000, isStart: false});
boundaries.push({time: record.responseHeadersEndTime / 1000, isStart: true});
boundaries.push({time: record.networkEndTime / 1000, isStart: false});
return boundaries;
}, /** @type {Array<{time: number, isStart: boolean}>} */([])).sort((a, b) => a.time - b.time);

Expand Down
4 changes: 3 additions & 1 deletion core/lib/navigation-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,9 @@ function getPageLoadError(navigationError, context) {
record.resourceType === NetworkRequest.TYPES.Document
);
if (documentRequests.length) {
mainRecord = documentRequests.reduce((min, r) => (r.startTime < min.startTime ? r : min));
mainRecord = documentRequests.reduce((min, r) => {
return r.networkRequestTime < min.networkRequestTime ? r : min;
});
}
}

Expand Down
2 changes: 1 addition & 1 deletion core/lib/network-recorder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ class NetworkRecorder extends RequestEventEmitter {

let candidates = recordsByURL.get(initiatorURL) || [];
// The initiator must come before the initiated request.
candidates = candidates.filter(cand => cand.responseReceivedTime <= record.startTime);
candidates = candidates.filter(c => c.responseHeadersEndTime <= record.networkRequestTime);
if (candidates.length > 1) {
// Disambiguate based on prefetch. Prefetch requests have type 'Other' and cannot
// initiate requests, so we drop them here.
Expand Down
Loading