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(network-analyzer): move throughput to NetworkAnalyzer #5900

Merged
merged 3 commits into from
Sep 28, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 0 additions & 10 deletions lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,6 @@ class UnusedBytes extends Audit {
}
}

/**
* @param {number} bytes
* @param {number} networkThroughput measured in bytes/second
* @return {number}
*/
static bytesToMs(bytes, networkThroughput) {
const milliseconds = bytes / networkThroughput * 1000;
return milliseconds;
}

/**
* Estimates the number of bytes this network record would have consumed on the network based on the
* uncompressed size (totalBytes). Uses the actual transfer size from the network record if applicable.
Expand Down
12 changes: 4 additions & 8 deletions lighthouse-core/audits/byte-efficiency/total-byte-weight.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,20 @@ class TotalByteWeight extends ByteEfficiencyAudit {
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
const devtoolsLogs = artifacts.devtoolsLogs[ByteEfficiencyAudit.DEFAULT_PASS];
const [networkRecords, networkThroughput] = await Promise.all([
artifacts.requestNetworkRecords(devtoolsLogs),
artifacts.requestNetworkThroughput(devtoolsLogs),
]);
const devtoolsLog = artifacts.devtoolsLogs[ByteEfficiencyAudit.DEFAULT_PASS];
const records = await artifacts.requestNetworkRecords(devtoolsLog);

let totalBytes = 0;
/** @type {Array<{url: string, totalBytes: number, totalMs: number}>} */
/** @type {Array<{url: string, totalBytes: number}>} */
let results = [];
networkRecords.forEach(record => {
records.forEach(record => {
// exclude data URIs since their size is reflected in other resources
// exclude unfinished requests since they won't have transfer size information
if (record.parsedURL.scheme === 'data' || !record.finished) return;

const result = {
url: record.url,
totalBytes: record.transferSize,
totalMs: ByteEfficiencyAudit.bytesToMs(record.transferSize, networkThroughput),
};

totalBytes += result.totalBytes;
Expand Down
13 changes: 8 additions & 5 deletions lighthouse-core/gather/computed/network-analysis.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class NetworkAnalysis extends ComputedArtifact {

/**
* @param {Array<LH.Artifacts.NetworkRequest>} records
* @return {LH.Artifacts.NetworkAnalysis}
* @return {Omit<LH.Artifacts.NetworkAnalysis, 'throughput'|'records'>}
*/
static computeRTTAndServerResponseTime(records) {
// First pass compute the estimated observed RTT to each origin's servers.
Expand Down Expand Up @@ -45,7 +45,11 @@ class NetworkAnalysis extends ComputedArtifact {
serverResponseTimeByOrigin.set(origin, summary.median);
}

return {rtt: minimumRtt, additionalRttByOrigin, serverResponseTimeByOrigin, throughput: 0};
return {
rtt: minimumRtt,
additionalRttByOrigin,
serverResponseTimeByOrigin,
};
}

/**
Expand All @@ -55,10 +59,9 @@ class NetworkAnalysis extends ComputedArtifact {
*/
async compute_(devtoolsLog, computedArtifacts) {
const records = await computedArtifacts.requestNetworkRecords(devtoolsLog);
const throughput = await computedArtifacts.requestNetworkThroughput(devtoolsLog);
const throughput = NetworkAnalyzer.estimateThroughput(records);
const rttAndServerResponseTime = NetworkAnalysis.computeRTTAndServerResponseTime(records);
rttAndServerResponseTime.throughput = throughput * 8; // convert from KBps to Kbps
return rttAndServerResponseTime;
return {records, throughput, ...rttAndServerResponseTime};
}
}

Expand Down
74 changes: 0 additions & 74 deletions lighthouse-core/gather/computed/network-throughput.js

This file was deleted.

58 changes: 58 additions & 0 deletions lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,64 @@ class NetworkAnalyzer {
return NetworkAnalyzer.summarize(estimatesByOrigin);
}


/**
* Computes the average throughput for the given records in bits/second.
* Excludes data URI, failed or otherwise incomplete, and cached requests.
* Returns Infinity if there were no analyzable network records.
*
* @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
* @return {number}
*/
static estimateThroughput(networkRecords) {
exterkamp marked this conversation as resolved.
Show resolved Hide resolved
let totalBytes = 0;

// We will measure throughput by summing the total bytes downloaded by the total time spent
// downloading those bytes. We slice up all the network records into start/end boundaries, so
// it's easier to deal with the gaps in downloading.
const timeBoundaries = networkRecords.reduce((boundaries, record) => {
const scheme = record.parsedURL && record.parsedURL.scheme;
// Requests whose bodies didn't come over the network or didn't completely finish will mess
// with the computation, just skip over them.
if (scheme === 'data' || record.failed || !record.finished ||
record.statusCode > 300 || !record.transferSize) {
return boundaries;
}

// 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});
return boundaries;
}, /** @type {Array<{time: number, isStart: boolean}>} */([])).sort((a, b) => a.time - b.time);

if (!timeBoundaries.length) {
return Infinity;
}

let inflight = 0;
let currentStart = 0;
let totalDuration = 0;

timeBoundaries.forEach(boundary => {
if (boundary.isStart) {
if (inflight === 0) {
// We just ended a quiet period, keep track of when the download period started
currentStart = boundary.time;
}
inflight++;
} else {
inflight--;
if (inflight === 0) {
// We just entered a quiet period, update our duration with the time we spent downloading
totalDuration += boundary.time - currentStart;
}
}
});

return totalBytes * 8 / totalDuration;
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, if we only show this in bytes in the end, and all the tests add a * 8, why does this function use bits?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Historically it was measured in bytes (which is why all the tests are now * 8), but because this PR moves the code into network throughput where everything is measured in bits I wanted to make it consistent. Since I was just doing a straight copy with minimal changes and network records use bytes, I thought it was most straightforward to do it at the end but could also do a totalBytes -> totalBits rename and do it as it sums if you think that's clearer :)

Copy link
Member

Choose a reason for hiding this comment

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

I think to me it would be clearer if it collected the totalBits and then return that, but then that's a lot more multiplication for very little reason. It's fine like this if the standard is to return bits, I just thought it was funny to calculate the bits then just do bytes * 8 for the tests.

}

/**
* @param {Array<LH.Artifacts.NetworkRequest>} records
* @return {LH.Artifacts.NetworkRequest}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ function generateArtifacts(records) {
return {
devtoolsLogs: {defaultPass: []},
requestNetworkRecords: () => Promise.resolve(records),
requestNetworkThroughput: () => Promise.resolve(1024),
};
}

Expand Down
96 changes: 0 additions & 96 deletions lighthouse-core/test/gather/computed/network-throughput-test.js

This file was deleted.

Loading