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 11 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 {
node.addDependency(directInitiatorNode);
}
});
} else if (node !== rootNode) {
rootNode.addDependent(node);
} else if (node !== directInitiatorNode) {
directInitiatorNode.addDependent(node);
}

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

/**
* @param {NetworkRequest} record The record to find the initiator of
* @param {Map<string, NetworkRequest>} Network requests that are possibly prefetch requests,
* keyed by URL.
* @param {Map<string, NetworkRequest>} Network requests that are definitely not prefetch
* requests, keyed by URL.
* @param {?NetworkRequest} The request that initiated record, or null if no valid initiator
* was found.
*@private
warrengm marked this conversation as resolved.
Show resolved Hide resolved
*/
static _chooseInitiatorRequest(record, possiblePrefetchRecords, nonPrefetchRecords) {
if (record.redirectSource && record.redirectSource.responseReceivedTime <= record.startTime) {
warrengm marked this conversation as resolved.
Show resolved Hide resolved
return record.redirectSource;
}
const stackFrames = (record.initiator.stack && record.initiator.stack.callFrames) || [];
const initiatorURL = record.initiator.url || (stackFrames[0] && stackFrames[0].url);
// Choose a non-prefetch request if one exists.
if (nonPrefetchRecords.has(initiatorURL) &&
warrengm marked this conversation as resolved.
Show resolved Hide resolved
nonPrefetchRecords.get(initiatorURL).responseReceivedTime <= record.startTime) {
return nonPrefetchRecords.get(initiatorURL);
}
// If we couldn't find a non-prefetch request as the initiator, fallback to possible prefetch
// record. Maybe we can improve detection of prefetch requests in the future, but this is based
// purely on resourceType as of writing this comment.
if (possiblePrefetchRecords.has(initiatorURL) &&
possiblePrefetchRecords.get(initiatorURL).responseReceivedTime <= record.startTime) {
return possiblePrefetchRecords.get(initiatorURL);
}
return null;
}

/**
* Construct network records from a log of devtools protocol messages.
* @param {LH.DevtoolsLog} devtoolsLog
Expand All @@ -340,20 +371,26 @@ 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
const recordsByURL = new Map();
// The initiator is unlikely to be a prefetch record which have type 'Other'
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

// Create two maps of initiators, one for prefetch, one for regular, that contain the earliest
// request for the URL. The prefetch map may have false positives so we may fall back to it
// for setting the initiator.
const possiblePrefetchRecords = new Map();
const nonPrefetchRecords = new Map();
for (const record of records) {
if (recordsByURL.has(record.url)) continue;
recordsByURL.set(record.url, record);
if (record.type === NetworkRequest.TYPES.Other) {
warrengm marked this conversation as resolved.
Show resolved Hide resolved
if (!possiblePrefetchRecords.has(record.url)) {
possiblePrefetchRecords.set(record.url, record);
}
} else if (!nonPrefetchRecords.has(record.url)) {
nonPrefetchRecords.set(record.url, record);
}
}

// 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._chooseInitiatorRequest(
record, possiblePrefetchRecords, nonPrefetchRecords);
if (initiator) {
record.setInitiatorRequest(initiator);
}
Expand Down
26 changes: 26 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,32 @@ describe('PageDependencyGraph computed artifact:', () => {
assert.deepEqual(nodes[3].getDependencies(), [nodes[1], nodes[2]]);
});

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