Skip to content

core(network-request): use ms instead of seconds #14567

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 4 commits into from
Nov 29, 2022
Merged

core(network-request): use ms instead of seconds #14567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eda995e
core(network-request): use ms instead of seconds
connorjclark Nov 28, 2022
93b3a3b
Merge branch 'main' into network-ms
connorjclark Nov 29, 2022
3f4984b
comment
connorjclark Nov 29, 2022
225da2c
Merge remote-tracking branch 'origin/main' into network-ms
connorjclark Nov 29, 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
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 @@ -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.requestStartTime < interactiveTimestamp / 1000 - IGNORE_THRESHOLD_IN_MS;
});
}

Expand Down
2 changes: 1 addition & 1 deletion core/audits/byte-efficiency/render-blocking-resources.js
View file
Open in desktop
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.endTime > fcpTsInMs) continue;
// TODO: beacon to Sentry, https://github.com/GoogleChrome/lighthouse/issues/7041
if (!nodesByUrl[resource.tag.url]) continue;

Expand Down
12 changes: 6 additions & 6 deletions core/audits/critical-request-chains.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ class CriticalRequestChains extends Audit {
depth,
id,
node: child,
chainDuration: (child.request.endTime - startTime) * 1000,
chainTransferSize: (transferSize + child.request.transferSize),
chainDuration: child.request.endTime - startTime,
chainTransferSize: transferSize + child.request.transferSize,
});

// Carry on walking.
Expand All @@ -96,7 +96,7 @@ class CriticalRequestChains extends Audit {
transferSize: 0,
};
CriticalRequestChains._traverse(tree, opts => {
const duration = opts.chainDuration;
const duration = opts.chainDuration * 1000;
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,
endTime: request.endTime,
responseReceivedTime: request.responseReceivedTime,
startTime: request.startTime / 1000,
endTime: request.endTime / 1000,
responseReceivedTime: request.responseReceivedTime / 1000,
transferSize: request.transferSize,
};

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) * 1000, 3000);
const wastedMs = Math.min(record.endTime - record.startTime, 3000);

return {
url: record.url,
Expand Down
10 changes: 5 additions & 5 deletions core/audits/network-requests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 < earliestStartTime || !Number.isFinite(time) ?
undefined : (time - earliestStartTime);

const results = records.map(record => {
const endTimeDeltaMs = record.lrStatistics?.endTimeDeltaMs;
Expand All @@ -64,8 +64,8 @@ class NetworkRequests extends Audit {
return {
url: UrlUtils.elideDataURI(record.url),
protocol: record.protocol,
startTime: timeToMs(record.startTime),
endTime: timeToMs(record.endTime),
startTime: normalizeTime(record.startTime),
endTime: normalizeTime(record.endTime),
finished: record.finished,
transferSize: record.transferSize,
resourceSize: record.resourceSize,
Expand Down Expand Up @@ -112,7 +112,7 @@ class NetworkRequests extends Audit {

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

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

tableRows.push({
Expand Down
10 changes: 5 additions & 5 deletions core/audits/uses-rel-preconnect.js
View file
Open in desktop
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_IN_MS = 15_000;

const IGNORE_THRESHOLD_IN_MS = 50;

Expand Down Expand Up @@ -96,7 +96,7 @@ class UsesRelPreconnectAudit extends Audit {
* @return {boolean}
*/
static socketStartTimeIsBelowThreshold(record, mainResource) {
return Math.max(0, record.startTime - mainResource.endTime) < PRECONNECT_SOCKET_MAX_IDLE;
return Math.max(0, record.startTime - mainResource.endTime) < PRECONNECT_SOCKET_MAX_IDLE_IN_MS;
}

/**
Expand Down Expand Up @@ -150,7 +150,7 @@ class UsesRelPreconnectAudit extends Audit {
!lcpGraphURLs.has(record.url) ||
// 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.
// Make sure the requests are below the PRECONNECT_SOCKET_MAX_IDLE_IN_MS (15s) mark.
!UsesRelPreconnectAudit.socketStartTimeIsBelowThreshold(record, mainResource)
) {
return;
Expand Down Expand Up @@ -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.startTime -
mainResource.endTime +
firstRecordOfOrigin.timing.dnsStart;

const wastedMs = Math.min(connectionTime, timeBetweenMainResourceAndDnsStart);
Expand Down
8 changes: 6 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 @@ -22,17 +22,21 @@ class NetworkNode extends BaseNode {
}

/**
* In seconds.
* TODO: make ms.
* @return {number}
*/
get startTime() {
return this._record.startTime * 1000 * 1000;
return this._record.startTime * 1000;
Copy link
Member

Choose a reason for hiding this comment

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

seems kinda awkward that the lantern depgraph and crc are still in seconds.

wdyt about changing both of those in here, too?

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 mentioned this in the other PR, but I would like to keep lantern changes in a separate PR. I expect it to change a number of things in a trivial way (snapshots, accuracy) due to the limits of floating point precision. It should be a quick change I can follow up with when this lands.

Copy link
Member

Choose a reason for hiding this comment

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

ok!

Copy link
Member

Choose a reason for hiding this comment

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

maybe add some comments in network-node/base-node/cpu-node and crc that these times are in seconds?
and/or a TODO?

cuz right now its kinda impossible to tell from the code.

}

/**
* In seconds.
* TODO: make ms.
* @return {number}
*/
get endTime() {
return this._record.endTime * 1000 * 1000;
return this._record.endTime * 1000;
}

/**
Expand Down
6 changes: 3 additions & 3 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) * 1000;
const totalTime = record.endTime - record.startTime;
const downloadTimeAfterFirstByte = totalTime - timing.receiveHeadersEnd;
const numberOfRoundTrips = Math.log2(record.transferSize / INITIAL_CWD);

Expand Down Expand Up @@ -399,8 +399,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, isStart: true});
boundaries.push({time: record.endTime, isStart: false});
boundaries.push({time: record.responseReceivedTime / 1000, isStart: true});
boundaries.push({time: record.endTime / 1000, isStart: false});
return boundaries;
}, /** @type {Array<{time: number, isStart: boolean}>} */([])).sort((a, b) => a.time - b.time);

Expand Down
27 changes: 16 additions & 11 deletions core/lib/network-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,14 @@ class NetworkRequest {
this.parsedURL = /** @type {ParsedURL} */ ({scheme: ''});
this.documentURL = '';

/**
* When the network service is about to handle a request, ie. just before going to the
* HTTP cache or going to the network for DNS/connection setup, in milliseconds.
*/
this.startTime = -1;
/** @type {number} */
/** When the last byte of the response body is received, in milliseconds. */
this.endTime = -1;
/** @type {number} */
/** When the last byte of the response headers is received, in milliseconds. */
this.responseReceivedTime = -1;

// Go read the comment on _updateTransferSizeForLightrider.
Expand Down Expand Up @@ -217,7 +221,8 @@ class NetworkRequest {
};
this.isSecure = UrlUtils.isSecureScheme(this.parsedURL.scheme);

this.startTime = data.timestamp;
// Expected to be overriden with better value in `_recomputeTimesWithResourceTiming`.
this.startTime = data.timestamp * 1000;

this.requestMethod = data.request.method;

Expand Down Expand Up @@ -261,7 +266,7 @@ class NetworkRequest {
if (this.finished) return;

this.finished = true;
this.endTime = data.timestamp;
this.endTime = data.timestamp * 1000;
if (data.encodedDataLength >= 0) {
this.transferSize = data.encodedDataLength;
}
Expand All @@ -279,7 +284,7 @@ class NetworkRequest {
if (this.finished) return;

this.finished = true;
this.endTime = data.timestamp;
this.endTime = data.timestamp * 1000;

this.failed = true;
this.resourceType = data.type && RESOURCE_TYPES[data.type];
Expand All @@ -305,7 +310,7 @@ class NetworkRequest {
this._onResponse(data.redirectResponse, data.timestamp, data.type);
this.resourceType = undefined;
this.finished = true;
this.endTime = data.timestamp;
this.endTime = data.timestamp * 1000;

this._updateResponseReceivedTimeIfNecessary();
}
Expand All @@ -319,7 +324,7 @@ class NetworkRequest {

/**
* @param {LH.Crdp.Network.Response} response
* @param {number} timestamp
* @param {number} timestamp in seconds
* @param {LH.Crdp.Network.ResponseReceivedEvent['type']=} resourceType
*/
_onResponse(response, timestamp, resourceType) {
Expand All @@ -330,7 +335,7 @@ class NetworkRequest {

if (response.protocol) this.protocol = response.protocol;

this.responseReceivedTime = timestamp;
this.responseReceivedTime = timestamp * 1000;

this.transferSize = response.encodedDataLength;
if (typeof response.fromDiskCache === 'boolean') this.fromDiskCache = response.fromDiskCache;
Expand Down Expand Up @@ -364,8 +369,8 @@ class NetworkRequest {
// Take startTime and responseReceivedTime from timing data for better accuracy.
// Timing's requestTime is a baseline in seconds, rest of the numbers there are ticks in millis.
// TODO: This skips the "queuing time" before the netstack has taken over ... is this a mistake?
this.startTime = timing.requestTime;
const headersReceivedTime = timing.requestTime + timing.receiveHeadersEnd / 1000;
this.startTime = timing.requestTime * 1000;
const headersReceivedTime = this.startTime + timing.receiveHeadersEnd;
if (!this.responseReceivedTime || this.responseReceivedTime < 0) {
this.responseReceivedTime = headersReceivedTime;
}
Expand Down Expand Up @@ -478,7 +483,7 @@ class NetworkRequest {
}

this.lrStatistics = {
endTimeDeltaMs: (this.endTime - (this.startTime + (totalMs / 1000))) * 1000,
endTimeDeltaMs: this.endTime - (this.startTime + totalMs),
TCPMs: TCPMs,
requestMs: requestMs,
responseMs: responseMs,
Expand Down
18 changes: 9 additions & 9 deletions core/test/audits/byte-efficiency/offscreen-images-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ describe('OffscreenImages audit', () => {

it('disregards images loaded after TTI', () => {
const topLevelTasks = [{ts: 1900, duration: 100}];
const networkRecord = generateRecord({resourceSizeInKb: 100, startTime: 3});
const networkRecord = generateRecord({resourceSizeInKb: 100, startTime: 3000});
const artifacts = {
ViewportDimensions: DEFAULT_DIMENSIONS,
GatherContext: {gatherMode: 'navigation'},
Expand All @@ -377,7 +377,7 @@ describe('OffscreenImages audit', () => {
});

it('disregards images loaded after Trace End when interactive throws error', () => {
const networkRecord = generateRecord({resourceSizeInKb: 100, startTime: 3});
const networkRecord = generateRecord({resourceSizeInKb: 100, startTime: 3000});
const artifacts = {
ViewportDimensions: DEFAULT_DIMENSIONS,
GatherContext: {gatherMode: 'navigation'},
Expand Down Expand Up @@ -420,7 +420,7 @@ describe('OffscreenImages audit', () => {
resourceSize: wastedSize,
transferSize: wastedSize,
requestId: 'a',
startTime: 1,
startTime: 1000,
priority: 'High',
timing: {receiveHeadersEnd: 1.25},
};
Expand All @@ -429,7 +429,7 @@ describe('OffscreenImages audit', () => {
resourceSize: wastedSize,
transferSize: wastedSize,
requestId: 'b',
startTime: 2.25,
startTime: 2_250,
priority: 'High',
timing: {receiveHeadersEnd: 2.5},
};
Expand Down Expand Up @@ -481,7 +481,7 @@ describe('OffscreenImages audit', () => {
resourceSize: wastedSize,
transferSize: wastedSize,
requestId: 'a',
startTime: 1,
startTime: 1000,
priority: 'High',
timing: {receiveHeadersEnd: 1.25},
};
Expand All @@ -490,7 +490,7 @@ describe('OffscreenImages audit', () => {
resourceSize: wastedSize,
transferSize: wastedSize,
requestId: 'b',
startTime: 1.25,
startTime: 1_250,
priority: 'High',
timing: {receiveHeadersEnd: 1.5},
};
Expand Down Expand Up @@ -542,8 +542,8 @@ describe('OffscreenImages audit', () => {
it('rethrow error when interactive throws error in Lantern', async () => {
context = {settings: {throttlingMethod: 'simulate'}, computedCache: new Map()};
const networkRecords = [
generateRecord({url: 'a', resourceSizeInKb: 100, startTime: 3}),
generateRecord({url: 'b', resourceSizeInKb: 100, startTime: 4}),
generateRecord({url: 'a', resourceSizeInKb: 100, startTime: 3000}),
generateRecord({url: 'b', resourceSizeInKb: 100, startTime: 4000}),
];
const artifacts = {
ViewportDimensions: DEFAULT_DIMENSIONS,
Expand Down Expand Up @@ -583,7 +583,7 @@ describe('OffscreenImages audit', () => {
resourceSize: wastedSize,
transferSize: 0,
requestId: 'a',
startTime: 1,
startTime: 1000,
priority: 'High',
timing: {receiveHeadersEnd: 1.25},
};
Expand Down
Loading