From 3273e129a002b6512bd46752ffe57f13be7835b2 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 23 Aug 2018 17:02:30 -0700 Subject: [PATCH 1/2] core(preload): remove CRC dependency --- lighthouse-core/audits/uses-rel-preload.js | 63 +++--- .../test/audits/uses-rel-preload-test.js | 186 ++++++------------ 2 files changed, 104 insertions(+), 145 deletions(-) diff --git a/lighthouse-core/audits/uses-rel-preload.js b/lighthouse-core/audits/uses-rel-preload.js index 9c4bc48b717b..2d0fef1b0112 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 = { @@ -70,16 +71,49 @@ class UsesRelPreloadAudit extends Audit { } /** + * @param {LH.Artifacts.NetworkRequest} mainResource + * @param {LH.Gatherer.Simulation.GraphNode} graph + * @return {Set} + */ + static getURLsToPreload(mainResource, graph) { + /** @type {Set} */ + const urls = new Set(); + + 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 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} 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 +202,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} */ - 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); diff --git a/lighthouse-core/test/audits/uses-rel-preload-test.js b/lighthouse-core/test/audits/uses-rel-preload-test.js index ab41e062d269..9b0ce4889404 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); }); From 596d8fe1b3b8b0bf893878f0007e2f9e59e3dae2 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 23 Aug 2018 17:08:42 -0700 Subject: [PATCH 2/2] remove flatten too --- lighthouse-core/audits/uses-rel-preload.js | 33 ---------------------- 1 file changed, 33 deletions(-) diff --git a/lighthouse-core/audits/uses-rel-preload.js b/lighthouse-core/audits/uses-rel-preload.js index 2d0fef1b0112..2ec6cb3743e4 100644 --- a/lighthouse-core/audits/uses-rel-preload.js +++ b/lighthouse-core/audits/uses-rel-preload.js @@ -37,39 +37,6 @@ class UsesRelPreloadAudit extends Audit { }; } - /** - * @param {LH.Artifacts.CriticalRequestNode} chains - * @param {number} maxLevel - * @param {number=} minLevel - */ - static _flattenRequests(chains, maxLevel, minLevel = 0) { - /** @type {Array} */ - 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); - } - } - }); - }; - - flatten(chains, 0); - - return requests; - } - /** * @param {LH.Artifacts.NetworkRequest} mainResource * @param {LH.Gatherer.Simulation.GraphNode} graph