From 0cdc88edf1bdd814d23985ef5fb628e759c736eb Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Wed, 27 Apr 2016 17:01:25 -0400 Subject: [PATCH 01/11] qweqwe --- src/scheduler.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/scheduler.js b/src/scheduler.js index 81dbb8f2946c..ae50881f5113 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -16,6 +16,7 @@ */ 'use strict'; +const child_process = require('child_process'); const fs = require('fs'); const log = require('./lib/log.js'); @@ -111,6 +112,12 @@ function saveAssets(tracingData, url) { log('info', 'trace file saved to disk', filename); } +function getNetDepGraph(artifacts) { + child_process.execSync('python scripts/netdep_graph_json.py'); + const depGraphString = fs.readFileSync('dependency-graph.json'); + return JSON.parse(depGraphString); +} + function run(gatherers, options) { const driver = options.driver; const tracingData = {}; @@ -151,12 +158,16 @@ function run(gatherers, options) { {traceContents: tracingData.traceContents}, {frameLoadEvents: tracingData.frameLoadEvents}); - const artifacts = flattenArtifacts(unflattenedArtifacts); + let artifacts = flattenArtifacts(unflattenedArtifacts); - if (options.flags.saveArtifacts) { + if (options.flags.saveArtifacts || options.flags.useNetDepGraph) { saveArtifacts(artifacts); } + if (options.flags.useNetDepGraph) { + artifacts.graph = getNetDepGraph(artifacts); + } + return artifacts; }); } From c4964d810d007f28ed087aa83e5899a09fdd67e9 Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Thu, 28 Apr 2016 09:51:09 -0400 Subject: [PATCH 02/11] WIP overdep --- src/scheduler.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/scheduler.js b/src/scheduler.js index ae50881f5113..e444ac13ebae 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -113,8 +113,11 @@ function saveAssets(tracingData, url) { } function getNetDepGraph(artifacts) { + child_process.execSync('python scripts/process_artifacts.py'); child_process.execSync('python scripts/netdep_graph_json.py'); - const depGraphString = fs.readFileSync('dependency-graph.json'); + // './' + '..' prevents brfs from inlining it + // This way extention can still work + const depGraphString = fs.readFileSync('./' + 'dependency-graph.json'); return JSON.parse(depGraphString); } From 8d5b7c79a9bcf92ce66e92c83178bfedb6847626 Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Fri, 29 Apr 2016 14:04:35 -0400 Subject: [PATCH 03/11] overdep WIP --- .../overdependent-critical-resources.js | 109 ++++++++++++++++++ src/lib/drivers/driver.js | 2 + src/lib/network-recorder.js | 5 + src/lighthouse.js | 1 + src/scheduler.js | 11 +- 5 files changed, 123 insertions(+), 5 deletions(-) create mode 100644 src/audits/performance/overdependent-critical-resources.js diff --git a/src/audits/performance/overdependent-critical-resources.js b/src/audits/performance/overdependent-critical-resources.js new file mode 100644 index 000000000000..7509b47cfd51 --- /dev/null +++ b/src/audits/performance/overdependent-critical-resources.js @@ -0,0 +1,109 @@ +/** + * @license + * Copyright 2016 Google Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +const FMPMetric = require('../../metrics/first-meaningful-paint'); +const Audit = require('../audit'); + +class Node { + constructor(requestId, parent) { + this.children = []; + this.parent = null; + this.requestId = requestId; + } + + setParent(parentNode) { + this.parent = parentNode; + } + + addChild(childNode) { + this.children.push(child); + } +} + +class OverdependentCriticalResources extends Audit { + /** + * @override + */ + static get tags() { + return ['Performance']; + } + + /** + * @override + */ + static get name() { + return 'overdependent-critical-resources'; + } + + /** + * @override + */ + static get description() { + return 'Long chain of critical resources'; + } + + /** + * Audits the page to see if there is a long chain of critical resource + * dependency + * @param {!Artifacts} artifacts The artifacts from the gather phase. + * @return {!AuditResult} The score from the audit, ranging from 0-100. + */ + static audit(artifacts) { + const criticalPriorities = ['VeryHigh', 'High', 'Medium']; + const criticalRequests = artifacts.networkRecords.filter(req => + criticalPriorities.indexOf(req._initialPriority) > -1 + ); + + const graph = artifacts.networkDependencyGraph; + const requestIdToNodes = new Map(); + for (let request of criticalRequests) { + const requestId = request._requestId; + const requestNode = new Node(requestId, null); + requestIdToNodes.set(requestId, requestNode); + } + + for (let edge of graph.edges) { + const fromNode = graph.nodes[edge.__from_node_index]; + const toNode = grpah.nodes[edge.__to_node_index]; + const fromRequestId = fromNode.request.request_id; + const toRequestId = toNode.request.request_id; + + if (requestIdToNodes.has(fromRequestId) && + requestIdToNodes.has(toRequestId)) { + fromRequestNode = requestIdToNodes.get(fromRequestId); + toRequestNode = requestIdToNodes.get(toRequestId); + + fromRequestNode.addChild(toRequestNode); + toRequestNode.addParent(fromRequestNode); + } + } + + + const nodesList = [...requestIdToNodes.values()]; + const parentlessNodes = nodesList.filter(node => node.parent === null); + + console.log("#$#@!$@#$@#$"); + console.log(JSON.stringify(nodesList)); + + return Promise.resolve(OverdependentCriticalResources.generateAuditResult( + 42, 43, "foo")); + } +} + +module.exports = OverdependentCriticalResources; diff --git a/src/lib/drivers/driver.js b/src/lib/drivers/driver.js index 34f95be4791d..fff65cfb2403 100644 --- a/src/lib/drivers/driver.js +++ b/src/lib/drivers/driver.js @@ -208,6 +208,7 @@ class DriverBase { this.on('Network.dataReceived', this._networkRecorder.onDataReceived); this.on('Network.loadingFinished', this._networkRecorder.onLoadingFinished); this.on('Network.loadingFailed', this._networkRecorder.onLoadingFailed); + this.on('Network.resourceChangedPriority', this._networkRecorder.onResourceChangedPriority); this.sendCommand('Network.enable').then(_ => { resolve(); @@ -223,6 +224,7 @@ class DriverBase { this.off('Network.dataReceived', this._networkRecorder.onDataReceived); this.off('Network.loadingFinished', this._networkRecorder.onLoadingFinished); this.off('Network.loadingFailed', this._networkRecorder.onLoadingFailed); + this.off('Network.resourceChangedPriority', this._networkRecorder.onResourceChangedPriority); resolve({ networkRecords: this._networkRecords, diff --git a/src/lib/network-recorder.js b/src/lib/network-recorder.js index fa5beac0fe77..889a56fdf9f8 100644 --- a/src/lib/network-recorder.js +++ b/src/lib/network-recorder.js @@ -38,6 +38,7 @@ class NetworkRecorder { this.onDataReceived = this.onDataReceived.bind(this); this.onLoadingFinished = this.onLoadingFinished.bind(this); this.onLoadingFailed = this.onLoadingFailed.bind(this); + this.onResourceChangedPriority = this.onResourceChangedPriority.bind(this); } // There are a few differences between the debugging protocol naming and @@ -86,6 +87,10 @@ class NetworkRecorder { data.timestamp, data.type, data.errorText, data.canceled, data.blockedReason); } + + onResourceChangedPriority(data) { + this._rawEvents.push({method: 'Network.resourceChangedPriority', params: data}); + } } module.exports = NetworkRecorder; diff --git a/src/lighthouse.js b/src/lighthouse.js index bd7ff456908a..0ec90559f19b 100644 --- a/src/lighthouse.js +++ b/src/lighthouse.js @@ -39,6 +39,7 @@ const audits = [ require('./audits/mobile-friendly/viewport'), require('./audits/mobile-friendly/display'), require('./audits/performance/first-meaningful-paint'), + require('./audits/performance/overdependent-critical-resources'), require('./audits/manifest/exists'), require('./audits/manifest/background-color'), require('./audits/manifest/theme-color'), diff --git a/src/scheduler.js b/src/scheduler.js index e444ac13ebae..6a5c70048baf 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -112,13 +112,14 @@ function saveAssets(tracingData, url) { log('info', 'trace file saved to disk', filename); } -function getNetDepGraph(artifacts) { +function getNetworkDependencyGraph(artifacts) { child_process.execSync('python scripts/process_artifacts.py'); child_process.execSync('python scripts/netdep_graph_json.py'); - // './' + '..' prevents brfs from inlining it - // This way extention can still work + // './' + '..' prevents brfs from inlining the generated file + // This way extention can still compile + // The extension never hits this code path since is guarded by cli flag const depGraphString = fs.readFileSync('./' + 'dependency-graph.json'); - return JSON.parse(depGraphString); + return JSON.parse(depGraphString).graph; } function run(gatherers, options) { @@ -168,7 +169,7 @@ function run(gatherers, options) { } if (options.flags.useNetDepGraph) { - artifacts.graph = getNetDepGraph(artifacts); + artifacts.networkDependencyGraph = getNetworkDependencyGraph(artifacts); } return artifacts; From f01db280d3cc9a51eb2daab99bcef03b974665e0 Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Wed, 4 May 2016 15:29:30 -0400 Subject: [PATCH 04/11] Add audit and tests --- scripts/process_artifacts.py | 3 +- .../overdependent-critical-resources.js | 109 --------------- .../overdependent-critical-resources.js | 129 ++++++++++++++++++ 3 files changed, 131 insertions(+), 110 deletions(-) delete mode 100644 src/audits/performance/overdependent-critical-resources.js create mode 100644 test/src/audits/performance/overdependent-critical-resources.js diff --git a/scripts/process_artifacts.py b/scripts/process_artifacts.py index fac7ab869279..ed5898c69301 100644 --- a/scripts/process_artifacts.py +++ b/scripts/process_artifacts.py @@ -42,7 +42,8 @@ def create_page_track(frame_load_events): def create_request_track(raw_network_events): request_track = RequestTrack(None) for event in raw_network_events: - request_track.Handle(event['method'], event) + if event['method'] in RequestTrack._METHOD_TO_HANDLER: + request_track.Handle(event['method'], event) return request_track.ToJsonDict() def main(): diff --git a/src/audits/performance/overdependent-critical-resources.js b/src/audits/performance/overdependent-critical-resources.js deleted file mode 100644 index 7509b47cfd51..000000000000 --- a/src/audits/performance/overdependent-critical-resources.js +++ /dev/null @@ -1,109 +0,0 @@ -/** - * @license - * Copyright 2016 Google Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -'use strict'; - -const FMPMetric = require('../../metrics/first-meaningful-paint'); -const Audit = require('../audit'); - -class Node { - constructor(requestId, parent) { - this.children = []; - this.parent = null; - this.requestId = requestId; - } - - setParent(parentNode) { - this.parent = parentNode; - } - - addChild(childNode) { - this.children.push(child); - } -} - -class OverdependentCriticalResources extends Audit { - /** - * @override - */ - static get tags() { - return ['Performance']; - } - - /** - * @override - */ - static get name() { - return 'overdependent-critical-resources'; - } - - /** - * @override - */ - static get description() { - return 'Long chain of critical resources'; - } - - /** - * Audits the page to see if there is a long chain of critical resource - * dependency - * @param {!Artifacts} artifacts The artifacts from the gather phase. - * @return {!AuditResult} The score from the audit, ranging from 0-100. - */ - static audit(artifacts) { - const criticalPriorities = ['VeryHigh', 'High', 'Medium']; - const criticalRequests = artifacts.networkRecords.filter(req => - criticalPriorities.indexOf(req._initialPriority) > -1 - ); - - const graph = artifacts.networkDependencyGraph; - const requestIdToNodes = new Map(); - for (let request of criticalRequests) { - const requestId = request._requestId; - const requestNode = new Node(requestId, null); - requestIdToNodes.set(requestId, requestNode); - } - - for (let edge of graph.edges) { - const fromNode = graph.nodes[edge.__from_node_index]; - const toNode = grpah.nodes[edge.__to_node_index]; - const fromRequestId = fromNode.request.request_id; - const toRequestId = toNode.request.request_id; - - if (requestIdToNodes.has(fromRequestId) && - requestIdToNodes.has(toRequestId)) { - fromRequestNode = requestIdToNodes.get(fromRequestId); - toRequestNode = requestIdToNodes.get(toRequestId); - - fromRequestNode.addChild(toRequestNode); - toRequestNode.addParent(fromRequestNode); - } - } - - - const nodesList = [...requestIdToNodes.values()]; - const parentlessNodes = nodesList.filter(node => node.parent === null); - - console.log("#$#@!$@#$@#$"); - console.log(JSON.stringify(nodesList)); - - return Promise.resolve(OverdependentCriticalResources.generateAuditResult( - 42, 43, "foo")); - } -} - -module.exports = OverdependentCriticalResources; diff --git a/test/src/audits/performance/overdependent-critical-resources.js b/test/src/audits/performance/overdependent-critical-resources.js new file mode 100644 index 000000000000..35a0e74bb282 --- /dev/null +++ b/test/src/audits/performance/overdependent-critical-resources.js @@ -0,0 +1,129 @@ +/** + * Copyright 2016 Google Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +'use strict'; + +const Audit = require('../../../../src/audits/performance/overdependent-critical-resources'); +const assert = require('assert'); +const artifacts = require('./ocr-artifacts.json'); + +function generateTestArtifacts(prioritiesList, edges) { + const networkRecords = prioritiesList.map((priority, index) => + ({_requestId: index, _initialPriority: priority})); + + const nodes = networkRecords.map(record => + ({request: {request_id: record._requestId}})); + + const artifactEdges = edges.map(edge => + ({__from_node_index: edge[0], __to_node_index: edge[1]})); + + return { + networkRecords: networkRecords, + networkDependencyGraph: { + nodes: nodes, + edges: artifactEdges + } + }; +} + +function testAudit(data) { + const artifacts = generateTestArtifacts(data.priorityList, data.edges); + return Audit.audit(artifacts) + .then(response => response.rawValue) + .then(chains => chains.map(chain => chain.map(node => node.requestId))) + .then(requestIdChains => + assert.deepEqual( + new Set(requestIdChains), new Set(data.expectedChains))); +} + +const HIGH = 'High'; +const VERY_HIGH = 'VeryHigh'; +const MEDIUM = 'Medium'; +const LOW = 'Low'; +const VERY_Low = 'VeryLow'; + +/* global describe, it*/ +describe('Performance: overdependent-critical-resources audit', () => { + it('returns correct data for chain of four critical requests', () => + testAudit({ + priorityList: [HIGH, MEDIUM, VERY_HIGH, HIGH], + edges: [[0, 1], [1, 2], [2, 3]], + expectedChains: [[0, 1, 2, 3]] + })); + + it('returns correct data for chain interleaved with non-critical requests', + () => testAudit({ + priorityList: [MEDIUM, HIGH, LOW, MEDIUM, HIGH], + edges: [[0, 1], [1, 2], [2, 3], [3, 4]], + expectedChains: [[0, 1], [3, 4]] + })); + + it('returns correct data for two parallel chains', () => + testAudit({ + priorityList: [HIGH, HIGH, HIGH, HIGH], + edges: [[0, 2], [1, 3]], + expectedChains: [[1, 3], [0, 2]] + })); + + it('returns correct data for two parallel chains', () => + testAudit({ + priorityList: [HIGH, HIGH, HIGH, HIGH], + edges: [[0, 2], [1, 3]], + expectedChains: [[0, 2], [1, 3]] + })); + + it('return correct data for fork at root', () => + testAudit({ + priorityList: [HIGH, HIGH, HIGH], + edges: [[0, 1], [0, 2]], + expectedChains: [[0, 1], [0, 2]] + })) + + it('return correct data for fork at non root', () => + testAudit({ + priorityList: [HIGH, HIGH, HIGH, HIGH], + edges: [[0, 1], [1, 2], [1, 3]], + expectedChains: [[0, 1, 2], [0, 1, 3]] + })); + + it('returns empty chain list when no critical request', () => + testAudit({ + priorityList: [LOW, LOW], + edges: [[0, 1]], + expectedChains: [] + })); + + it('returns empty chain list when no request whatsoever', () => + testAudit({ + priorityList: [], + edges: [], + expectedChains: [] + })); + + it('returns two single node chains for two independent requests', () => + testAudit({ + priorityList: [HIGH, HIGH], + edges: [], + expectedChains: [[0], [1]] + })); + + it('returns correct data on a random kinda big graph', () => + testAudit({ + priorityList: Array(9).fill(HIGH), + edges: [[0, 1], [1, 2], [1, 3], [4, 5], [5,7], [7, 8], [5, 6]], + expectedChains: [ + [0, 1, 2], [0, 1, 3], [4, 5, 7, 8], [4, 5, 6] + ]})); +}); From d742c05850d59b92dd0034b704d197628f47af1b Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Wed, 4 May 2016 15:36:50 -0400 Subject: [PATCH 05/11] Actually add audit file. Remove old require --- .../overdependent-critical-resources.js | 126 ++++++++++++++++++ .../overdependent-critical-resources.js | 5 +- 2 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 src/audits/performance/overdependent-critical-resources.js diff --git a/src/audits/performance/overdependent-critical-resources.js b/src/audits/performance/overdependent-critical-resources.js new file mode 100644 index 000000000000..50d122f7c9ec --- /dev/null +++ b/src/audits/performance/overdependent-critical-resources.js @@ -0,0 +1,126 @@ +/** + * @license + * Copyright 2016 Google Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +const FMPMetric = require('../../metrics/first-meaningful-paint'); +const Audit = require('../audit'); + +const flatten = arr => arr.reduce((a, b) => a.concat(b), []); + +class Node { + constructor(requestId, parent) { + this.children = []; + this.parent = null; + this.requestId = requestId; + } + + setParent(parentNode) { + this.parent = parentNode; + } + + addChild(childNode) { + this.children.push(childNode); + } + + toJSON() { + return "{requestId: " + this.requestId + ", parent: " + (this.parent && this.parent.requestId) + "}"; + } +} + +class OverdependentCriticalResources extends Audit { + /** + * @override + */ + static get tags() { + return ['Performance']; + } + + /** + * @override + */ + static get name() { + return 'overdependent-critical-resources'; + } + + /** + * @override + */ + static get description() { + return 'Long chain of critical resources'; + } + + static _getChains(startNode) { + if (startNode.children.length === 0) { + return [[startNode]]; + } else { + const childrenChains = flatten(startNode.children.map(child => + OverdependentCriticalResources._getChains(child))); + return childrenChains.map(chain => [startNode].concat(chain)); + } + } + + /** + * Audits the page to see if there is a long chain of critical resource + * dependency + * @param {!Artifacts} artifacts The artifacts from the gather phase. + * @return {!AuditResult} The score from the audit, ranging from 0-100. + */ + static audit(artifacts) { + const criticalPriorities = ['VeryHigh', 'High', 'Medium']; + const criticalRequests = artifacts.networkRecords.filter(req => + criticalPriorities.indexOf(req._initialPriority) > -1 + ); + + const graph = artifacts.networkDependencyGraph; + const requestIdToNodes = new Map(); + for (let request of criticalRequests) { + const requestId = request._requestId; + const requestNode = new Node(requestId, null); + requestIdToNodes.set(requestId, requestNode); + } + + for (let edge of graph.edges) { + const fromNode = graph.nodes[edge.__from_node_index]; + const toNode = graph.nodes[edge.__to_node_index]; + const fromRequestId = fromNode.request.request_id; + const toRequestId = toNode.request.request_id; + + if (requestIdToNodes.has(fromRequestId) && + requestIdToNodes.has(toRequestId)) { + const fromRequestNode = requestIdToNodes.get(fromRequestId); + const toRequestNode = requestIdToNodes.get(toRequestId); + + fromRequestNode.addChild(toRequestNode); + toRequestNode.setParent(fromRequestNode); + } + } + + const nodesList = [...requestIdToNodes.values()]; + const orphanNodes = nodesList.filter(node => node.parent === null); + const chains = flatten(orphanNodes.map(node => + OverdependentCriticalResources._getChains(node))); + + const maxChainLength = Math.max(0, ...chains.map(chain => chain.length)); + + // Returning random things + the chain. DO NOT SHIP. + return Promise.resolve(OverdependentCriticalResources.generateAuditResult( + maxChainLength, chains, "foo bar")); + } +} + +module.exports = OverdependentCriticalResources; diff --git a/test/src/audits/performance/overdependent-critical-resources.js b/test/src/audits/performance/overdependent-critical-resources.js index 35a0e74bb282..e4101a6f87aa 100644 --- a/test/src/audits/performance/overdependent-critical-resources.js +++ b/test/src/audits/performance/overdependent-critical-resources.js @@ -17,7 +17,6 @@ const Audit = require('../../../../src/audits/performance/overdependent-critical-resources'); const assert = require('assert'); -const artifacts = require('./ocr-artifacts.json'); function generateTestArtifacts(prioritiesList, edges) { const networkRecords = prioritiesList.map((priority, index) => @@ -84,14 +83,14 @@ describe('Performance: overdependent-critical-resources audit', () => { expectedChains: [[0, 2], [1, 3]] })); - it('return correct data for fork at root', () => + it('returns correct data for fork at root', () => testAudit({ priorityList: [HIGH, HIGH, HIGH], edges: [[0, 1], [0, 2]], expectedChains: [[0, 1], [0, 2]] })) - it('return correct data for fork at non root', () => + it('returns correct data for fork at non root', () => testAudit({ priorityList: [HIGH, HIGH, HIGH, HIGH], edges: [[0, 1], [1, 2], [1, 3]], From ad660dacba5c4d644aede2250c4990d5d4056803 Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Thu, 5 May 2016 20:09:30 -0400 Subject: [PATCH 06/11] criticial chain more WIP --- .../critical-network-request-chains.js} | 52 ++++++------------- .../overdependent-critical-resources.js | 2 +- 2 files changed, 18 insertions(+), 36 deletions(-) rename src/{audits/performance/overdependent-critical-resources.js => gatherers/critical-network-request-chains.js} (71%) diff --git a/src/audits/performance/overdependent-critical-resources.js b/src/gatherers/critical-network-request-chains.js similarity index 71% rename from src/audits/performance/overdependent-critical-resources.js rename to src/gatherers/critical-network-request-chains.js index 50d122f7c9ec..6526a6732e01 100644 --- a/src/audits/performance/overdependent-critical-resources.js +++ b/src/gatherers/critical-network-request-chains.js @@ -17,10 +17,10 @@ 'use strict'; -const FMPMetric = require('../../metrics/first-meaningful-paint'); -const Audit = require('../audit'); +const Gather = require('../gather'); const flatten = arr => arr.reduce((a, b) => a.concat(b), []); +const contains = (arr, elm) => arr.indexOf(elm) > -1; class Node { constructor(requestId, parent) { @@ -42,50 +42,32 @@ class Node { } } -class OverdependentCriticalResources extends Audit { - /** - * @override - */ - static get tags() { - return ['Performance']; - } - - /** - * @override - */ - static get name() { - return 'overdependent-critical-resources'; - } - - /** - * @override - */ - static get description() { - return 'Long chain of critical resources'; - } +class CriticalNetworkRequestChains extends Gather { static _getChains(startNode) { + // DFS-ish to get chains if (startNode.children.length === 0) { return [[startNode]]; } else { const childrenChains = flatten(startNode.children.map(child => - OverdependentCriticalResources._getChains(child))); + CriticalNetworkRequestChains._getChains(child))); return childrenChains.map(chain => [startNode].concat(chain)); } } + get criticalPriorities() { + return ['VeryHigh', 'High', 'Medium']; + } + /** * Audits the page to see if there is a long chain of critical resource * dependency - * @param {!Artifacts} artifacts The artifacts from the gather phase. - * @return {!AuditResult} The score from the audit, ranging from 0-100. */ - static audit(artifacts) { - const criticalPriorities = ['VeryHigh', 'High', 'Medium']; + static postProfiling() { const criticalRequests = artifacts.networkRecords.filter(req => - criticalPriorities.indexOf(req._initialPriority) > -1 - ); + contains(criticalPriorities, req)); + // Build a map of requestID -> Node. const graph = artifacts.networkDependencyGraph; const requestIdToNodes = new Map(); for (let request of criticalRequests) { @@ -94,6 +76,7 @@ class OverdependentCriticalResources extends Audit { requestIdToNodes.set(requestId, requestNode); } + // Connect the parents and children. for (let edge of graph.edges) { const fromNode = graph.nodes[edge.__from_node_index]; const toNode = graph.nodes[edge.__to_node_index]; @@ -113,14 +96,13 @@ class OverdependentCriticalResources extends Audit { const nodesList = [...requestIdToNodes.values()]; const orphanNodes = nodesList.filter(node => node.parent === null); const chains = flatten(orphanNodes.map(node => - OverdependentCriticalResources._getChains(node))); + CriticalNetworkRequestChains._getChains(node))); const maxChainLength = Math.max(0, ...chains.map(chain => chain.length)); - // Returning random things + the chain. DO NOT SHIP. - return Promise.resolve(OverdependentCriticalResources.generateAuditResult( - maxChainLength, chains, "foo bar")); + return Promise.resolve({CriticalNetworkRequestChains: chains}); + })); } } -module.exports = OverdependentCriticalResources; +module.exports = CriticalNetworkRequestChains; diff --git a/test/src/audits/performance/overdependent-critical-resources.js b/test/src/audits/performance/overdependent-critical-resources.js index e4101a6f87aa..793d2fe4ec59 100644 --- a/test/src/audits/performance/overdependent-critical-resources.js +++ b/test/src/audits/performance/overdependent-critical-resources.js @@ -118,7 +118,7 @@ describe('Performance: overdependent-critical-resources audit', () => { expectedChains: [[0], [1]] })); - it('returns correct data on a random kinda big graph', () => + it('returns correct data on a random big graph', () => testAudit({ priorityList: Array(9).fill(HIGH), edges: [[0, 1], [1, 2], [1, 3], [4, 5], [5,7], [7, 8], [5, 6]], From 123d133a40aa01ef66eb1afe18d5cea5dd2dceb6 Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Fri, 6 May 2016 09:47:45 -0400 Subject: [PATCH 07/11] even more WIP --- scripts/process_artifacts.py | 2 +- src/gatherers/critical-network-chains.js | 156 ++++++++++++++++++ .../critical-network-request-chains.js | 108 ------------ src/lighthouse.js | 5 +- src/scheduler.js | 19 +-- 5 files changed, 163 insertions(+), 127 deletions(-) create mode 100644 src/gatherers/critical-network-chains.js delete mode 100644 src/gatherers/critical-network-request-chains.js diff --git a/scripts/process_artifacts.py b/scripts/process_artifacts.py index ed5898c69301..7a37f4cb3bf4 100644 --- a/scripts/process_artifacts.py +++ b/scripts/process_artifacts.py @@ -47,7 +47,7 @@ def create_request_track(raw_network_events): return request_track.ToJsonDict() def main(): - with open('artifacts.log', 'r') as f: + with open('clovisData.json', 'r') as f: artifacts = json.load(f) clovis_trace = {} diff --git a/src/gatherers/critical-network-chains.js b/src/gatherers/critical-network-chains.js new file mode 100644 index 000000000000..c1f7fa0c88ea --- /dev/null +++ b/src/gatherers/critical-network-chains.js @@ -0,0 +1,156 @@ +/** + * @license + * Copyright 2016 Google Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +const Gather = require('./gather'); +const child_process = require('child_process'); +const fs = require('fs'); +const log = require('../lib/log.js'); + +const flatten = arr => arr.reduce((a, b) => a.concat(b), []); +const contains = (arr, elm) => arr.indexOf(elm) > -1; + +class Node { + get requestId() { + return this.request._requestId; + } + constructor(request, parent) { + this.children = []; + this.parent = null; + this.request = request; + } + + setParent(parentNode) { + this.parent = parentNode; + } + + addChild(childNode) { + this.children.push(childNode); + } + + toJSON() { + return "{requestId: " + this.requestId + ", parent: " + (this.parent && this.parent.requestId) + "}"; + } +} + +class CriticalNetworkChains extends Gather { + + get criticalPriorities() { + return ['VeryHigh', 'High', 'Medium']; + } + + postProfiling(options, tracingData) { + const graph = this._getNetworkDependencyGraph(options.url, tracingData); + const chains = this.getCriticalChains(tracingData.networkRecords, graph); + + // There logs are here so we can test this gatherer + // Will be removed when we can a way to surface them in the report + const urlChains = chains.map(chain => chain.map(request => request._url)); + const debuggingData = chains.map(chain => ({ + urls: chain.map(request => request._url), + totalRequests: chain.length, + times: chain.map(request => ({ + startTime: request._startTime, + endTime: request._endTime, + responseReceivedTime: request.responseReceivedTime + })), + // TODO: what happens with long polling? is endTime infinite? + totalTimeSpent: chain.reduce( + (sum, req) => sum + (req._endTime - req._startTime), 0) + })); + log.log('info', JSON.stringify(debuggingData)); + return {CriticalNetworkChains: chains}; + } + + getCriticalChains(networkRecords, graph) { + // TODO: Should we also throw out requests after DOMContentLoaded? + const criticalRequests = networkRecords.filter( + req => contains(this.criticalPriorities, req._initialPriority)); + + // Build a map of requestID -> Node. + const requestIdToNodes = new Map(); + for (let request of criticalRequests) { + const requestNode = new Node(request, null); + requestIdToNodes.set(requestNode.requestId, requestNode); + } + + // Connect the parents and children. + for (let edge of graph.edges) { + const fromNode = graph.nodes[edge.__from_node_index]; + const toNode = graph.nodes[edge.__to_node_index]; + const fromRequestId = fromNode.request.request_id; + const toRequestId = toNode.request.request_id; + + if (requestIdToNodes.has(fromRequestId) && + requestIdToNodes.has(toRequestId)) { + const fromRequestNode = requestIdToNodes.get(fromRequestId); + const toRequestNode = requestIdToNodes.get(toRequestId); + + fromRequestNode.addChild(toRequestNode); + toRequestNode.setParent(fromRequestNode); + } + } + + const nodesList = [...requestIdToNodes.values()]; + const orphanNodes = nodesList.filter(node => node.parent === null); + const nodeChains = flatten(orphanNodes.map( + node => this._getChainsDFS(node))); + const requestChains = nodeChains.map(chain => chain.map( + node => node.request)); + return requestChains; + } + + _getChainsDFS(startNode) { + if (startNode.children.length === 0) { + return [[startNode]]; + } else { + const childrenChains = flatten(startNode.children.map(child => + this._getChainsDFS(child))); + return childrenChains.map(chain => [startNode].concat(chain)); + } + } + + _saveClovisData(url, tracingData, filename) { + const clovisData = { + url: url, + traceContents: tracingData.traceContents, + frameLoadEvents: tracingData.frameLoadEvents, + rawNetworkEvents: tracingData.rawNetworkEvents + }; + fs.writeFileSync(filename, JSON.stringify(clovisData)); + } + + _getNetworkDependencyGraph(url, tracingData) { + const clovisDataFilename = "clovisData.json"; + const clovisGraphFilename = "dependency-graph.json"; + + // These will go away once we implement initiator graph ourselves + this._saveClovisData(url, tracingData, clovisDataFilename); + child_process.execSync('python scripts/process_artifacts.py'); + child_process.execSync('python scripts/netdep_graph_json.py'); + const depGraphString = fs.readFileSync(clovisGraphFilename); + + // TODO: make sure non existent files do not bring whole lighthouse down + fs.unlinkSync(clovisDataFilename); + fs.unlinkSync(clovisGraphFilename); + + return JSON.parse(depGraphString).graph; + } +} + +module.exports = CriticalNetworkChains; diff --git a/src/gatherers/critical-network-request-chains.js b/src/gatherers/critical-network-request-chains.js deleted file mode 100644 index 6526a6732e01..000000000000 --- a/src/gatherers/critical-network-request-chains.js +++ /dev/null @@ -1,108 +0,0 @@ -/** - * @license - * Copyright 2016 Google Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -'use strict'; - -const Gather = require('../gather'); - -const flatten = arr => arr.reduce((a, b) => a.concat(b), []); -const contains = (arr, elm) => arr.indexOf(elm) > -1; - -class Node { - constructor(requestId, parent) { - this.children = []; - this.parent = null; - this.requestId = requestId; - } - - setParent(parentNode) { - this.parent = parentNode; - } - - addChild(childNode) { - this.children.push(childNode); - } - - toJSON() { - return "{requestId: " + this.requestId + ", parent: " + (this.parent && this.parent.requestId) + "}"; - } -} - -class CriticalNetworkRequestChains extends Gather { - - static _getChains(startNode) { - // DFS-ish to get chains - if (startNode.children.length === 0) { - return [[startNode]]; - } else { - const childrenChains = flatten(startNode.children.map(child => - CriticalNetworkRequestChains._getChains(child))); - return childrenChains.map(chain => [startNode].concat(chain)); - } - } - - get criticalPriorities() { - return ['VeryHigh', 'High', 'Medium']; - } - - /** - * Audits the page to see if there is a long chain of critical resource - * dependency - */ - static postProfiling() { - const criticalRequests = artifacts.networkRecords.filter(req => - contains(criticalPriorities, req)); - - // Build a map of requestID -> Node. - const graph = artifacts.networkDependencyGraph; - const requestIdToNodes = new Map(); - for (let request of criticalRequests) { - const requestId = request._requestId; - const requestNode = new Node(requestId, null); - requestIdToNodes.set(requestId, requestNode); - } - - // Connect the parents and children. - for (let edge of graph.edges) { - const fromNode = graph.nodes[edge.__from_node_index]; - const toNode = graph.nodes[edge.__to_node_index]; - const fromRequestId = fromNode.request.request_id; - const toRequestId = toNode.request.request_id; - - if (requestIdToNodes.has(fromRequestId) && - requestIdToNodes.has(toRequestId)) { - const fromRequestNode = requestIdToNodes.get(fromRequestId); - const toRequestNode = requestIdToNodes.get(toRequestId); - - fromRequestNode.addChild(toRequestNode); - toRequestNode.setParent(fromRequestNode); - } - } - - const nodesList = [...requestIdToNodes.values()]; - const orphanNodes = nodesList.filter(node => node.parent === null); - const chains = flatten(orphanNodes.map(node => - CriticalNetworkRequestChains._getChains(node))); - - const maxChainLength = Math.max(0, ...chains.map(chain => chain.length)); - - return Promise.resolve({CriticalNetworkRequestChains: chains}); - })); - } -} - -module.exports = CriticalNetworkRequestChains; diff --git a/src/lighthouse.js b/src/lighthouse.js index 5ca9b5ac5fc2..90b517144b29 100644 --- a/src/lighthouse.js +++ b/src/lighthouse.js @@ -41,7 +41,6 @@ const audits = [ require('./audits/mobile-friendly/viewport'), require('./audits/mobile-friendly/display'), require('./audits/performance/first-meaningful-paint'), - require('./audits/performance/overdependent-critical-resources'), require('./audits/performance/speed-index-metric'), require('./audits/manifest/exists'), require('./audits/manifest/background-color'), @@ -79,6 +78,10 @@ module.exports = function(driver, opts) { } const gatherers = gathererClasses.map(G => new G()); + if (opts.flags.useNetDepGraph) { + const criticalChainClass = require('./gatherers/critical-network-chains'); + gatherers.push(new criticalChainClass()); + } return Scheduler .run(gatherers, Object.assign({}, opts, {driver})) diff --git a/src/scheduler.js b/src/scheduler.js index 5ce1ef704b67..aeebe1dfde2e 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -16,9 +16,7 @@ */ 'use strict'; -const child_process = require('child_process'); const fs = require('fs'); - const log = require('./lib/log.js'); function loadPage(driver, gatherers, options) { @@ -112,20 +110,11 @@ function saveAssets(tracingData, url) { log.log('info', 'trace file saved to disk', filename); } -function getNetworkDependencyGraph(artifacts) { - child_process.execSync('python scripts/process_artifacts.py'); - child_process.execSync('python scripts/netdep_graph_json.py'); - // './' + '..' prevents brfs from inlining the generated file - // This way extention can still compile - // The extension never hits this code path since is guarded by cli flag - const depGraphString = fs.readFileSync('./' + 'dependency-graph.json'); - return JSON.parse(depGraphString).graph; -} - function run(gatherers, options) { const driver = options.driver; const tracingData = {}; + if (options.url === undefined || options.url === null) { throw new Error('You must provide a url to scheduler'); } @@ -168,14 +157,10 @@ function run(gatherers, options) { let artifacts = flattenArtifacts(unflattenedArtifacts); - if (options.flags.saveArtifacts || options.flags.useNetDepGraph) { + if (options.flags.saveArtifacts) { saveArtifacts(artifacts); } - if (options.flags.useNetDepGraph) { - artifacts.networkDependencyGraph = getNetworkDependencyGraph(artifacts); - } - return artifacts; }); } From b3c7e46ff26f1522d35fd957d63ab6a02c6c41df Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Fri, 6 May 2016 23:03:02 -0400 Subject: [PATCH 08/11] critical chains: more more WIP --- scripts/netdep_graph_json.py | 9 +++- scripts/process_artifacts.py | 10 +++- src/gatherers/critical-network-chains.js | 34 +++++++++---- src/lib/frame-load-recorder.js | 5 +- src/lighthouse.js | 1 + .../critical-network-chains.js} | 51 ++++++++++--------- 6 files changed, 71 insertions(+), 39 deletions(-) rename test/src/{audits/performance/overdependent-critical-resources.js => gatherers/critical-network-chains.js} (73%) diff --git a/scripts/netdep_graph_json.py b/scripts/netdep_graph_json.py index 1dc3ce285c8e..84c15ea16c68 100644 --- a/scripts/netdep_graph_json.py +++ b/scripts/netdep_graph_json.py @@ -14,6 +14,7 @@ import os import sys +import pdb, traceback, sys if 'CHROMIUM_SOURCE_PATH' not in os.environ: print 'You must set the environment variable CHROMIUM_SOURCE_PATH' @@ -44,7 +45,13 @@ def get_network_dependency_graph(json_dict): def main(): with open('clovis-trace.log') as f: - graph = get_network_dependency_graph(json.load(f)) + try: + graph = get_network_dependency_graph(json.load(f)) + except: + type, value, tb = sys.exc_info() + traceback.print_exc() + pdb.post_mortem(tb) + output_file = "dependency-graph.json" with open(output_file, 'w') as f: diff --git a/scripts/process_artifacts.py b/scripts/process_artifacts.py index 7a37f4cb3bf4..afe163e49f9d 100644 --- a/scripts/process_artifacts.py +++ b/scripts/process_artifacts.py @@ -35,8 +35,14 @@ def create_tracing_track(trace_events): or event['cat'] == '__metadata']} def create_page_track(frame_load_events): - events = [{'frame_id': e['frameId'], 'method': e['method']} - for e in frame_load_events] + events = [] + for e in frame_load_events: + clovis_event = { + 'frame_id': e['frameId'], + 'method': e['method'], + 'parent_frame_id': e.get('parentFrameId', None) + } + events.append(clovis_event) return {'events': events} def create_request_track(raw_network_events): diff --git a/src/gatherers/critical-network-chains.js b/src/gatherers/critical-network-chains.js index c1f7fa0c88ea..6af15db1ae58 100644 --- a/src/gatherers/critical-network-chains.js +++ b/src/gatherers/critical-network-chains.js @@ -59,21 +59,29 @@ class CriticalNetworkChains extends Gather { const chains = this.getCriticalChains(tracingData.networkRecords, graph); // There logs are here so we can test this gatherer - // Will be removed when we can a way to surface them in the report + // Will be removed when we have a way to surface them in the report const urlChains = chains.map(chain => chain.map(request => request._url)); - const debuggingData = chains.map(chain => ({ + const nonTrivialChains = chains.filter(chain => chain.length > 1); + + // Note: Approximately, + // startTime: time when request was dispatched + // responseReceivedTime: either time to first byte, or time of receiving + // the end of response headers + // endTime: time when response loading finished + const debuggingData = nonTrivialChains.map(chain => ({ urls: chain.map(request => request._url), totalRequests: chain.length, times: chain.map(request => ({ startTime: request._startTime, endTime: request._endTime, - responseReceivedTime: request.responseReceivedTime + responseReceivedTime: request._responseReceivedTime })), - // TODO: what happens with long polling? is endTime infinite? - totalTimeSpent: chain.reduce( - (sum, req) => sum + (req._endTime - req._startTime), 0) + totalTimeBetweenBeginAndEnd: + (chain[chain.length - 1]._endTime - chain[0]._startTime), + totalLoadingTime: chain.reduce((acc, req) => + acc + (req._endTime - req._responseReceivedTime), 0) })); - log.log('info', JSON.stringify(debuggingData)); + log.log('info', 'cricitalChains', JSON.stringify(debuggingData)); return {CriticalNetworkChains: chains}; } @@ -142,12 +150,16 @@ class CriticalNetworkChains extends Gather { // These will go away once we implement initiator graph ourselves this._saveClovisData(url, tracingData, clovisDataFilename); child_process.execSync('python scripts/process_artifacts.py'); - child_process.execSync('python scripts/netdep_graph_json.py'); + child_process.execSync('python scripts/netdep_graph_json.py', {stdio: [0, 1, 2]}); const depGraphString = fs.readFileSync(clovisGraphFilename); - // TODO: make sure non existent files do not bring whole lighthouse down - fs.unlinkSync(clovisDataFilename); - fs.unlinkSync(clovisGraphFilename); + try { + fs.unlinkSync(clovisDataFilename); + fs.unlinkSync(clovisGraphFilename); + } catch(e) { + console.error(e); + // Should not halt lighthouse for a file delete error + } return JSON.parse(depGraphString).graph; } diff --git a/src/lib/frame-load-recorder.js b/src/lib/frame-load-recorder.js index eaae8e071f68..1f7bbaab21b1 100644 --- a/src/lib/frame-load-recorder.js +++ b/src/lib/frame-load-recorder.js @@ -38,7 +38,10 @@ class FrameLoadRecorder { } onFrameAttached(data) { - this._events.push({frameId: data.frameId, method: 'Page.frameAttached'}); + this._events.push({ + frameId: data.frameId, + method: 'Page.frameAttached', + parentFrameId: data.parentFrameId}); } } diff --git a/src/lighthouse.js b/src/lighthouse.js index 90b517144b29..d7bfce6efdb5 100644 --- a/src/lighthouse.js +++ b/src/lighthouse.js @@ -78,6 +78,7 @@ module.exports = function(driver, opts) { } const gatherers = gathererClasses.map(G => new G()); + if (opts.flags.useNetDepGraph) { const criticalChainClass = require('./gatherers/critical-network-chains'); gatherers.push(new criticalChainClass()); diff --git a/test/src/audits/performance/overdependent-critical-resources.js b/test/src/gatherers/critical-network-chains.js similarity index 73% rename from test/src/audits/performance/overdependent-critical-resources.js rename to test/src/gatherers/critical-network-chains.js index 793d2fe4ec59..c776622ff19a 100644 --- a/test/src/audits/performance/overdependent-critical-resources.js +++ b/test/src/gatherers/critical-network-chains.js @@ -15,36 +15,39 @@ */ 'use strict'; -const Audit = require('../../../../src/audits/performance/overdependent-critical-resources'); +const GathererClass = require('../../../src/gatherers/critical-network-chains'); const assert = require('assert'); -function generateTestArtifacts(prioritiesList, edges) { +const Gatherer = new GathererClass(); + +function mockTracingData(prioritiesList, edges) { const networkRecords = prioritiesList.map((priority, index) => ({_requestId: index, _initialPriority: priority})); const nodes = networkRecords.map(record => ({request: {request_id: record._requestId}})); - const artifactEdges = edges.map(edge => + const graphEdges = edges.map(edge => ({__from_node_index: edge[0], __to_node_index: edge[1]})); return { networkRecords: networkRecords, - networkDependencyGraph: { + graph: { nodes: nodes, - edges: artifactEdges + edges: graphEdges } }; } -function testAudit(data) { - const artifacts = generateTestArtifacts(data.priorityList, data.edges); - return Audit.audit(artifacts) - .then(response => response.rawValue) - .then(chains => chains.map(chain => chain.map(node => node.requestId))) - .then(requestIdChains => - assert.deepEqual( - new Set(requestIdChains), new Set(data.expectedChains))); +function testGetCriticalChain(data) { + const mockData = mockTracingData(data.priorityList, data.edges); + const criticalChains = Gatherer.getCriticalChains( + mockData.networkRecords, mockData.graph); + // It is sufficient to only check the requestIds are correct in the chain + const requestIdChains = criticalChains.map(chain => + chain.map(node => node.requestId)); + // Ordering of the chains do not matter + assert.deepEqual(new Set(requestIdChains), new Set(data.expectedChains)); } const HIGH = 'High'; @@ -54,72 +57,72 @@ const LOW = 'Low'; const VERY_Low = 'VeryLow'; /* global describe, it*/ -describe('Performance: overdependent-critical-resources audit', () => { +describe('CriticalNetworkChain gatherer: getCriticalChain function', () => { it('returns correct data for chain of four critical requests', () => - testAudit({ + testGetCriticalChain({ priorityList: [HIGH, MEDIUM, VERY_HIGH, HIGH], edges: [[0, 1], [1, 2], [2, 3]], expectedChains: [[0, 1, 2, 3]] })); it('returns correct data for chain interleaved with non-critical requests', - () => testAudit({ + () => testGetCriticalChain({ priorityList: [MEDIUM, HIGH, LOW, MEDIUM, HIGH], edges: [[0, 1], [1, 2], [2, 3], [3, 4]], expectedChains: [[0, 1], [3, 4]] })); it('returns correct data for two parallel chains', () => - testAudit({ + testGetCriticalChain({ priorityList: [HIGH, HIGH, HIGH, HIGH], edges: [[0, 2], [1, 3]], expectedChains: [[1, 3], [0, 2]] })); it('returns correct data for two parallel chains', () => - testAudit({ + testGetCriticalChain({ priorityList: [HIGH, HIGH, HIGH, HIGH], edges: [[0, 2], [1, 3]], expectedChains: [[0, 2], [1, 3]] })); it('returns correct data for fork at root', () => - testAudit({ + testGetCriticalChain({ priorityList: [HIGH, HIGH, HIGH], edges: [[0, 1], [0, 2]], expectedChains: [[0, 1], [0, 2]] })) it('returns correct data for fork at non root', () => - testAudit({ + testGetCriticalChain({ priorityList: [HIGH, HIGH, HIGH, HIGH], edges: [[0, 1], [1, 2], [1, 3]], expectedChains: [[0, 1, 2], [0, 1, 3]] })); it('returns empty chain list when no critical request', () => - testAudit({ + testGetCriticalChain({ priorityList: [LOW, LOW], edges: [[0, 1]], expectedChains: [] })); it('returns empty chain list when no request whatsoever', () => - testAudit({ + testGetCriticalChain({ priorityList: [], edges: [], expectedChains: [] })); it('returns two single node chains for two independent requests', () => - testAudit({ + testGetCriticalChain({ priorityList: [HIGH, HIGH], edges: [], expectedChains: [[0], [1]] })); it('returns correct data on a random big graph', () => - testAudit({ + testGetCriticalChain({ priorityList: Array(9).fill(HIGH), edges: [[0, 1], [1, 2], [1, 3], [4, 5], [5,7], [7, 8], [5, 6]], expectedChains: [ From 93b7c2a7a90623f8e4e8c5c5eb76f30c7a5908d4 Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Fri, 6 May 2016 23:29:14 -0400 Subject: [PATCH 09/11] fix lints --- scripts/netdep_graph_json.py | 8 +--- scripts/process_artifacts.py | 10 ++--- src/gatherers/critical-network-chains.js | 38 +++++++++---------- src/lighthouse.js | 4 +- src/scheduler.js | 1 - test/src/gatherers/critical-network-chains.js | 12 +++--- 6 files changed, 32 insertions(+), 41 deletions(-) diff --git a/scripts/netdep_graph_json.py b/scripts/netdep_graph_json.py index 84c15ea16c68..45772ff67bef 100644 --- a/scripts/netdep_graph_json.py +++ b/scripts/netdep_graph_json.py @@ -14,7 +14,6 @@ import os import sys -import pdb, traceback, sys if 'CHROMIUM_SOURCE_PATH' not in os.environ: print 'You must set the environment variable CHROMIUM_SOURCE_PATH' @@ -45,12 +44,7 @@ def get_network_dependency_graph(json_dict): def main(): with open('clovis-trace.log') as f: - try: - graph = get_network_dependency_graph(json.load(f)) - except: - type, value, tb = sys.exc_info() - traceback.print_exc() - pdb.post_mortem(tb) + graph = get_network_dependency_graph(json.load(f)) output_file = "dependency-graph.json" diff --git a/scripts/process_artifacts.py b/scripts/process_artifacts.py index afe163e49f9d..c3aa6e389fa0 100644 --- a/scripts/process_artifacts.py +++ b/scripts/process_artifacts.py @@ -36,11 +36,11 @@ def create_tracing_track(trace_events): def create_page_track(frame_load_events): events = [] - for e in frame_load_events: + for event in frame_load_events: clovis_event = { - 'frame_id': e['frameId'], - 'method': e['method'], - 'parent_frame_id': e.get('parentFrameId', None) + 'frame_id': event['frameId'], + 'method': event['method'], + 'parent_frame_id': event.get('parentFrameId', None) } events.append(clovis_event) return {'events': events} @@ -48,7 +48,7 @@ def create_page_track(frame_load_events): def create_request_track(raw_network_events): request_track = RequestTrack(None) for event in raw_network_events: - if event['method'] in RequestTrack._METHOD_TO_HANDLER: + if event['method'] in RequestTrack._METHOD_TO_HANDLER: # pylint: disable=protected-access request_track.Handle(event['method'], event) return request_track.ToJsonDict() diff --git a/src/gatherers/critical-network-chains.js b/src/gatherers/critical-network-chains.js index 6af15db1ae58..9eda43fbe693 100644 --- a/src/gatherers/critical-network-chains.js +++ b/src/gatherers/critical-network-chains.js @@ -18,7 +18,7 @@ 'use strict'; const Gather = require('./gather'); -const child_process = require('child_process'); +const childProcess = require('child_process'); const fs = require('fs'); const log = require('../lib/log.js'); @@ -27,11 +27,11 @@ const contains = (arr, elm) => arr.indexOf(elm) > -1; class Node { get requestId() { - return this.request._requestId; + return this.request.requestId; } constructor(request, parent) { this.children = []; - this.parent = null; + this.parent = parent; this.request = request; } @@ -43,9 +43,6 @@ class Node { this.children.push(childNode); } - toJSON() { - return "{requestId: " + this.requestId + ", parent: " + (this.parent && this.parent.requestId) + "}"; - } } class CriticalNetworkChains extends Gather { @@ -60,7 +57,6 @@ class CriticalNetworkChains extends Gather { // There logs are here so we can test this gatherer // Will be removed when we have a way to surface them in the report - const urlChains = chains.map(chain => chain.map(request => request._url)); const nonTrivialChains = chains.filter(chain => chain.length > 1); // Note: Approximately, @@ -72,14 +68,14 @@ class CriticalNetworkChains extends Gather { urls: chain.map(request => request._url), totalRequests: chain.length, times: chain.map(request => ({ - startTime: request._startTime, - endTime: request._endTime, - responseReceivedTime: request._responseReceivedTime + startTime: request.startTime, + endTime: request.endTime, + responseReceivedTime: request.responseReceivedTime })), totalTimeBetweenBeginAndEnd: - (chain[chain.length - 1]._endTime - chain[0]._startTime), + (chain[chain.length - 1].endTime - chain[0].startTime), totalLoadingTime: chain.reduce((acc, req) => - acc + (req._endTime - req._responseReceivedTime), 0) + acc + (req.endTime - req.responseReceivedTime), 0) })); log.log('info', 'cricitalChains', JSON.stringify(debuggingData)); return {CriticalNetworkChains: chains}; @@ -126,11 +122,11 @@ class CriticalNetworkChains extends Gather { _getChainsDFS(startNode) { if (startNode.children.length === 0) { return [[startNode]]; - } else { - const childrenChains = flatten(startNode.children.map(child => - this._getChainsDFS(child))); - return childrenChains.map(chain => [startNode].concat(chain)); } + + const childrenChains = flatten(startNode.children.map(child => + this._getChainsDFS(child))); + return childrenChains.map(chain => [startNode].concat(chain)); } _saveClovisData(url, tracingData, filename) { @@ -144,19 +140,19 @@ class CriticalNetworkChains extends Gather { } _getNetworkDependencyGraph(url, tracingData) { - const clovisDataFilename = "clovisData.json"; - const clovisGraphFilename = "dependency-graph.json"; + const clovisDataFilename = 'clovisData.json'; + const clovisGraphFilename = 'dependency-graph.json'; // These will go away once we implement initiator graph ourselves this._saveClovisData(url, tracingData, clovisDataFilename); - child_process.execSync('python scripts/process_artifacts.py'); - child_process.execSync('python scripts/netdep_graph_json.py', {stdio: [0, 1, 2]}); + childProcess.execSync('python scripts/process_artifacts.py'); + childProcess.execSync('python scripts/netdep_graph_json.py', {stdio: [0, 1, 2]}); const depGraphString = fs.readFileSync(clovisGraphFilename); try { fs.unlinkSync(clovisDataFilename); fs.unlinkSync(clovisGraphFilename); - } catch(e) { + } catch (e) { console.error(e); // Should not halt lighthouse for a file delete error } diff --git a/src/lighthouse.js b/src/lighthouse.js index d7bfce6efdb5..a505907fc2b0 100644 --- a/src/lighthouse.js +++ b/src/lighthouse.js @@ -80,8 +80,8 @@ module.exports = function(driver, opts) { const gatherers = gathererClasses.map(G => new G()); if (opts.flags.useNetDepGraph) { - const criticalChainClass = require('./gatherers/critical-network-chains'); - gatherers.push(new criticalChainClass()); + const CriticalChainClass = require('./gatherers/critical-network-chains'); + gatherers.push(new CriticalChainClass()); } return Scheduler diff --git a/src/scheduler.js b/src/scheduler.js index aeebe1dfde2e..30268036cd8a 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -114,7 +114,6 @@ function run(gatherers, options) { const driver = options.driver; const tracingData = {}; - if (options.url === undefined || options.url === null) { throw new Error('You must provide a url to scheduler'); } diff --git a/test/src/gatherers/critical-network-chains.js b/test/src/gatherers/critical-network-chains.js index c776622ff19a..97b0fff9b290 100644 --- a/test/src/gatherers/critical-network-chains.js +++ b/test/src/gatherers/critical-network-chains.js @@ -22,13 +22,15 @@ const Gatherer = new GathererClass(); function mockTracingData(prioritiesList, edges) { const networkRecords = prioritiesList.map((priority, index) => - ({_requestId: index, _initialPriority: priority})); + ({requestId: index, initialPriority: priority})); + /* eslint-disable camelcase */ const nodes = networkRecords.map(record => ({request: {request_id: record._requestId}})); const graphEdges = edges.map(edge => ({__from_node_index: edge[0], __to_node_index: edge[1]})); + /* eslint-enable camelcase */ return { networkRecords: networkRecords, @@ -54,7 +56,7 @@ const HIGH = 'High'; const VERY_HIGH = 'VeryHigh'; const MEDIUM = 'Medium'; const LOW = 'Low'; -const VERY_Low = 'VeryLow'; +const VERY_LOW = 'VeryLow'; /* global describe, it*/ describe('CriticalNetworkChain gatherer: getCriticalChain function', () => { @@ -67,7 +69,7 @@ describe('CriticalNetworkChain gatherer: getCriticalChain function', () => { it('returns correct data for chain interleaved with non-critical requests', () => testGetCriticalChain({ - priorityList: [MEDIUM, HIGH, LOW, MEDIUM, HIGH], + priorityList: [MEDIUM, HIGH, LOW, MEDIUM, HIGH, VERY_LOW], edges: [[0, 1], [1, 2], [2, 3], [3, 4]], expectedChains: [[0, 1], [3, 4]] })); @@ -91,7 +93,7 @@ describe('CriticalNetworkChain gatherer: getCriticalChain function', () => { priorityList: [HIGH, HIGH, HIGH], edges: [[0, 1], [0, 2]], expectedChains: [[0, 1], [0, 2]] - })) + })); it('returns correct data for fork at non root', () => testGetCriticalChain({ @@ -124,7 +126,7 @@ describe('CriticalNetworkChain gatherer: getCriticalChain function', () => { it('returns correct data on a random big graph', () => testGetCriticalChain({ priorityList: Array(9).fill(HIGH), - edges: [[0, 1], [1, 2], [1, 3], [4, 5], [5,7], [7, 8], [5, 6]], + edges: [[0, 1], [1, 2], [1, 3], [4, 5], [5, 7], [7, 8], [5, 6]], expectedChains: [ [0, 1, 2], [0, 1, 3], [4, 5, 7, 8], [4, 5, 6] ]})); From 72163c2d0a28a8139b8235856bb1ec867f989fab Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Mon, 9 May 2016 10:06:25 -0400 Subject: [PATCH 10/11] let -> const. Remove Duplicate test. --- src/scheduler.js | 2 +- test/src/gatherers/critical-network-chains.js | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/scheduler.js b/src/scheduler.js index 30268036cd8a..8ab132d299f2 100644 --- a/src/scheduler.js +++ b/src/scheduler.js @@ -154,7 +154,7 @@ function run(gatherers, options) { {traceContents: tracingData.traceContents}, {frameLoadEvents: tracingData.frameLoadEvents}); - let artifacts = flattenArtifacts(unflattenedArtifacts); + const artifacts = flattenArtifacts(unflattenedArtifacts); if (options.flags.saveArtifacts) { saveArtifacts(artifacts); diff --git a/test/src/gatherers/critical-network-chains.js b/test/src/gatherers/critical-network-chains.js index 97b0fff9b290..e4564955a397 100644 --- a/test/src/gatherers/critical-network-chains.js +++ b/test/src/gatherers/critical-network-chains.js @@ -81,13 +81,6 @@ describe('CriticalNetworkChain gatherer: getCriticalChain function', () => { expectedChains: [[1, 3], [0, 2]] })); - it('returns correct data for two parallel chains', () => - testGetCriticalChain({ - priorityList: [HIGH, HIGH, HIGH, HIGH], - edges: [[0, 2], [1, 3]], - expectedChains: [[0, 2], [1, 3]] - })); - it('returns correct data for fork at root', () => testGetCriticalChain({ priorityList: [HIGH, HIGH, HIGH], From 2a1be4ae37d937e00caa4322cdbc5f2815ef4dd0 Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Mon, 9 May 2016 14:09:25 -0400 Subject: [PATCH 11/11] Link to priority doc --- src/gatherers/critical-network-chains.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/gatherers/critical-network-chains.js b/src/gatherers/critical-network-chains.js index 9eda43fbe693..1431ee1e31d6 100644 --- a/src/gatherers/critical-network-chains.js +++ b/src/gatherers/critical-network-chains.js @@ -48,6 +48,10 @@ class Node { class CriticalNetworkChains extends Gather { get criticalPriorities() { + // For now, critical request == render blocking request (as decided by + // blink). Blink treats requests with the following priority levels as + // render blocking. + // See https://docs.google.com/document/d/1bCDuq9H1ih9iNjgzyAL0gpwNFiEP4TZS-YLRp_RuMlc return ['VeryHigh', 'High', 'Medium']; }