new_audit(preconnect): add preconnect audit #4362

wardpeet · 2018-01-25T23:49:24Z

Filter same origin
Filter all records requested by main resource
Filter all non http(s) records (urls with no origin)
Filter out allready connected origins (connection time is 0)
Filter out all records that took place after 10s
Added unit tests
Added smoketests

paulirish · 2018-01-31T17:45:28Z

exclude same-origin (as preload already handles them Preload audit improvements #4425)
exclude origins @ depth === 1 because we already connected to them

paulirish

basically let's do the first half of #4425 and then the two checkboxes below (exclude same origin, and depth===1). Then we're mostly good here.

wardpeet · 2018-03-22T23:55:13Z

lighthouse-core/audits/uses-rel-preconnect.js

+        const requestDelay = record._startTime - mainResource._endTime;
+        const recordOrigin = URL.getOrigin(record.url);
+
+        if (preconnectResults[recordOrigin]) {


better way to find unique origins?

IMO use a Set for preconnectResults instead

yeah I might flip this around a bit ward

create a set of all origins

filter out the bad origins

for each origin, find the earliest record that meets the other criteria

paulirish · 2018-03-23T00:01:04Z

lighthouse-core/audits/uses-rel-preconnect.js

+        const requestDelay = record._startTime - mainResource._endTime;
+        const recordOrigin = URL.getOrigin(record.url);
+
+        if (preconnectResults[recordOrigin]) {


IMO use a Set for preconnectResults instead

paulirish · 2018-03-23T00:01:21Z

lighthouse-core/audits/uses-rel-preconnect.js

+      // filter out all resources that are not loaded by the document
+      .filter(record => {
+        return record.initiatorRequest() !== mainResource && record !== mainResource;
+      // filter out urls that do not have an origin


seems like L62 would already have taken care of these.

paulirish · 2018-03-23T00:05:37Z

lighthouse-core/audits/uses-rel-preconnect.js

+      // filter out urls that do not have an origin
+      }).filter(record => {
+        return !!URL.getOrigin(record.url);
+      // filter out all resources where origins are already resolved


since we're only displaying origins and not full request URLs, I feel like this is already taken care of by reduction down to unique origins below.

(The assumption being that connect is always a cost for new origins... which seems a reasonable assumption. right @patrickhulce ?)

yeah but he's trying to find the first representative network record for the origin so you do need to filter out the network records that were already connected

paulirish · 2018-03-23T00:08:33Z

lighthouse-core/audits/uses-rel-preconnect.js

+const Util = require('../report/v2/renderer/util');
+const UnusedBytes = require('./byte-efficiency/byte-efficiency-audit');
+const URL = require('../lib/url-shim');
+const PRECONNECT_SOCKET_MAX_IDLE = 10;


Can you add some comments?

Preconnect establishes a "clean" socket. Chrome's socket manager will keep an unused socket
around for 10s. Meaning, the time delta between processing preconnect a request should be <10s, otherwise it's wasted

also I think we could actually use 12-15 for our threshold, tbh. we'd include slightly more connections and there's a decent chance in real loads, the preconnect would be used.

paulirish · 2018-03-23T00:11:06Z

lighthouse-core/audits/uses-rel-preconnect.js

+        }
+
+        // make sure the requests are below the PRECONNECT_SOCKET_MAX_IDLE (10s) mark
+        if (Math.max(0, requestDelay) < PRECONNECT_SOCKET_MAX_IDLE) {


style wise it'd probably be better to check the inverse and return, like the other filters.

that way at the end we take everything that made it through the gauntlet and add to the set.

paulirish · 2018-03-23T00:11:34Z

lighthouse-core/audits/uses-rel-preconnect.js

+    });
+
+    const headings = [
+      {key: 'url', itemType: 'url', text: 'URL'},


text: 'Origin'

based on your screenshot i'd say our url renderer is a little awkward in handling this origin-only URLs.

let's fix that. :)
we can check for pathname of / and if that's the case, not separate the two.... probably want most of that in Util.parseURL and small adjustments in the _renderTextURL

patrickhulce

nice work! excited to get this one in 👍

patrickhulce · 2018-03-23T00:19:29Z

lighthouse-core/audits/uses-rel-preconnect.js

+        ' before an HTTP request is actually sent to the server. This will reduce multiple, ' +
+        `costly round trips to any origin. [Learn more](${learnMoreUrl}).`,
+      requiredArtifacts: ['devtoolsLogs'],
+      scoringMode: Audit.SCORING_MODES.NUMERIC,


scoring display mode

patrickhulce · 2018-03-23T00:19:38Z

lighthouse-core/audits/uses-rel-preconnect.js

+   */
+  static get meta() {
+    return {
+      category: 'Performance',


no more category :)

patrickhulce · 2018-03-23T00:20:44Z

lighthouse-core/audits/uses-rel-preconnect.js

+        return !!URL.getOrigin(record.url);
+      // filter out all resources where origins are already resolved
+      }).filter(record => {
+        return !UsesRelPreconnectAudit.hasAlreadyConnectedToOrigin(record);


might want to explicitly call out invalid timing information as well, i.e. if dnsStart/etc are not finite positive numbers

call out? Do you mean log to sentry?

oh sorry, call out just in a comment with an extra check :)

I meant, "let's also filter requests with invalid timing information"

patrickhulce · 2018-03-23T00:21:28Z

lighthouse-core/audits/uses-rel-preconnect.js

+      // filter out urls that do not have an origin
+      }).filter(record => {
+        return !!URL.getOrigin(record.url);
+      // filter out all resources where origins are already resolved


yeah but he's trying to find the first representative network record for the origin so you do need to filter out the network records that were already connected

patrickhulce · 2018-03-23T00:22:25Z

lighthouse-core/audits/uses-rel-preconnect.js

+      });
+
+    const results = Object.values(preconnectResults).map(record => {
+      const wastedMs = record.timing.connectEnd - record.timing.dnsStart;


we'll want to Math.min this with the delta between connectStart and main document resource end

patrickhulce · 2018-03-23T00:24:03Z

lighthouse-core/audits/uses-rel-preconnect.js

+        const requestDelay = record._startTime - mainResource._endTime;
+        const recordOrigin = URL.getOrigin(record.url);
+
+        if (preconnectResults[recordOrigin]) {


yeah I might flip this around a bit ward

create a set of all origins

filter out the bad origins

for each origin, find the earliest record that meets the other criteria

wardpeet · 2018-03-25T23:18:54Z

@paulirish @patrickhulce
I think I have all your review comments done, not sure if I implemented them like you guys wanted 🙄.

i'm not really sure about the displayUrl helpers though..

Also smoketests are failing on travis as it's such a low number. Could this be a docker issue? that host is has these origins cached, should I just check for items than?

patrickhulce

review #️⃣2️⃣!

patrickhulce · 2018-03-26T20:25:57Z

lighthouse-core/audits/uses-rel-preconnect.js

+      description: 'Avoid multiple, costly round trips to any origin',
+      informative: true,
+      helpText:
+        'Consider using<link rel="preconnect dns-prefetch"> to set up early connections ' +


@paulirish may have opinions here too, but WDYT about?

Consider adding preconnect or dns-prefetch resource hints to establish early connections to important third-party origins. Learn more.

fine by me i most of the time just copy what you guys say 😄 but I think <link tags shouldn't be in the description like you describe

patrickhulce · 2018-03-26T20:27:48Z

lighthouse-core/audits/uses-rel-preconnect.js

+
+    const origins = networkRecords
+      .filter(record => {
+        // filter out all resources that have the same origin


nit: move this comment down a line like the others :)

patrickhulce · 2018-03-26T20:30:13Z

lighthouse-core/audits/uses-rel-preconnect.js

+    const results = [];
+    preconnectOrigins.forEach(origin => {
+      const records = networkRecords.filter(record => URL.getOrigin(record.url) === origin);
+      if (!records.length) {


this seems impossible, you didn't run across this IRL did you?

seems like the same checks from above also belong in this spot though?

patrickhulce · 2018-03-26T20:31:07Z

lighthouse-core/audits/uses-rel-preconnect.js

+
+      // Sometimes requests are done simultaneous and the connection has not been made
+      // chrome will try to connect for each network record, we get the first record
+      let firstRecordOfOrigin;


@paulirish is this an acceptable occasion for reduce in your 👀 ? :)

this works too

patrickhulce · 2018-03-26T20:34:17Z

lighthouse-core/audits/uses-rel-preconnect.js

+        firstRecordOfOrigin._startTime * 1000 -
+        mainResource._endTime * 1000 +
+        firstRecordOfOrigin.timing.connectStart;
+      // calculate delta between connectionTime and timeToConnectionStart from main resource


this comment is a bit confusing, didn't we already compute the delta and the line below is ensuring we just cap the connectionTime savings?

patrickhulce · 2018-03-26T20:35:01Z

lighthouse-core/report/v2/renderer/crc-details-renderer.js

@@ -110,7 +110,7 @@ class CriticalRequestChainRenderer {
    const {file, hostname} = Util.parseURL(segment.node.request.url);
    const treevalEl = dom.find('.crc-node__tree-value', chainsEl);
    dom.find('.crc-node__tree-file', treevalEl).textContent = `${file}`;
-    dom.find('.crc-node__tree-hostname', treevalEl).textContent = `(${hostname})`;
+    dom.find('.crc-node__tree-hostname', treevalEl).textContent = hostname ? `(${hostname})` : '';


nice find :) if it doesn't have a hostname though should we be showing it? what type of request is that, data URI?

no it comes back from the parseUrl where a request might be the root so I don't return a hostname.

it actually looks weird so I have to do the origin display inside the report and not in the parseUrl

Updated to
critical request chain:

preconnect table:

patrickhulce · 2018-03-26T20:38:44Z

lighthouse-core/test/audits/users-rel-preconnect-test.js

+  url: 'https://www.example.com/',
+  _endTime: 1,
+};
+const mainResourceRecord = {


why do these two need to be different? :)

good question 😛

patrickhulce · 2018-03-26T20:41:20Z

lighthouse-core/test/audits/users-rel-preconnect-test.js

+    assert.equal(extendedInfo.value.length, 2);
+    assert.deepStrictEqual(extendedInfo.value, [
+      {url: 'https://cdn.example.com', wastedMs: Util.formatMilliseconds(200)},
+      {url: 'https://othercdn.example.com', wastedMs: Util.formatMilliseconds(400)},


would you mind explaining this result? seems like it should be min of 500 (600 - 100) and 300 (1000 * (1.2 - 1) + 100)

when looking in code I use connectStart where I should have used dnsStart 🤦‍♂️

const connectionTime = 600 - 100; const timeBetweenMainResourceAndConnectStart = 1.2 * 1000 - 1 * 1000 + 200; // calculate delta between connectionTime and timeToConnectionStart from main resource const wastedMs = Math.min(500, 400);

ah yeah I missed that too :) seems like timeBetweenMainResourceAndDnsStart should fix it 👍 (or fallback to connect start I guess if dns start is invalid, is that a real case?)

patrickhulce

few more comments sorry for the delay!

patrickhulce · 2018-04-10T22:07:05Z

lighthouse-cli/test/fixtures/perf/preload_tester.js

@@ -7,3 +7,6 @@
 /* eslint-disable */

 document.write('<script src="level-2.js?delay=500"></script>')
+
+// load another origin
+fetch('http://localhost:10503/preload.html', () => {});


nit: do we even need the callback?

not sure why I even added it as it returns a promise

patrickhulce · 2018-04-10T22:07:38Z

lighthouse-core/audits/uses-rel-preconnect.js

+const PRECONNECT_SOCKET_MAX_IDLE = 15;
+
+const learnMoreUrl =
+  'https://developers.google.com/web/fundamentals/performance/resource-prioritization#preconnect';


as @brendankenny pointed out to me we can throw this inline without violating our max-len since it's a URL 👍

patrickhulce · 2018-04-10T22:10:13Z

lighthouse-core/audits/uses-rel-preconnect.js

+      .filter(record => {
+        return (
+          // filter out all resources that have the same origin
+          !URL.originsMatch(mainResource.url, record.url) &&


IIRC new URL is actually pretty expensive and can take seconds in the worst case, so let's reorder for some cheaper ones first? maybe just sticking initiator request and hasAlreadyConnected first is enough

should we use webinspector parsedurl instead?
mainResource.parsedURL.securityOrigin() === record.parsedURL.securityOrigin()

yeah even better! 👍

patrickhulce · 2018-04-10T22:14:37Z

lighthouse-core/audits/uses-rel-preconnect.js

+          // filter out urls that do not have an origin (data, ...)
+          !!URL.getOrigin(record.url) &&
+          // filter out all resources where origins are already resolved
+          !UsesRelPreconnectAudit.hasAlreadyConnectedToOrigin(record) &&


seems like we'll need a corresponding filter to make sure the timing information is valid

static hasValidTiming(record) { return record.timing && record.timing.connectEnd > 0 && record.timing.connectStart > 0; } // filter out all resources where timing info was invalid !UsesRelPreconnect...

in most cases it'll be the same, but we've seen some weird edge cases in other environments where timing is just nonsense negative numbers or undefined

patrickhulce · 2018-04-10T22:16:09Z

lighthouse-core/audits/uses-rel-preconnect.js

+      maxWasted = Math.max(wastedMs, maxWasted);
+      results.push({
+        url: new URL(firstRecordOfOrigin.url).origin,
+        wastedMs: Util.formatMilliseconds(wastedMs),


let's keep this a number and mark the wastedMs as a type: 'ms'

{key: 'wastedMs', itemType: 'ms', granularity: 1, text: 'Potential Savings'}

patrickhulce · 2018-04-10T22:17:41Z

lighthouse-core/report/v2/renderer/details-renderer.js

@@ -96,8 +96,8 @@ class DetailsRenderer {
    let title;
    try {
      const parsed = Util.parseURL(url);
-      displayedPath = parsed.file;
-      displayedHost = `(${parsed.hostname})`;
+      displayedPath = parsed.file === '/' ? parsed.origin :parsed.file;


nit: space after colon : parsed.file

outdated.

wardpeet · 2018-04-12T19:35:26Z

@patrickhulce @paulirish @brendankenny
fixed all comments

patrickhulce

LGTM % nits! nice work @wardpeet!

patrickhulce · 2018-04-12T21:49:12Z

lighthouse-core/audits/uses-rel-preconnect.js

+    const preconnectOrigins = new Set(origins);
+    const results = [];
+    preconnectOrigins.forEach(origin => {
+      const records = networkRecords.filter(record => URL.getOrigin(record.url) === origin);


I think we still want to double check that we're only looking at records with valid timing, also let's use parsedURL.securityOrigin to be consistent with above
could probably combine this with the forEach below too and do it in one pass :)

patrickhulce · 2018-04-12T22:08:34Z

lighthouse-core/report/v2/renderer/util.js

-        name = name.slice(0, MAX_LENGTH - 1 - (name.length - dotIndex)) +
-            // Show file extension
-            `${ELLIPSIS}${name.slice(dotIndex)}`;
+        name =


think we can revert some of this diff noise?

paulirish · 2018-04-25T18:59:08Z

btw for whatever reason 2/3rds of my last review comments are collapsed but they are unaddressed so far. Probably because I commented on that specific commit, and there was one following it. shrug.

paulirish · 2018-04-26T19:45:36Z

lgtm. ready to merge when green.

addyosmani · 2018-04-26T22:10:23Z

\o/ Thanks for the diligent reviews getting this audit firmed up @paulirish @patrickhulce and @brendankenny. Big thanks for all your work getting this audit landed, @wardpeet high-fives

…gleChrome#4362)

devtools-bot added the waiting4reviewer label Jan 25, 2018

wardpeet added new_audit chillin ❄ labels Jan 25, 2018

wardpeet force-pushed the feature/preconnect branch from 0eb4516 to c2de538 Compare January 25, 2018 23:54

paulirish mentioned this pull request Mar 7, 2018

audit: multiple, costly round trips to any origin 🚙 (preconnect) #3106

Closed

paulirish requested changes Mar 7, 2018

View reviewed changes

addyosmani added this to the Sprint Once: March 12 - 23 milestone Mar 16, 2018

wardpeet force-pushed the feature/preconnect branch from c2de538 to 83c1ba1 Compare March 22, 2018 23:52

wardpeet requested review from brendankenny and patrickhulce as code owners March 22, 2018 23:52

wardpeet changed the title ~~WIP: new-audit(preconnect): Add preconnect audit~~ new-audit(preconnect): Add preconnect audit Mar 22, 2018

wardpeet changed the title ~~new-audit(preconnect): Add preconnect audit~~ new-audit(preconnect): add preconnect audit Mar 22, 2018

wardpeet removed the chillin ❄ label Mar 22, 2018

wardpeet commented Mar 22, 2018

View reviewed changes

paulirish previously requested changes Mar 23, 2018

View reviewed changes

patrickhulce suggested changes Mar 23, 2018

View reviewed changes

wardpeet requested a review from vinamratasingal-zz as a code owner March 25, 2018 21:55

paulirish modified the milestones: Sprint Once: March 12 - 23, Sprint Doce: Mar 26 - April 6 Mar 26, 2018

patrickhulce reviewed Mar 26, 2018

View reviewed changes

wardpeet force-pushed the feature/preconnect branch from 93fc3a7 to ab9137c Compare April 10, 2018 21:07

paulirish removed the request for review from vinamratasingal-zz April 10, 2018 21:27

patrickhulce suggested changes Apr 10, 2018

View reviewed changes

GoogleChrome deleted a comment from patrickhulce Apr 12, 2018

patrickhulce approved these changes Apr 12, 2018

View reviewed changes

wardpeet force-pushed the feature/preconnect branch from a5fb719 to ce26b99 Compare April 15, 2018 22:26

wardpeet added 15 commits April 25, 2018 19:28

Add smoketests

2956c81

Better support for origins

8e5d6e1

Update prettier & eslint

4895e7f

calculate delta between main resource and connection start

40de4d1

fix tests

5ad8505

Review changes

801b262

Fix url rendering in report

7e173ed

Review changes

764cf42

revert noise

2e38e8a

Convet URL.getOrigin to parsedURL.securityOrigin()

1b3e281

Sort by wastedms

0bcf72d

Fix travis

cfaff7d

Review changes

990a69a

Setup basic TS for preconnect

5c8bd4f

remove unused url

07bf3c0

wardpeet force-pushed the feature/preconnect branch from a7459fe to 07bf3c0 Compare April 25, 2018 17:29

paulirish changed the title ~~new-audit(preconnect): add preconnect audit~~ new_audit(preconnect): add preconnect audit Apr 25, 2018

paulirish added waiting4committer and removed waiting4reviewer labels Apr 25, 2018

devtools-bot added waiting4reviewer and removed waiting4committer labels Apr 25, 2018

wardpeet added 2 commits April 26, 2018 21:29

remove securityOrigin from d.ts

d773bb3

Ready?

15209aa

paulirish merged commit 1176a63 into master Apr 26, 2018

paulirish deleted the feature/preconnect branch April 26, 2018 22:04

paulirish removed the waiting4reviewer label Apr 30, 2018

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

new_audit: add preconnect audit (avoid costly origin roundtrips) (Goo…

36bbe55

…gleChrome#4362)

new_audit(preconnect): add preconnect audit #4362

new_audit(preconnect): add preconnect audit #4362

Conversation

wardpeet commented Jan 25, 2018 • edited Loading

paulirish commented Jan 31, 2018 • edited by wardpeet Loading

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Mar 25, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Apr 12, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Apr 25, 2018

paulirish commented Apr 26, 2018

addyosmani commented Apr 26, 2018

wardpeet commented Jan 25, 2018 •

edited

Loading

paulirish commented Jan 31, 2018 •

edited by wardpeet

Loading

wardpeet Mar 27, 2018 •

edited

Loading