Merge 0ebdeb6 into 7e01883

lighthouse-core/audits/critical-request-chains.js

@@ -37,7 +37,7 @@ class CriticalRequestChains extends Audit {
       title: str_(UIStrings.title),
       description: str_(UIStrings.description),
       scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE,
-      requiredArtifacts: ['devtoolsLogs', 'URL'],
+      requiredArtifacts: ['traces', 'devtoolsLogs', 'URL'],
     };
   }
 
@@ -167,9 +167,10 @@ class CriticalRequestChains extends Audit {
    * @return {Promise<LH.Audit.Product>}
    */
   static audit(artifacts, context) {
+    const trace = artifacts.traces[Audit.DEFAULT_PASS];
     const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS];
     const URL = artifacts.URL;
-    return ComputedChains.request({devtoolsLog, URL}, context).then(chains => {
+    return ComputedChains.request({devtoolsLog, trace, URL}, context).then(chains => {
       let chainCount = 0;
       /**
        * @param {LH.Audit.SimpleCriticalRequestNode} node

lighthouse-core/computed/critical-request-chains.js

@@ -7,8 +7,8 @@
 
 const makeComputedArtifact = require('./computed-artifact.js');
 const NetworkRequest = require('../lib/network-request.js');
-const NetworkRecords = require('./network-records.js');
 const MainResource = require('./main-resource.js');
+const PageDependencyGraph = require('./page-dependency-graph.js');
 
 class CriticalRequestChains {
   /**
@@ -67,108 +67,79 @@ class CriticalRequestChains {
   }
 
   /**
-   * @param {Array<LH.Artifacts.NetworkRequest>} networkRecords
+   * Create a tree of critical requests.
    * @param {LH.Artifacts.NetworkRequest} mainResource
+   * @param {LH.Gatherer.Simulation.GraphNode} graph
    * @return {LH.Artifacts.CriticalRequestNode}
    */
-  static extractChain(networkRecords, mainResource) {
-    networkRecords = networkRecords.filter(req => req.finished);
-
-    // Build a map of requestID -> Node.
-    /** @type {Map<string, LH.Artifacts.NetworkRequest>} */
-    const requestIdToRequests = new Map();
-    for (const request of networkRecords) {
-      requestIdToRequests.set(request.requestId, request);
-    }
-
-    // Get all the critical requests.
-    /** @type {Array<LH.Artifacts.NetworkRequest>} */
-    const criticalRequests = networkRecords.filter(request =>
-      CriticalRequestChains.isCritical(request, mainResource));
-
-    // Create a tree of critical requests.
+  static extractChainsFromGraph(mainResource, graph) {
     /** @type {LH.Artifacts.CriticalRequestNode} */
-    const criticalRequestChains = {};
-    for (const request of criticalRequests) {
-      // Work back from this request up to the root. If by some weird quirk we are giving request D
-      // here, which has ancestors C, B and A (where A is the root), we will build array [C, B, A]
-      // during this phase.
-      /** @type {Array<string>} */
-      const ancestors = [];
-      let ancestorRequest = request.initiatorRequest;
-      /** @type {LH.Artifacts.CriticalRequestNode|undefined} */
-      let node = criticalRequestChains;
-      while (ancestorRequest) {
-        const ancestorIsCritical = CriticalRequestChains.isCritical(ancestorRequest, mainResource);
-
-        // If the parent request isn't a high priority request it won't be in the
-        // requestIdToRequests map, and so we can break the chain here. We should also
-        // break it if we've seen this request before because this is some kind of circular
-        // reference, and that's bad.
-        if (!ancestorIsCritical || ancestors.includes(ancestorRequest.requestId)) {
-          // Set the ancestors to an empty array and unset node so that we don't add
-          // the request in to the tree.
-          ancestors.length = 0;
-          node = undefined;
-          break;
-        }
-        ancestors.push(ancestorRequest.requestId);
-        ancestorRequest = ancestorRequest.initiatorRequest;
-      }
-
-      // With the above array we can work from back to front, i.e. A, B, C, and during this process
-      // we can build out the tree for any nodes that have yet to be created.
-      let ancestor = ancestors.pop();
-      while (ancestor && node) {
-        const parentRequest = requestIdToRequests.get(ancestor);
-        if (!parentRequest) {
-          throw new Error(`request with id ${ancestor} not found.`);
-        }
-
-        const parentRequestId = parentRequest.requestId;
-        if (!node[parentRequestId]) {
-          node[parentRequestId] = {
-            request: parentRequest,
+    const rootNode = {};
+
+    /**
+     * @param {LH.Artifacts.NetworkRequest[]} path
+     */
+    function addChain(path) {
+      let currentNode = rootNode;
+
+      for (const record of path) {
+        if (!currentNode[record.requestId]) {
+          currentNode[record.requestId] = {
+            request: record,
             children: {},
           };
         }
 
-        // Step to the next iteration.
-        ancestor = ancestors.pop();
-        node = node[parentRequestId].children;
+        currentNode = currentNode[record.requestId].children;
       }
+    }
 
-      if (!node) {
-        continue;
-      }
+    // By default `traverse` will discover nodes in BFS-order regardless of dependencies, but
+    // here we need traversal in a topological sort order. We'll visit a node only when its
+    // dependencies have been met.
+    const seenNodes = new Set();
+    /** @param {LH.Gatherer.Simulation.GraphNode} node */
+    function getNextNodes(node) {
+      return node.getDependents().filter(n => n.getDependencies().every(d => seenNodes.has(d)));
+    }
 
-      // If the node already exists, bail.
-      if (node[request.requestId]) {
-        continue;
-      }
+    graph.traverse((node, traversalPath) => {
+      seenNodes.add(node);
+      if (node.type !== 'network') return;
+      if (!CriticalRequestChains.isCritical(node.record, mainResource)) return;
 
-      // node should now point to the immediate parent for this request.
-      node[request.requestId] = {
-        request,
-        children: {},
-      };
-    }
+      const networkPath = traversalPath
+        .filter(/** @return {n is LH.Gatherer.Simulation.GraphNetworkNode} */
+          n => n.type === 'network')
+        .reverse()
+        .map(node => node.record);
+
+      // Ignore if some ancestor is not a critical request.
+      if (networkPath.some(r => !CriticalRequestChains.isCritical(r, mainResource))) return;
 
-    return criticalRequestChains;
+      addChain(networkPath);
+    }, getNextNodes);
+
+    return rootNode;
   }
 
   /**
-   * @param {{URL: LH.Artifacts['URL'], devtoolsLog: LH.DevtoolsLog}} data
+   * @param {{URL: LH.Artifacts['URL'], devtoolsLog: LH.DevtoolsLog, trace: LH.Trace}} data
    * @param {LH.Audit.Context} context
    * @return {Promise<LH.Artifacts.CriticalRequestNode>}
    */
   static async compute_(data, context) {
-    const [networkRecords, mainResource] = await Promise.all([
-      NetworkRecords.request(data.devtoolsLog, context),
-      MainResource.request(data, context),
-    ]);
+    const mainResource = await MainResource.request({
+      URL: data.URL,
+      devtoolsLog: data.devtoolsLog,
+    }, context);
+
+    const graph = await PageDependencyGraph.request({
+      trace: data.trace,
+      devtoolsLog: data.devtoolsLog,
+    }, context);
 
-    return CriticalRequestChains.extractChain(networkRecords, mainResource);
+    return CriticalRequestChains.extractChainsFromGraph(mainResource, graph);
   }
 }
 

lighthouse-core/lib/network-recorder.js

@@ -333,7 +333,7 @@ class NetworkRecorder extends EventEmitter {
    * @return {NetworkRequest|null}
    * @private
    */
-  static _chooseInitiator(record, recordsByURL) {
+  static _chooseInitiatorRequest(record, recordsByURL) {
     if (record.redirectSource) {
       return record.redirectSource;
     }
@@ -393,11 +393,11 @@ class NetworkRecorder extends EventEmitter {
       recordsByURL.set(record.url, records);
     }
 
-    // set the initiator and redirects array
+    // set the initiatorRequest and redirects array
     for (const record of records) {
-      const initiator = NetworkRecorder._chooseInitiator(record, recordsByURL);
-      if (initiator) {
-        record.setInitiatorRequest(initiator);
+      const initiatorRequest = NetworkRecorder._chooseInitiatorRequest(record, recordsByURL);
+      if (initiatorRequest) {
+        record.setInitiatorRequest(initiatorRequest);
       }
 
       let finalRecord = record;

lighthouse-core/lib/network-request.js

@@ -145,10 +145,10 @@ class NetworkRequest {
   }
 
   /**
-   * @param {NetworkRequest} initiator
+   * @param {NetworkRequest} initiatorRequest
    */
-  setInitiatorRequest(initiator) {
-    this.initiatorRequest = initiator;
+  setInitiatorRequest(initiatorRequest) {
+    this.initiatorRequest = initiatorRequest;
   }
 
   /**

lighthouse-core/test/audits/critical-request-chains-test.js

@@ -10,6 +10,7 @@
 const CriticalRequestChains = require('../../audits/critical-request-chains.js');
 const redditDevtoolsLog = require('../fixtures/artifacts/perflog/defaultPass.devtoolslog.json');
 const assert = require('assert').strict;
+const createTestTrace = require('../create-test-trace.js');
 const networkRecordsToDevtoolsLog = require('../network-records-to-devtools-log.js');
 
 const FAILING_CHAIN_RECORDS = [
@@ -66,10 +67,14 @@ const PASSING_CHAIN_RECORDS_2 = [
 const EMPTY_CHAIN_RECORDS = [];
 
 const mockArtifacts = (chainNetworkRecords) => {
+  const trace = createTestTrace({topLevelTasks: [{ts: 0}]});
   const devtoolsLog = networkRecordsToDevtoolsLog(chainNetworkRecords);
   const finalUrl = chainNetworkRecords[0] ? chainNetworkRecords[0].url : 'https://example.com';
 
   return {
+    traces: {
+      [CriticalRequestChains.DEFAULT_PASS]: trace,
+    },
     devtoolsLogs: {
       [CriticalRequestChains.DEFAULT_PASS]: devtoolsLog,
     },
@@ -100,6 +105,7 @@ describe('Performance: critical-request-chains audit', () => {
 
   it('calculates the correct chain result for a real trace', () => {
     const artifacts = {
+      traces: {defaultPass: createTestTrace({topLevelTasks: [{ts: 0}]})},
       devtoolsLogs: {defaultPass: redditDevtoolsLog},
       URL: {finalUrl: 'https://www.reddit.com/r/nba'},
     };