From 671b2afedcbf25fa45527d172a2d99adf8b4f14e Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 17 May 2018 15:33:34 -0700 Subject: [PATCH 1/3] core(network-recorder): handle QUIC requests --- lighthouse-core/lib/network-recorder.js | 10 +- .../test/gather/network-recorder-test.js | 97 +++++++++++++++++++ 2 files changed, 105 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/lib/network-recorder.js b/lighthouse-core/lib/network-recorder.js index 5df4dab88056..be09b5a94dea 100644 --- a/lighthouse-core/lib/network-recorder.js +++ b/lighthouse-core/lib/network-recorder.js @@ -107,9 +107,15 @@ class NetworkRecorder extends EventEmitter { return; } + // QUIC network requests don't always "finish" even when they're done loading data + // Use receivedHeaders instead, see https://github.com/GoogleChrome/lighthouse/issues/5254 + const isQUIC = record._responseHeaders && record._responseHeaders + .find(header => header.name.toLowerCase() === 'alt-svc' && /quic/.test(header.value)); + const receivedHeaders = record._timing && record._timing.receiveHeadersEnd > 0; + // convert the network record timestamp to ms timeBoundaries.push({time: record.startTime * 1000, isStart: true}); - if (record.finished) { + if (record.finished || (isQUIC && receivedHeaders && record.endTime)) { timeBoundaries.push({time: record.endTime * 1000, isStart: false}); } }); @@ -119,7 +125,7 @@ class NetworkRecorder extends EventEmitter { .sort((a, b) => a.time - b.time); let numInflightRequests = 0; - let quietPeriodStart = 0; + let quietPeriodStart = -Infinity; /** @type {Array<{start: number, end: number}>} */ const quietPeriods = []; timeBoundaries.forEach(boundary => { diff --git a/lighthouse-core/test/gather/network-recorder-test.js b/lighthouse-core/test/gather/network-recorder-test.js index 8a0a4b54713a..93508682b4ce 100644 --- a/lighthouse-core/test/gather/network-recorder-test.js +++ b/lighthouse-core/test/gather/network-recorder-test.js @@ -16,4 +16,101 @@ describe('network recorder', function() { const records = NetworkRecorder.recordsFromLogs(devtoolsLogItems); assert.equal(records.length, 76); }); + + describe('#findNetworkQuietPeriods', () => { + function record(data) { + const url = data.url || 'https://example.com'; + const scheme = url.split(':')[0]; + return Object.assign({ + url, + finished: !!data.endTime, + parsedURL: {scheme}, + }, data); + } + + it('should find the 0-quiet periods', () => { + const records = [ + record({startTime: 0, endTime: 1}), + record({startTime: 2, endTime: 3}), + record({startTime: 4, endTime: 5}), + ]; + + const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0); + assert.deepStrictEqual(periods, [ + {start: -Infinity, end: 0}, + {start: 1000, end: 2000}, + {start: 3000, end: 4000}, + {start: 5000, end: Infinity}, + ]); + }); + + it('should find the 2-quiet periods', () => { + const records = [ + record({startTime: 0, endTime: 1.5}), + record({startTime: 0, endTime: 2}), + record({startTime: 0, endTime: 2.5}), + record({startTime: 2, endTime: 3}), + record({startTime: 4, endTime: 5}), + ]; + + const periods = NetworkRecorder.findNetworkQuietPeriods(records, 2); + assert.deepStrictEqual(periods, [ + {start: -Infinity, end: 0}, + {start: 1500, end: Infinity}, + ]); + }); + + it('should handle unfinished requests', () => { + const records = [ + record({startTime: 0, endTime: 1.5}), + record({startTime: 0, endTime: 2}), + record({startTime: 0, endTime: 2.5}), + record({startTime: 2, endTime: 3}), + record({startTime: 2}), + record({startTime: 2}), + record({startTime: 4, endTime: 5}), + record({startTime: 5.5}), + ]; + + const periods = NetworkRecorder.findNetworkQuietPeriods(records, 2); + assert.deepStrictEqual(periods, [ + {start: -Infinity, end: 0}, + {start: 1500, end: 2000}, + {start: 3000, end: 4000}, + {start: 5000, end: 5500}, + ]); + }); + + it('should ignore data URIs', () => { + const records = [ + record({startTime: 0, endTime: 1}), + record({startTime: 0, endTime: 2, url: 'data:image/png;base64,'}), + ]; + + const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0); + assert.deepStrictEqual(periods, [ + {start: -Infinity, end: 0}, + {start: 1000, end: Infinity}, + ]); + }); + + it('should handle QUIC requests', () => { + const quicRequest = { + finished: false, + _responseHeaders: [{name: 'ALT-SVC', value: 'hq=":49288";quic="1,1abadaba,51303334,0"'}], + _timing: {receiveHeadersEnd: 1.28}, + }; + + const records = [ + record({startTime: 0, endTime: 1}), + record({startTime: 0, endTime: 2, ...quicRequest}), + ]; + + const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0); + assert.deepStrictEqual(periods, [ + {start: -Infinity, end: 0}, + {start: 2000, end: Infinity}, + ]); + }); + }); }); From 62115b0bf4202a727aa205ba67ae4381d7669877 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 17 May 2018 15:38:55 -0700 Subject: [PATCH 2/3] fix --- lighthouse-core/gather/computed/metrics/interactive.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/gather/computed/metrics/interactive.js b/lighthouse-core/gather/computed/metrics/interactive.js index 01bf0cfc248f..520d49606c0c 100644 --- a/lighthouse-core/gather/computed/metrics/interactive.js +++ b/lighthouse-core/gather/computed/metrics/interactive.js @@ -39,8 +39,14 @@ class Interactive extends MetricArtifact { // Consider network records that had 4xx/5xx status code as "failed" record.statusCode < 400; }); - return NetworkRecorder.findNetworkQuietPeriods(filteredNetworkRecords, + + const periods = NetworkRecorder.findNetworkQuietPeriods(filteredNetworkRecords, ALLOWED_CONCURRENT_REQUESTS, traceEndTsInMs); + periods.forEach(period => { + if (period.start === -Infinity) period.start = 0; + }); + + return periods; } /** From e753060c6ef9697d35febad96de78bd3f16c2d74 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 18 May 2018 14:45:18 -0700 Subject: [PATCH 3/3] revert -Infinity, separate method --- .../gather/computed/metrics/interactive.js | 8 +----- lighthouse-core/lib/network-recorder.js | 25 ++++++++++++------- .../test/gather/network-recorder-test.js | 5 ---- 3 files changed, 17 insertions(+), 21 deletions(-) diff --git a/lighthouse-core/gather/computed/metrics/interactive.js b/lighthouse-core/gather/computed/metrics/interactive.js index 520d49606c0c..01bf0cfc248f 100644 --- a/lighthouse-core/gather/computed/metrics/interactive.js +++ b/lighthouse-core/gather/computed/metrics/interactive.js @@ -39,14 +39,8 @@ class Interactive extends MetricArtifact { // Consider network records that had 4xx/5xx status code as "failed" record.statusCode < 400; }); - - const periods = NetworkRecorder.findNetworkQuietPeriods(filteredNetworkRecords, + return NetworkRecorder.findNetworkQuietPeriods(filteredNetworkRecords, ALLOWED_CONCURRENT_REQUESTS, traceEndTsInMs); - periods.forEach(period => { - if (period.start === -Infinity) period.start = 0; - }); - - return periods; } /** diff --git a/lighthouse-core/lib/network-recorder.js b/lighthouse-core/lib/network-recorder.js index be09b5a94dea..d37b069fe0a8 100644 --- a/lighthouse-core/lib/network-recorder.js +++ b/lighthouse-core/lib/network-recorder.js @@ -89,6 +89,19 @@ class NetworkRecorder extends EventEmitter { } } + /** + * QUIC network requests don't always "finish" even when they're done loading data, use recievedHeaders + * @see https://github.com/GoogleChrome/lighthouse/issues/5254 + * @param {LH.WebInspector.NetworkRequest} record + * @return {boolean} + */ + static _isQUICAndFinished(record) { + const isQUIC = record._responseHeaders && record._responseHeaders + .some(header => header.name.toLowerCase() === 'alt-svc' && /quic/.test(header.value)); + const receivedHeaders = record._timing && record._timing.receiveHeadersEnd > 0; + return !!(isQUIC && receivedHeaders && record.endTime); + } + /** * Finds all time periods where the number of inflight requests is less than or equal to the * number of allowed concurrent requests. @@ -107,15 +120,9 @@ class NetworkRecorder extends EventEmitter { return; } - // QUIC network requests don't always "finish" even when they're done loading data - // Use receivedHeaders instead, see https://github.com/GoogleChrome/lighthouse/issues/5254 - const isQUIC = record._responseHeaders && record._responseHeaders - .find(header => header.name.toLowerCase() === 'alt-svc' && /quic/.test(header.value)); - const receivedHeaders = record._timing && record._timing.receiveHeadersEnd > 0; - // convert the network record timestamp to ms timeBoundaries.push({time: record.startTime * 1000, isStart: true}); - if (record.finished || (isQUIC && receivedHeaders && record.endTime)) { + if (record.finished || NetworkRecorder._isQUICAndFinished(record)) { timeBoundaries.push({time: record.endTime * 1000, isStart: false}); } }); @@ -125,7 +132,7 @@ class NetworkRecorder extends EventEmitter { .sort((a, b) => a.time - b.time); let numInflightRequests = 0; - let quietPeriodStart = -Infinity; + let quietPeriodStart = 0; /** @type {Array<{start: number, end: number}>} */ const quietPeriods = []; timeBoundaries.forEach(boundary => { @@ -149,7 +156,7 @@ class NetworkRecorder extends EventEmitter { quietPeriods.push({start: quietPeriodStart, end: endTime}); } - return quietPeriods; + return quietPeriods.filter(period => period.start !== period.end); } /** diff --git a/lighthouse-core/test/gather/network-recorder-test.js b/lighthouse-core/test/gather/network-recorder-test.js index 93508682b4ce..2aadd9bb6376 100644 --- a/lighthouse-core/test/gather/network-recorder-test.js +++ b/lighthouse-core/test/gather/network-recorder-test.js @@ -37,7 +37,6 @@ describe('network recorder', function() { const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0); assert.deepStrictEqual(periods, [ - {start: -Infinity, end: 0}, {start: 1000, end: 2000}, {start: 3000, end: 4000}, {start: 5000, end: Infinity}, @@ -55,7 +54,6 @@ describe('network recorder', function() { const periods = NetworkRecorder.findNetworkQuietPeriods(records, 2); assert.deepStrictEqual(periods, [ - {start: -Infinity, end: 0}, {start: 1500, end: Infinity}, ]); }); @@ -74,7 +72,6 @@ describe('network recorder', function() { const periods = NetworkRecorder.findNetworkQuietPeriods(records, 2); assert.deepStrictEqual(periods, [ - {start: -Infinity, end: 0}, {start: 1500, end: 2000}, {start: 3000, end: 4000}, {start: 5000, end: 5500}, @@ -89,7 +86,6 @@ describe('network recorder', function() { const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0); assert.deepStrictEqual(periods, [ - {start: -Infinity, end: 0}, {start: 1000, end: Infinity}, ]); }); @@ -108,7 +104,6 @@ describe('network recorder', function() { const periods = NetworkRecorder.findNetworkQuietPeriods(records, 0); assert.deepStrictEqual(periods, [ - {start: -Infinity, end: 0}, {start: 2000, end: Infinity}, ]); });