Skip to content

Commit

Permalink
feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@patrickhulce
patrickhulce committed Sep 12, 2017
1 parent c7b6661 commit 02e5f03
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 47 deletions.
18 changes: 10 additions & 8 deletions lighthouse-core/gather/computed/dependency-graph/cpu-node.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@ const Node = require('./node');
class CPUNode extends Node {
/**
* @param {!TraceEvent} parentEvent
* @param {!Array<TraceEvent>=} children
* @param {!Array<TraceEvent>=} childEvents
*/
constructor(parentEvent, children = []) {
super(`${parentEvent.tid}.${parentEvent.ts}`);
constructor(parentEvent, childEvents = []) {
const nodeId = `${parentEvent.tid}.${parentEvent.ts}`;
super(nodeId);

this._event = parentEvent;
this._children = children;
this._childEvents = childEvents;
}

/**
Expand Down Expand Up @@ -49,22 +51,22 @@ class CPUNode extends Node {
/**
* @return {!TraceEvent}
*/
get children() {
return this._children;
get childEvents() {
return this._childEvents;
}

/**
* @return {boolean}
*/
didPerformLayout() {
return this._children.some(evt => evt.name === 'Layout');
return this._childEvents.some(evt => evt.name === 'Layout');
}

/**
* @return {!CPUNode}
*/
cloneWithoutRelationships() {
return new CPUNode(this._event, this._children);
return new CPUNode(this._event, this._childEvents);
}
}

Expand Down
18 changes: 14 additions & 4 deletions lighthouse-core/gather/computed/dependency-graph/estimator/estimator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class Estimator {

/**
* @param {!Node} node
* @param {!Object} values
* @param {!NodeTimingData} values
*/
_setTimingData(node, values) {
const timingData = this._nodeTiming.get(node) || {};
Expand Down Expand Up @@ -183,7 +183,7 @@ class Estimator {

// Try to add all its dependents to the queue
for (const dependent of node.getDependents()) {
// Skip this node if one of its dependencies hasn't finished yet
// Skip dependent node if one of its dependencies hasn't finished yet
const dependencies = dependent.getDependencies();
if (dependencies.some(dependency => !this._nodesCompleted.has(dependency))) continue;

Expand Down Expand Up @@ -321,7 +321,7 @@ class Estimator {

/**
* Estimates the time taken to process all of the graph's nodes.
* @return {{timeInMs: number, nodeTiming: !Map<!Node, Object>}}
* @return {{timeInMs: number, nodeTiming: !Map<!Node, !NodeTimingData>}}
*/
estimateWithDetails() {
// initialize all the necessary data containers
Expand All @@ -342,7 +342,7 @@ class Estimator {
// add root node to queue
this._markNodeAsInQueue(rootNode, totalElapsedTime);

// loop as long as we have nodes in the queue or currently in process
// loop as long as we have nodes in the queue or currently in progress
while (nodesInQueue.size || nodesInProgress.size) {
depth++;

Expand Down Expand Up @@ -387,3 +387,13 @@ class Estimator {
}

module.exports = Estimator;

/**
* @typedef {{
* estimatedTimeElapsed: number|undefined,
* timeElapsed: number|undefined,
* timeElapsedOvershoot: number|undefined,
* bytesDownloaded: number|undefined,
* }}
*/
Estimator.NodeTimingData; // eslint-disable-line no-unused-expressions
8 changes: 0 additions & 8 deletions lighthouse-core/gather/computed/dependency-graph/node.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,14 +120,6 @@ class Node {
this._dependencies.push(node);
}

/**
* @param {!Node}
* @return {boolean}
*/
hasDependency(node) {
return this._dependencies.includes(node);
}

/**
* Clones the node's information without adding any dependencies/dependents.
* @return {!Node}
Expand Down
39 changes: 20 additions & 19 deletions lighthouse-core/gather/computed/page-dependency-graph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ const CPUNode = require('./dependency-graph/cpu-node');
const GraphEstimator = require('./dependency-graph/estimator/estimator');
const TracingProcessor = require('../../lib/traces/tracing-processor');

const MINIMUM_TASK_DURATION = 10 * 1000;
// Tasks smaller than 10 ms have minimal impact on simulation
const MINIMUM_TASK_DURATION_OF_INTEREST = 10;

class PageDependencyGraphArtifact extends ComputedArtifact {
get name() {
Expand Down Expand Up @@ -41,22 +42,20 @@ class PageDependencyGraphArtifact extends ComputedArtifact {
* @param {!Array<!WebInspector.NetworkRequest>} networkRecords
* @return {!NetworkNodeOutput}
*/
static getNetworkNodes(networkRecords) {
static getNetworkNodeOutput(networkRecords) {
const nodes = [];
const idToNodeMap = new Map();
const urlToNodeMap = new Map();

networkRecords.forEach(record => {
const node = new NetworkNode(record);
nodes.push(node);
idToNodeMap.set(record.requestId, node);

if (urlToNodeMap.has(record.url)) {
const list = urlToNodeMap.get(record.url);
list.push(node);
} else {
urlToNodeMap.set(record.url, [node]);
}
const list = urlToNodeMap.get(record.url) || [];
list.push(node);

idToNodeMap.set(record.requestId, node);
urlToNodeMap.set(record.url, list);
});

return {nodes, idToNodeMap, urlToNodeMap};
Expand All @@ -69,14 +68,16 @@ class PageDependencyGraphArtifact extends ComputedArtifact {
static getCPUNodes(traceOfTab) {
const nodes = [];
let i = 0;

const minimumEvtDur = MINIMUM_TASK_DURATION_OF_INTEREST * 1000;
while (i < traceOfTab.mainThreadEvents.length) {
const evt = traceOfTab.mainThreadEvents[i];

// Skip all trace events that aren't schedulable tasks with sizable duration
if (
evt.name !== TracingProcessor.SCHEDULABLE_TASK_TITLE ||
!evt.dur ||
evt.dur < MINIMUM_TASK_DURATION
evt.dur < minimumEvtDur
) {
i++;
continue;
Expand Down Expand Up @@ -111,7 +112,7 @@ class PageDependencyGraphArtifact extends ComputedArtifact {
const parentCandidates = networkNodeOutput.urlToNodeMap.get(initiator) || [rootNode];
// Only add the initiator relationship if the initiator is unambiguous
const parent = parentCandidates.length === 1 ? parentCandidates[0] : rootNode;
parent.addDependent(node);
node.addDependency(parent);
});
} else if (node !== rootNode) {
rootNode.addDependent(node);
Expand All @@ -123,13 +124,12 @@ class PageDependencyGraphArtifact extends ComputedArtifact {
* @param {!Node} rootNode
* @param {!NetworkNodeOutput} networkNodeOutput
* @param {!Array<!CPUNode>} cpuNodes
* @param {!TraceOfTabArtifact} traceOfTab
*/
static linkCPUNodes(rootNode, networkNodeOutput, cpuNodes) {
function addDependentNetworkRequest(cpuNode, reqId) {
const networkNode = networkNodeOutput.idToNodeMap.get(reqId);
if (!networkNode || networkNode.resourceType !== 'xhr') return;
networkNode.addDependency(cpuNode);
cpuNode.addDependent(networkNode);
}

function addDependencyOnUrl(cpuNode, url) {
Expand All @@ -153,7 +153,7 @@ class PageDependencyGraphArtifact extends ComputedArtifact {

const timers = new Map();
for (const node of cpuNodes) {
for (const evt of node.children) {
for (const evt of node.childEvents) {
if (!evt.args.data) continue;

const url = evt.args.data.url;
Expand Down Expand Up @@ -217,21 +217,22 @@ class PageDependencyGraphArtifact extends ComputedArtifact {
* @return {!Node}
*/
static createGraph(traceOfTab, networkRecords) {
const networkNodeOutput = this.getNetworkNodes(networkRecords);
const cpuNodes = this.getCPUNodes(traceOfTab);
const networkNodeOutput = PageDependencyGraphArtifact.getNetworkNodeOutput(networkRecords);
const cpuNodes = PageDependencyGraphArtifact.getCPUNodes(traceOfTab);

const rootRequest = networkRecords.reduce((min, r) => (min.startTime < r.startTime ? min : r));
const rootNode = networkNodeOutput.idToNodeMap.get(rootRequest.requestId);

this.linkNetworkNodes(rootNode, networkNodeOutput, networkRecords);
this.linkCPUNodes(rootNode, networkNodeOutput, cpuNodes, traceOfTab);
PageDependencyGraphArtifact.linkNetworkNodes(rootNode, networkNodeOutput, networkRecords);
PageDependencyGraphArtifact.linkCPUNodes(rootNode, networkNodeOutput, cpuNodes);

return rootNode;
}

/**
* Estimates the duration of the graph and returns individual node timing information.
* @param {!Node} rootNode
* @return {}
* @return {{timeInMs: number, nodeTiming: !Map<!Node, !NodeTimingData>}}
*/
static estimateGraph(rootNode) {
return new GraphEstimator(rootNode).estimateWithDetails();
Expand Down
16 changes: 8 additions & 8 deletions lighthouse-core/test/gather/computed/page-dependency-graph-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,14 @@ describe('PageDependencyGraph computed artifact:', () => {
});
});

describe('#getNetworkNodes', () => {
describe('#getNetworkNodeOutput', () => {
const request1 = createRequest(1, 'urlA');
const request2 = createRequest(2, 'urlB');
const request3 = createRequest(3, 'urlB');
const networkRecords = [request1, request2, request3];

it('should create network nodes', () => {
const networkNodeOutput = PageDependencyGraph.getNetworkNodes(networkRecords);
const networkNodeOutput = PageDependencyGraph.getNetworkNodeOutput(networkRecords);
for (let i = 0; i < networkRecords.length; i++) {
const node = networkNodeOutput.nodes[i];
assert.ok(node, `did not create node at index ${i}`);
Expand All @@ -85,15 +85,15 @@ describe('PageDependencyGraph computed artifact:', () => {
});

it('should index nodes by ID', () => {
const networkNodeOutput = PageDependencyGraph.getNetworkNodes(networkRecords);
const networkNodeOutput = PageDependencyGraph.getNetworkNodeOutput(networkRecords);
const indexedById = networkNodeOutput.idToNodeMap;
for (const record of networkRecords) {
assert.equal(indexedById.get(record.requestId).record, record);
}
});

it('should index nodes by URL', () => {
const networkNodeOutput = PageDependencyGraph.getNetworkNodes(networkRecords);
const networkNodeOutput = PageDependencyGraph.getNetworkNodeOutput(networkRecords);
const nodes = networkNodeOutput.nodes;
const indexedByUrl = networkNodeOutput.urlToNodeMap;
assert.deepEqual(indexedByUrl.get('urlA'), [nodes[0]]);
Expand Down Expand Up @@ -122,15 +122,15 @@ describe('PageDependencyGraph computed artifact:', () => {
assert.equal(node1.id, '1.0');
assert.equal(node1.type, 'cpu');
assert.equal(node1.event, traceOfTab.mainThreadEvents[0]);
assert.equal(node1.children.length, 2);
assert.equal(node1.children[1].name, 'OtherEvent');
assert.equal(node1.childEvents.length, 2);
assert.equal(node1.childEvents[1].name, 'OtherEvent');

const node2 = nodes[1];
assert.equal(node2.id, '1.250000');
assert.equal(node2.type, 'cpu');
assert.equal(node2.event, traceOfTab.mainThreadEvents[5]);
assert.equal(node2.children.length, 1);
assert.equal(node2.children[0].name, 'LaterEvent');
assert.equal(node2.childEvents.length, 1);
assert.equal(node2.childEvents[0].name, 'LaterEvent');
});
});

Expand Down

0 comments on commit 02e5f03

Please sign in to comment.