Repository: GoogleChrome/lighthouse
Pull request #5901 (2018-08-24 00:07:49)

core(preload): remove CRC dependency
Leftover cleanup from Lantern changes that happened while the preload PR was ongoing. Just removes the dependency on the CRC computed artifact, so that CRC can move to be based on the graph much more easily.


Patch:
diff --git a/lighthouse-core/audits/uses-rel-preload.js b/lighthouse-core/audits/uses-rel-preload.js
index 9c4bc48b7..2ec6cb374 100644
--- a/lighthouse-core/audits/uses-rel-preload.js
+++ b/lighthouse-core/audits/uses-rel-preload.js
@@ -8,6 +8,7 @@
 const URL = require('../lib/url-shim');
 const Audit = require('./audit');
 const UnusedBytes = require('./byte-efficiency/byte-efficiency-audit');
+const CriticalRequestChains = require('../gather/computed/critical-request-chains');
 const i18n = require('../lib/i18n');
 
 const UIStrings = {
@@ -37,49 +38,49 @@ class UsesRelPreloadAudit extends Audit {
   }
 
   /**
-   * @param {LH.Artifacts.CriticalRequestNode} chains
-   * @param {number} maxLevel
-   * @param {number=} minLevel
+   * @param {LH.Artifacts.NetworkRequest} mainResource
+   * @param {LH.Gatherer.Simulation.GraphNode} graph
+   * @return {Set<string>}
    */
-  static _flattenRequests(chains, maxLevel, minLevel = 0) {
-    /** @type {Array<LH.Artifacts.NetworkRequest>} */
-    const requests = [];
-
-    /**
-     * @param {LH.Artifacts.CriticalRequestNode} chains
-     * @param {number} level
-     */
-    const flatten = (chains, level) => {
-      Object.keys(chains).forEach(chain => {
-        if (chains[chain]) {
-          const currentChain = chains[chain];
-          if (level >= minLevel) {
-            requests.push(currentChain.request);
-          }
-
-          if (level < maxLevel) {
-            flatten(currentChain.children, level + 1);
-          }
-        }
-      });
-    };
+  static getURLsToPreload(mainResource, graph) {
+    /** @type {Set<string>} */
+    const urls = new Set();
 
-    flatten(chains, 0);
+    graph.traverse((node, traversalPath) => {
+      if (node.type !== 'network') return;
+      // Don't include the node itself or any CPU nodes in the initiatorPath
+      const path = traversalPath.slice(1).filter(initiator => initiator.type === 'network');
+      if (!UsesRelPreloadAudit.shouldPreloadRequest(node.record, mainResource, path)) return;
+      urls.add(node.record.url);
+    });
 
-    return requests;
+    return urls;
   }
 
   /**
+   * We want to preload all first party critical requests at depth 2.
+   * Third party requests can be tricky to know the URL ahead of time.
+   * Critical requests at depth 1 would already be identified by the browser for preloading.
+   * Critical requests deeper than depth 2 are more likely to be a case-by-case basis such that it
+   * would be a little risky to recommend blindly.
    *
    * @param {LH.Artifacts.NetworkRequest} request
    * @param {LH.Artifacts.NetworkRequest} mainResource
+   * @param {Array<LH.Gatherer.Simulation.GraphNode>} initiatorPath
    * @return {boolean}
    */
-  static shouldPreload(request, mainResource) {
-    if (request.isLinkPreload || URL.NON_NETWORK_PROTOCOLS.includes(request.protocol)) {
-      return false;
-    }
-
+  static shouldPreloadRequest(request, mainResource, initiatorPath) {
+    const mainResourceDepth = mainResource.redirects ? mainResource.redirects.length : 0;
+
+    // If it's already preloaded, no need to recommend it.
+    if (request.isLinkPreload) return false;
+    // It's not critical, don't recommend it.
+    if (!CriticalRequestChains.isCritical(request, mainResource)) return false;
+    // It's not a request loaded over the network, don't recommend it.
+    if (URL.NON_NETWORK_PROTOCOLS.includes(request.protocol)) return false;
+    // It's not at the right depth, don't recommend it.
+    if (initiatorPath.length !== mainResourceDepth + 2) return false;
+    // We survived everything else, just check that it's a first party request.
     return URL.rootDomainsMatch(request.url, mainResource.url);
   }
 
@@ -168,28 +169,13 @@ class UsesRelPreloadAudit extends Audit {
     const URL = artifacts.URL;
     const simulatorOptions = {trace, devtoolsLog, settings: context.settings};
 
-    const [critChains, mainResource, graph, simulator] = await Promise.all([
-      // TODO(phulce): eliminate dependency on CRC
-      artifacts.requestCriticalRequestChains({devtoolsLog, URL}),
+    const [mainResource, graph, simulator] = await Promise.all([
       artifacts.requestMainResource({devtoolsLog, URL}),
       artifacts.requestPageDependencyGraph({trace, devtoolsLog}),
       artifacts.requestLoadSimulator(simulatorOptions),
     ]);
 
-    // get all critical requests 2 + mainResourceIndex levels deep
-    const mainResourceIndex = mainResource.redirects ? mainResource.redirects.length : 0;
-
-    const criticalRequests = UsesRelPreloadAudit._flattenRequests(critChains,
-      3 + mainResourceIndex, 2 + mainResourceIndex);
-
-    /** @type {Set<string>} */
-    const urls = new Set();
-    for (const networkRecord of criticalRequests) {
-      if (UsesRelPreloadAudit.shouldPreload(networkRecord, mainResource)) {
-        urls.add(networkRecord.url);
-      }
-    }
-
+    const urls = UsesRelPreloadAudit.getURLsToPreload(mainResource, graph);
     const {results, wastedMs} = UsesRelPreloadAudit.computeWasteWithGraph(urls, graph, simulator);
     // sort results by wastedTime DESC
     results.sort((a, b) => b.wastedMs - a.wastedMs);


Test patch:
diff --git a/lighthouse-core/test/audits/uses-rel-preload-test.js b/lighthouse-core/test/audits/uses-rel-preload-test.js
index ab41e062d..9b0ce4889 100644
--- a/lighthouse-core/test/audits/uses-rel-preload-test.js
+++ b/lighthouse-core/test/audits/uses-rel-preload-test.js
@@ -24,13 +24,10 @@ describe('Performance: uses-rel-preload audit', () => {
   let mockGraph;
   let mockSimulator;
 
-  const mockArtifacts = (networkRecords, mockChain, mainResource = defaultMainResource) => {
+  const mockArtifacts = (networkRecords, mainResource = defaultMainResource) => {
     return {
       traces: {[UsesRelPreload.DEFAULT_PASS]: {traceEvents: []}},
       devtoolsLogs: {[UsesRelPreload.DEFAULT_PASS]: []},
-      requestCriticalRequestChains: () => {
-        return Promise.resolve(mockChain);
-      },
       requestLoadSimulator: () => mockSimulator,
       requestPageDependencyGraph: () => mockGraph,
       requestNetworkRecords: () => networkRecords,
@@ -40,21 +37,69 @@ describe('Performance: uses-rel-preload audit', () => {
     };
   };
 
-  function buildNode(requestId, url) {
-    return new NetworkNode({url, requestId});
-  }
-
   afterEach(() => {
     mockSimulator = undefined;
   });
 
   it('should suggest preload resource', () => {
-    const rootNode = buildNode(1, 'http://example.com');
-    const mainDocumentNode = buildNode(2, 'http://www.example.com:3000');
-    const scriptNode = buildNode(3, 'http://www.example.com/script.js');
-    const scriptAddedNode = buildNode(4, 'http://www.example.com/script-added.js');
-    const scriptSubNode = buildNode(5, 'http://sub.example.com/script-sub.js');
-    const scriptOtherNode = buildNode(6, 'http://otherdomain.com/script-other.js');
+    const mainResource = Object.assign({}, defaultMainResource, {
+      url: 'http://www.example.com:3000',
+      redirects: [''],
+    });
+
+    const networkRecords = [
+      {
+        requestId: '2',
+        resourceType: 'Document',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://example.com:3000',
+        redirects: [''],
+      },
+      {
+        requestId: '2:redirect',
+        resourceType: 'Document',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://www.example.com:3000',
+        redirects: [''],
+      },
+      {
+        requestId: '3',
+        resourceType: 'Script',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://www.example.com/script.js',
+      },
+      {
+        requestId: '4',
+        resourceType: 'Script',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://www.example.com/script-added.js',
+      },
+      {
+        requestId: '5',
+        resourceType: 'Script',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://sub.example.com/script-sub.js',
+      },
+      {
+        requestId: '6',
+        resourceType: 'Script',
+        priority: 'High',
+        isLinkPreload: false,
+        url: 'http://otherdomain.com/script-other.js',
+      },
+    ];
+
+    const rootNode = new NetworkNode(networkRecords[0]);
+    const mainDocumentNode = new NetworkNode(networkRecords[1]);
+    const scriptNode = new NetworkNode(networkRecords[2]);
+    const scriptAddedNode = new NetworkNode(networkRecords[3]);
+    const scriptSubNode = new NetworkNode(networkRecords[4]);
+    const scriptOtherNode = new NetworkNode(networkRecords[5]);
 
     mainDocumentNode.setIsMainDocument(true);
     mainDocumentNode.addDependency(rootNode);
@@ -101,68 +146,7 @@ describe('Performance: uses-rel-preload audit', () => {
       },
     };
 
-    const mainResource = Object.assign({}, defaultMainResource, {
-      url: 'http://www.example.com:3000',
-      redirects: [''],
-    });
-    const networkRecords = [
-      {
-        requestId: '2',
-        isLinkPreload: false,
-        url: 'http://www.example.com:3000',
-      },
-      {
-        requestId: '3',
-        isLinkPreload: false,
-        url: 'http://www.example.com/script.js',
-      },
-      {
-        requestId: '4',
-        isLinkPreload: false,
-        url: 'http://www.example.com/script-added.js',
-      },
-      {
-        requestId: '5',
-        isLinkPreload: false,
-        url: 'http://sub.example.com/script-sub.js',
-      },
-      {
-        requestId: '6',
-        isLinkPreload: false,
-        url: 'http://otherdomain.com/script-other.js',
-      },
-    ];
-
-    const chains = {
-      '1': {
-        children: {
-          '2': {
-            request: networkRecords[0],
-            children: {
-              '3': {
-                request: networkRecords[1],
-                children: {
-                  '4': {
-                    request: networkRecords[2],
-                    children: {},
-                  },
-                  '5': {
-                    request: networkRecords[3],
-                    children: {},
-                  },
-                  '6': {
-                    request: networkRecords[4],
-                    children: {},
-                  },
-                },
-              },
-            },
-          },
-        },
-      },
-    };
-
-    return UsesRelPreload.audit(mockArtifacts(networkRecords, chains, mainResource), {}).then(
+    return UsesRelPreload.audit(mockArtifacts(networkRecords, mainResource), {}).then(
       output => {
         assert.equal(output.rawValue, 1250);
         assert.equal(output.details.items.length, 2);
@@ -181,22 +165,8 @@ describe('Performance: uses-rel-preload audit', () => {
         url: 'http://www.example.com/script.js',
       },
     ];
-    const chains = {
-      '1': {
-        children: {
-          '2': {
-            children: {
-              '3': {
-                request: networkRecords[0],
-                children: {},
-              },
-            },
-          },
-        },
-      },
-    };
 
-    return UsesRelPreload.audit(mockArtifacts(networkRecords, chains), {}).then(output => {
+    return UsesRelPreload.audit(mockArtifacts(networkRecords), {}).then(output => {
       assert.equal(output.rawValue, 0);
       assert.equal(output.details.items.length, 0);
     });
@@ -211,22 +181,7 @@ describe('Performance: uses-rel-preload audit', () => {
       },
     ];
 
-    const chains = {
-      '1': {
-        children: {
-          '2': {
-            children: {
-              '3': {
-                request: networkRecords[0],
-                children: {},
-              },
-            },
-          },
-        },
-      },
-    };
-
-    return UsesRelPreload.audit(mockArtifacts(networkRecords, chains), {}).then(output => {
+    return UsesRelPreload.audit(mockArtifacts(networkRecords), {}).then(output => {
       assert.equal(output.rawValue, 0);
       assert.equal(output.details.items.length, 0);
     });
@@ -241,22 +196,7 @@ describe('Performance: uses-rel-preload audit', () => {
       },
     ];
 
-    const chains = {
-      '1': {
-        children: {
-          '2': {
-            children: {
-              '3': {
-                request: networkRecords[0],
-                children: {},
-              },
-            },
-          },
-        },
-      },
-    };
-
-    return UsesRelPreload.audit(mockArtifacts(networkRecords, chains), {}).then(output => {
+    return UsesRelPreload.audit(mockArtifacts(networkRecords), {}).then(output => {
       assert.equal(output.rawValue, 0);
       assert.equal(output.details.items.length, 0);
     });