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

Get critical chains: pure js version #310

Merged
merged 27 commits into from
May 11, 2016
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
0cdc88e
qweqwe
deepanjanroy Apr 27, 2016
c4964d8
WIP overdep
deepanjanroy Apr 28, 2016
8d5b7c7
overdep WIP
deepanjanroy Apr 29, 2016
e9a4850
Merge branch 'master' into overdep
deepanjanroy May 2, 2016
f01db28
Add audit and tests
deepanjanroy May 4, 2016
6916f5e
Merge branch 'master' into overdep
deepanjanroy May 4, 2016
d742c05
Actually add audit file. Remove old require
deepanjanroy May 4, 2016
ad660da
criticial chain more WIP
deepanjanroy May 6, 2016
b76cfde
Merge branch 'master' into overdep
deepanjanroy May 6, 2016
123d133
even more WIP
deepanjanroy May 6, 2016
b3c7e46
critical chains: more more WIP
deepanjanroy May 7, 2016
c3f293b
Merge branch 'master' into overdep
deepanjanroy May 7, 2016
93b7c2a
fix lints
deepanjanroy May 7, 2016
72163c2
let -> const. Remove Duplicate test.
deepanjanroy May 9, 2016
2a1be4a
Link to priority doc
deepanjanroy May 9, 2016
0ad2fc4
Get request dependencies from devtools initiator
deepanjanroy May 10, 2016
cfa80ac
Merge branch 'master' into makeclovisjsagain
deepanjanroy May 10, 2016
de54e42
fix tests
deepanjanroy May 10, 2016
9f2ac89
Fix lints
deepanjanroy May 10, 2016
b1fd37b
remove unnecessary require
deepanjanroy May 10, 2016
187b348
rename contains -> includes
deepanjanroy May 10, 2016
da1da08
Fix mock networklog code
deepanjanroy May 10, 2016
056ebe8
fix json. fix node naming
deepanjanroy May 10, 2016
b940c7a
add lots of type annotations
deepanjanroy May 11, 2016
e6804c7
sort and milisecs
deepanjanroy May 11, 2016
4065e12
Fix tests - real set deep equal
deepanjanroy May 11, 2016
6270862
lint
deepanjanroy May 11, 2016
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
1 change: 1 addition & 0 deletions scripts/netdep_graph_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def main():
with open('clovis-trace.log') as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is pure JS, do we not want to remove these python files?

Copy link
Member

Choose a reason for hiding this comment

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

(see the initial PR comment :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah okay... I'm not sure. Feels like we should keep it around in a separate branch on origin, but I don't want to merge it in unless we know we need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 200% confident about how correct my approach is or whether we'll end up needing the clovis graph for something else. Once we're sure clovis is never happening I'll purge all python from lighthouse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I didn't see your other comment. The python files were already on master. Do you want me to remove them and put them on other branch? There is still use for them in the sense you can do lighthouse --save-artifacts and then use the artifacts through clovis to generate the dependency graph png picture. If you want I can send out a separate PR removing all python/clovis code.

graph = get_network_dependency_graph(json.load(f))


output_file = "dependency-graph.json"
with open(output_file, 'w') as f:
json.dump(graph.deps_graph.ToJsonDict(), f)
Expand Down
15 changes: 11 additions & 4 deletions scripts/process_artifacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,25 @@ 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 event in frame_load_events:
clovis_event = {
'frame_id': event['frameId'],
'method': event['method'],
'parent_frame_id': event.get('parentFrameId', None)
}
events.append(clovis_event)
return {'events': 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: # pylint: disable=protected-access
request_track.Handle(event['method'], event)
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 = {}
Expand Down
145 changes: 145 additions & 0 deletions src/gatherers/critical-network-chains.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
/**
* @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 log = require('../lib/log.js');

const flatten = arr => arr.reduce((a, b) => a.concat(b), []);
const contains = (arr, elm) => arr.indexOf(elm) > -1;
Copy link
Member

Choose a reason for hiding this comment

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

change to includes to match [].includes (coming in node 6)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


class Node {
Copy link
Member

Choose a reason for hiding this comment

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

maybe something slightly more descriptive, RequestNode or something?

Copy link
Member

Choose a reason for hiding this comment

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

also a comment describing what children and parent are in this context would be helpful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the request graph is actually a tree, so these are tree nodes with one parent and a bunch of children. I'll rename and add a comment.

Copy link
Member

Choose a reason for hiding this comment

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

right, so to be clear that's the relationship the tree represents, something like InitiatorNode then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like RequestNode better, because it holds data about a request. The initiator relationship is captured by the edges, so I'll add a comment about what the parent-child relationship represents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the leaves are not initiators of anything so it would be weird to call them InitiatorNode.

get requestId() {
return this.request.requestId;
}
constructor(request, parent) {
this.children = [];
this.parent = parent;
this.request = request;
}

setParent(parentNode) {
this.parent = parentNode;
}

addChild(childNode) {
this.children.push(childNode);
}

toJSON() {
// Prevents circular reference so we can print nodes when needed
return `{
id: ${this.requestId},
parent: ${!!(this.parent) && this.parent.requestId},
Copy link
Member

Choose a reason for hiding this comment

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

drop the parens here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. changed it to print null when no parent.

children: ${this.children.map(child => child.requestId)}
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be surrounded by [] to make sure it's valid JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I call JSON.stringify on the array now.

}`;
}

}

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'];
}

getCriticalChains(networkRecords) {
// Drop the first request because it's uninteresting - it's the page html
// and always critical. No point including it in every request
const criticalRequests = networkRecords.slice(1).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 request of criticalRequests) {
if (request.initiatorRequest()) {
const parentRequestId = request.initiatorRequest().requestId;
const childNode = requestIdToNodes.get(request.requestId);
const parentNode = requestIdToNodes.get(parentRequestId);
if (childNode && parentNode) {
// Both child and parent must be critical
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: what does it mean (and is it possible) for a parent to not be critical but one of its children is critical? If possible, is that worth capturing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a very good question. One way this case can happen is if a low priority script ends up initiating a high priority script. I don't know the internals of blink well enough yet to answer if this case is possible. However, I really don't know if we should call the initiated script "critical" in that case, because it's very possible the page already loaded by the time we see this high priority request, and no request should be called "critical" at that point. We can improve this by checking the timestamp of requests and discarding any request that gets fired after domContentLoaded, since it's not render-blocking any more. I haven't seen this happen in practice yet though. Also, when we have better critical request data that are not just based on request priority, this issue will disappear.

The other way this happens and is absolutely worth fixing is when there are redirects. Having a request redirected sometimes sets the priority of the new request as 'VeryLow'. For example, the first request to theverge.com has prio VeryHigh, but it redirects to www.theverge.com whose prio is VeryLow. But this new request is actually the initiator of all the subsequent requests, and we end up with a non-critical parent having critical children. I'm still trying to understand this case better and this is why I have the TODO about redirects.

// TODO: We may need handle redirects carefully. Investigate
Copy link
Member

Choose a reason for hiding this comment

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

file an issue to track this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as soon as we merge this.

childNode.setParent(parentNode);
parentNode.addChild(childNode);
}
}
}

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;
}

postProfiling(options, tracingData) {
const chains = this.getCriticalChains(tracingData.networkRecords);

if (options.flags.useNetDepGraph) {
// 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 nonTrivialChains = chains.filter(chain => chain.length > 1);
Copy link
Member

@paulirish paulirish May 11, 2016

Choose a reason for hiding this comment

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

lets sort them desc by length?

.sort((a,b) => b.length - a.length)

Copy link
Member

Choose a reason for hiding this comment

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

ah actually we want .sort((a,b) => b.totalTimeBetweenBeginAndEnd - a.totalTimeBetweenBeginAndEnd) but after the bit below. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


// 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
})),
totalTimeBetweenBeginAndEnd:
(chain[chain.length - 1].endTime - chain[0].startTime),
Copy link
Member

Choose a reason for hiding this comment

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

Let's * 1000 to bring these values into ms

totalLoadingTime: chain.reduce((acc, req) =>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this number is valuable yet. But interesting to keep an eye on.

acc + (req.endTime - req.responseReceivedTime), 0)
}));
log.log('info', 'cricital chains', JSON.stringify(debuggingData));
log.log('info', 'lengths of critical chains', debuggingData.map(d => d.totalRequests));
}
return {CriticalNetworkChains: chains};
}

Copy link
Member

Choose a reason for hiding this comment

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

type docs would help here (and above) for clarity, even if not checked. Hard to wade through all the flattens :) @return {!Array<!Array<!RequestNode>>}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added some types. Did not annotate everything but I hope it makes it little more readable

_getChainsDFS(startNode) {
if (startNode.children.length === 0) {
return [[startNode]];
}

const childrenChains = flatten(startNode.children.map(child =>
this._getChainsDFS(child)));
return childrenChains.map(chain => [startNode].concat(chain));
}
}

module.exports = CriticalNetworkChains;
2 changes: 2 additions & 0 deletions src/lib/drivers/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,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();
Expand All @@ -294,6 +295,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,
Expand Down
5 changes: 4 additions & 1 deletion src/lib/frame-load-recorder.js
Original file line number Diff line number Diff line change
Expand Up @@ -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});
}

}
Expand Down
5 changes: 5 additions & 0 deletions src/lib/network-recorder.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
40 changes: 39 additions & 1 deletion src/lib/web-inspector.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,15 @@ global.NetworkAgent = {
Origin: 'origin',
Inspector: 'inspector',
Other: 'other'
},
InitiatorType: {
Other: 'other',
Parser: 'parser',
Redirect: 'redirect',
Script: 'script'
}
};

// Enum from SecurityState enum in protocol's Security domain
global.SecurityAgent = {
SecurityState: {
Expand Down Expand Up @@ -165,6 +172,7 @@ require('chrome-devtools-frontend/front_end/timeline/TimelineProfileTree.js');
require('chrome-devtools-frontend/front_end/components_lazy/FilmStripModel.js');
require('chrome-devtools-frontend/front_end/timeline/TimelineIRModel.js');
require('chrome-devtools-frontend/front_end/timeline/TimelineFrameModel.js');
// require('chrome-devtools-frontend/front_end/sdk/NetworkLog.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


// DevTools makes a few assumptions about using backing storage to hold traces.
WebInspector.DeferredTempFile = function() {};
Expand Down Expand Up @@ -195,6 +203,33 @@ WebInspector.ConsoleMessage.MessageType = {
Log: 'log'
};

// Mock NetworkLog
WebInspector.NetworkLog = function(target) {
this._requests = [];
target.networkManager.addEventListener(
WebInspector.NetworkManager.EventTypes.RequestStarted, this._onRequestStarted, this);
};

WebInspector.NetworkLog.prototype = {
requests: function() {
return this._requests;
},

requestForURL: function(url) {
for (var i = 0; i < this._requests.length; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we'll never have a large number of requests, but would it be better to just have an object (or Map) here, keyed on the url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely. I blindly copied over the implementation from devtools code, but for our use case a map definitely works better. Will change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (this._requests[i].url === url) {
return this._requests[i];
}
}
return null;
},

_onRequestStarted: function(event) {
var request = (event.data);
Copy link
Member

Choose a reason for hiding this comment

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

remove parens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

this._requests.push(request);
}
};

// Dependencies for color parsing.
require('chrome-devtools-frontend/front_end/common/Color.js');

Expand Down Expand Up @@ -222,7 +257,10 @@ WebInspector.NetworkManager.createWithFakeTarget = function() {
registerNetworkDispatcher() {}
};

return new WebInspector.NetworkManager(fakeTarget);
fakeTarget.networkManager = new WebInspector.NetworkManager(fakeTarget);
fakeTarget.networkLog = new WebInspector.NetworkLog(fakeTarget);

return fakeTarget.networkManager;
};

module.exports = WebInspector;
3 changes: 2 additions & 1 deletion src/lighthouse.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ const gathererClasses = [
require('./gatherers/html'),
require('./gatherers/manifest'),
require('./gatherers/accessibility'),
require('./gatherers/offline')
require('./gatherers/offline'),
require('./gatherers/critical-network-chains')
];

const audits = [
Expand Down
1 change: 0 additions & 1 deletion src/scheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
'use strict';

const fs = require('fs');

const log = require('./lib/log.js');

function loadPage(driver, gatherers, options) {
Expand Down
Loading