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

core(lantern): add edges from initiatorRequest when there are duplicate records #10097

Merged
merged 22 commits into from
Apr 2, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
18 changes: 12 additions & 6 deletions lighthouse-core/computed/page-dependency-graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,16 +144,22 @@ class PageDependencyGraph {
*/
static linkNetworkNodes(rootNode, networkNodeOutput) {
networkNodeOutput.nodes.forEach(node => {
const directInitiatorRequest = node.record.initiatorRequest || rootNode.record;
const directInitiatorNode =
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
networkNodeOutput.idToNodeMap.get(directInitiatorRequest.requestId) || rootNode;
const initiators = PageDependencyGraph.getNetworkInitiators(node.record);
if (initiators.length) {
initiators.forEach(initiator => {
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;
node.addDependency(parent);
const parentCandidates = networkNodeOutput.urlToNodeMap.get(initiator) || [];
// Only add the edge if the parent is unambiguous with valid timing.
if (parentCandidates.length === 1 && parentCandidates[0].startTime <= node.startTime) {
node.addDependency(parentCandidates[0]);
} else {
directInitiatorNode.addDependent(node);
}
});
} else if (node !== rootNode) {
rootNode.addDependent(node);
} else if (node !== directInitiatorNode) {
directInitiatorNode.addDependent(node);
}

if (!node.record.redirects) return;
Expand Down
49 changes: 41 additions & 8 deletions lighthouse-core/lib/network-recorder.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,42 @@ class NetworkRecorder extends EventEmitter {
return request;
}

/**
* @param {NetworkRequest} record The record to find the initiator of
* @param {Map<string, NetworkRequest[]>} recordsByURL
* @return {NetworkRequest|null}
* @private
*/
static _chooseInitiator(record, recordsByURL) {
if (record.redirectSource) {
return record.redirectSource;
}
const stackFrames = (record.initiator.stack && record.initiator.stack.callFrames) || [];
const initiatorURL = record.initiator.url || (stackFrames[0] && stackFrames[0].url);

let candidates = recordsByURL.get(initiatorURL) || [];
// The initiator must come before the initiated request.
candidates = candidates.filter(cand => cand.responseReceivedTime <= record.startTime);
if (candidates.length > 1) {
// Disambiguate based on resource type. Prefetch requests have type 'Other' and cannot
// initiate requests, so we drop them here.
const nonPrefetchCandidates = candidates.filter(
cand => cand.resourceType !== NetworkRequest.TYPES.Other);
if (nonPrefetchCandidates.length) {
candidates = nonPrefetchCandidates;
}
}
if (candidates.length > 1) {
// Disambiguate based on frame. It's likely that the initiator comes from the same frame.
const sameFrameCandidates = candidates.filter(cand => cand.frameId == record.frameId);
if (sameFrameCandidates.length) {
candidates = sameFrameCandidates;
}
}

return candidates.length ? candidates[0] : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry it's been awhile but should we only set the initiator if it's unambiguous? i.e.

Suggested change
return candidates.length ? candidates[0] : null;
return candidates.length === 1 ? candidates[0] : null;

Copy link
Collaborator

Choose a reason for hiding this comment

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

friendly bump on this, if you're already thought this through a simple explanation is all I'm lookin for here :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This works for now, but if you have a sec to explain your thoughts behind this at some point @warrengm that'd be great :)

}

/**
* Construct network records from a log of devtools protocol messages.
* @param {LH.DevtoolsLog} devtoolsLog
Expand All @@ -340,20 +376,17 @@ class NetworkRecorder extends EventEmitter {
// get out the list of records & filter out invalid records
const records = networkRecorder.getRecords().filter(record => record.isValid);

// create a map of all the records by URL to link up initiator
/** @type {Map<string, NetworkRequest> */
const recordsByURL = new Map();
for (const record of records) {
if (recordsByURL.has(record.url)) continue;
recordsByURL.set(record.url, record);
const records = recordsByURL.get(record.url) || [];
records.push(record);
recordsByURL.set(record.url, records);
}

// set the initiator and redirects array
for (const record of records) {
const stackFrames = (record.initiator.stack && record.initiator.stack.callFrames) || [];
const initiatorURL = record.initiator.url || (stackFrames[0] && stackFrames[0].url);
// If we were redirected to this request, our initiator is that redirect, otherwise, it's the
// initiator provided by the protocol. See https://github.com/GoogleChrome/lighthouse/pull/7352/
const initiator = record.redirectSource || recordsByURL.get(initiatorURL);
const initiator = NetworkRecorder._chooseInitiator(record, recordsByURL);
if (initiator) {
record.setInitiatorRequest(initiator);
}
Expand Down
51 changes: 51 additions & 0 deletions lighthouse-core/test/computed/page-dependency-graph-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,57 @@ describe('PageDependencyGraph computed artifact:', () => {
assert.deepEqual(nodes[3].getDependencies(), [nodes[1], nodes[2]]);
});

it('should link up script initiators only when timing is valid', () => {
const request1 = createRequest(1, '1', 0);
const request2 = createRequest(2, '2', 500);
const request3 = createRequest(3, '3', 500);
const request4 = createRequest(4, '4', 20);
request4.initiator = {
type: 'script',
stack: {callFrames: [{url: '2'}], parent: {parent: {callFrames: [{url: '3'}]}}},
};
const networkRecords = [request1, request2, request3, request4];

addTaskEvents(0, 0, []);

const graph = PageDependencyGraph.createGraph(traceOfTab, networkRecords);
const nodes = [];
graph.traverse(node => nodes.push(node));

assert.equal(nodes.length, 4);
assert.deepEqual(nodes.map(node => node.id), [1, 2, 3, 4]);
assert.deepEqual(nodes[0].getDependencies(), []);
assert.deepEqual(nodes[1].getDependencies(), [nodes[0]]);
assert.deepEqual(nodes[2].getDependencies(), [nodes[0]]);
assert.deepEqual(nodes[3].getDependencies(), [nodes[0]]);
});

it('should link up script initiators with prefetch requests', () => {
warrengm marked this conversation as resolved.
Show resolved Hide resolved
const request1 = createRequest(1, 'a.com/1', 0);
const request2Prefetch = createRequest(2, 'a.com/js', 5);
const request2Fetch = createRequest(3, 'a.com/js', 10);
const request3 = createRequest(4, 'a.com/4', 20);
request3.initiator = {
type: 'script',
stack: {callFrames: [{url: 'a.com/js'}], parent: {parent: {callFrames: [{url: 'js'}]}}},
};
request3.initiatorRequest = request2Fetch;
const networkRecords = [request1, request2Prefetch, request2Fetch, request3];

addTaskEvents(0, 0, []);

const graph = PageDependencyGraph.createGraph(traceOfTab, networkRecords);
const nodes = [];
graph.traverse(node => nodes.push(node));

assert.equal(nodes.length, 4);
assert.deepEqual(nodes.map(node => node.id), [1, 2, 3, 4]);
assert.deepEqual(nodes[0].getDependencies(), []);
assert.deepEqual(nodes[1].getDependencies(), [nodes[0]]);
assert.deepEqual(nodes[2].getDependencies(), [nodes[0]]);
assert.deepEqual(nodes[3].getDependencies(), [nodes[2]]);
});

it('should throw when root node is not related to main document', () => {
const request1 = createRequest(1, '1', 0, null, NetworkRequest.TYPES.Other);
const request2 = createRequest(2, '2', 5, null, NetworkRequest.TYPES.Document);
Expand Down
Loading