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 23 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
149 changes: 149 additions & 0 deletions src/gatherers/critical-network-chains.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
/**
* @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 includes = (arr, elm) => arr.indexOf(elm) > -1;

class RequestNode {
get requestId() {
return this.request.requestId;
}

constructor(request, parent) {
// The children of a RequestNode are the requests initiated by it
this.children = [];
// The parent of a RequestNode is the request that initiated it
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 : null},
children: ${JSON.stringify(this.children.map(child => child.requestId))}
}`;
}

}

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 => includes(this.criticalPriorities, req.initialPriority()));

// Build a map of requestID -> Node.
const requestIdToNodes = new Map();
for (let request of criticalRequests) {
const requestNode = new RequestNode(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;
30 changes: 29 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 @@ -195,6 +202,24 @@ WebInspector.ConsoleMessage.MessageType = {
Log: 'log'
};

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

WebInspector.NetworkLog.prototype = {
requestForURL: function(url) {
return this._requests.get(url) || null;
},

_onRequestStarted: function(event) {
var request = event.data;
this._requests.set(request.url, request);
}
};

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

Expand Down Expand Up @@ -222,7 +247,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