perf: consolidate DBW pass into defaultPass #2160

patrickhulce · 2017-05-05T17:57:05Z

in the name of Operation Yaquina Bay (#2146), closes #1737

shaves off ~40s on heavy sites like cnn.com (latest run when combined with other changes for me now completes in <1 minute 🎉 )

Changes:

Converts event-listeners, document.write, and permission requests on load to use the violation entries coming from the protocol instead of bespoke in-page execution
Deletes the corresponding gatherers associated with them
Moves all remaining audits into the performance pass since they all have afterPass entries only
Deletes no-console-time and no-datenow since they monkeypatch functions with capture callsites
Convert tags-blocking-first-paint's beforePass false positive detection into other checks using isLinkPreload flag and ensuring that the request actually finished before FCP.

Caveats:

Violations don't seem to be coming through for mutation event listeners yet, will follow up with @pavelfeldman
Fancier stuff that got removed could potentially be enabled during the perf pass depending on how heavy the callsite capturing is, but further testing is needed
smoke tests already pass, but unit tests will need to be updated if this gets cursory 👍 :)

patrickhulce · 2017-05-09T16:11:30Z

bueller...bueller...

paulirish

I went through the gatherers added to defaultPass and verified they only run in afterPass and audited their driver usage.

The speed improvement is real and I'm excited to see this happen. 👍

paulirish · 2017-05-09T17:52:39Z

lighthouse-core/audits/audit.js

+   * @param {!RegExp} pattern
+   * @return {!Array}
+   */
+  static getViolationResults(artifacts, pattern) {


technically this should be in a violation-audit.js file that others subclass, right?

kind of an abuse to put in on the general audit file.

paulirish · 2017-05-09T18:00:22Z

lighthouse-core/gather/gatherers/dobetterweb/all-event-listeners.js

@@ -118,18 +118,16 @@ class EventListeners extends Gatherer {
    })).then(nestedListeners => [].concat(...nestedListeners));
  }

-  beforePass(options) {


this change fixes the 2nd half of #2163

and doing it this way means we don't need pavel's CL

\o/

patrick, you should followup with pavel separately if you'd rather migrate to violations vs this approach. but this can be post-IO

I have no strong opinions, are the mutation listeners bad enough that we should put them in everyone's console or just check for in LH? basically, is there a legitimate reason to still use them?

basically, is there a legitimate reason to still use them?

no. you can use mutation observers. but the events are from a dark time.
only really shitty legacy code should be using them. i hope.

alright cool then violation sgtm

paulirish · 2017-05-09T18:03:27Z

lighthouse-core/gather/gatherers/dobetterweb/all-event-listeners.js

+    this._parsedScripts = new Map();
+    return Promise.resolve()
+      .then(() => this.listenForScriptParsedEvents())
+      .then(() => this.unlistenForScriptParsedEvents())
      .then(_ => options.driver.querySelectorAll('body, body /deep/ *')) // drill into shadow trees


if you want to fix the 1st half of #2163
we're awfully close to it right now.

i'm also fine with it coming in separately.

+1 to separate there's already a lot going on here

paulirish · 2017-05-09T18:08:00Z

lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js

@@ -104,6 +86,7 @@ function filteredAndIndexedByUrl(networkRecords) {
    // Include 404 scripts/links generated by the parser because they are likely blocking.
    if (isHtml || isParserScriptOrStyle || (isFailedRequest && isParserGenerated)) {
      prev[record._url] = {
+        isLinkPreload: record.isLinkPreload,


this is a little new.

CL: https://codereview.chromium.org/2751023005
cr rev: 460094

it's not yet in stable chrome (58), but its in 59.

how do you want to handle stable?

IMO it's not the end of the world to have it fire false positives for a while since there's now a mitigation that won't fire if it didn't actually occur before the paint

paulirish · 2017-05-09T18:22:02Z

lighthouse-core/config/default.js

@@ -121,8 +109,6 @@ module.exports = {
    "dobetterweb/external-anchors-use-rel-noopener",
    "dobetterweb/geolocation-on-start",
    "dobetterweb/link-blocking-first-paint",
-    "dobetterweb/no-console-time",
-    "dobetterweb/no-datenow",
    "dobetterweb/no-document-write",
    "dobetterweb/no-mutation-events",


im seeing this:

related to your change?

probably, we won't be getting the same level of detail now that we aren't listening the entire time. strange that it shows up when there are no entries though...

paulirish · 2017-05-09T18:23:16Z

lighthouse-core/config/default.js

-    "gatherers": [
-      "chrome-console-messages",
-      // "styles",
-      // "css-usage",


lets keep the commented out css-usage? :)

ha, alright, it's more dangerous now though since uncommenting could really mess up everything in the perf pass

paulirish · 2017-05-09T18:28:29Z

lighthouse-core/config/default.js

+      "dobetterweb/anchors-with-no-rel-noopener",
+      "dobetterweb/appcache",
+      "dobetterweb/domstats",
+      "dobetterweb/optimized-images",


in this gatherer, we flip Network domain on and off.

i'm not sure why though. I suppose its for the 'Network.getResponseBody' we do. there's a chance this method will work without network being enabled. lets doublecheck that? looking at response-compression.js, it appears to get response bodies while network is disabled:

(also driver.getRequestContent exists now which it should probably use?)

it could be a problem because we may allow more network events into the devtoolsLog than what we intended. but yeah... if we dont need to enable/disable in there then let's remove.

we don't need network here anymore after #2154

sg. do you want to remove the network.enable/disable stuff in that gatherer now, then?

paulirish · 2017-05-09T18:39:40Z

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

-          }
-        }
-      },
+      // 'no-mutation-events': {


add a comment about why its excluded right now?

doesn't need to be anymore I removed

paulirish · 2017-05-09T18:40:34Z

lighthouse-core/audits/dobetterweb/link-blocking-first-paint.js

    };
  }

  /**
   * @param {!Artifacts} artifacts
   * @param {string} tagFilter The tagName to filter on
+   * @param {number=} endedBefore The trace milisecond timestamp that offending tags must have ended
+   *    before (typically first contentful paint).
   * @param {number=} loadThreshold Filter to resources that took at least this


loadDurationThreshold

paulirish · 2017-05-09T18:41:01Z

lighthouse-core/audits/dobetterweb/link-blocking-first-paint.js

    };
  }

  /**
   * @param {!Artifacts} artifacts
   * @param {string} tagFilter The tagName to filter on
+   * @param {number=} endedBefore The trace milisecond timestamp that offending tags must have ended


endTimeMax

need a push?

ebidel · 2017-05-29T16:24:58Z

lighthouse-core/audits/dobetterweb/no-console-time.js

-/**
- * @license
- * Copyright 2016 Google Inc. All rights reserved.
- *


why were these audits removed!?

@patrickhulce

@ebidel

They were the only audits left that had perf impact and would need their own pass. Somewhere there was also a thread with @brendankenny on the dangers of letting these run during the perf pass which is also why I recommended not enabling them until we proved they had minimal impact.

From the PR description:

Deletes no-console-time and no-datenow since they monkeypatch functions with capture callsites
Fancier stuff that got removed could potentially be enabled during the perf pass depending on how heavy the callsite capturing is, but further testing is needed

ebidel · 2017-05-30T17:06:51Z

Right but can't we keep the code around for people to run themselves?

…

On Tue, May 30, 2017, 9:35 AM Patrick Hulce ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lighthouse-core/audits/dobetterweb/no-console-time.js <#2160 (comment)> : > @@ -1,82 +0,0 @@ -/** - * @license - * Copyright 2016 Google Inc. All rights reserved. - * @ebidel <https://github.com/ebidel> They were the only audits left that had perf impact and would need their own pass. Somewhere there was also a thread with @brendankenny <https://github.com/brendankenny> on the dangers of letting these run during the perf pass which is also why I recommended not enabling them until we proved they had minimal impact. From the PR description: Deletes no-console-time and no-datenow since they monkeypatch functions with capture callsites Fancier stuff that got removed could potentially be enabled during the perf pass depending on how heavy the callsite capturing is, but further testing is needed — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2160 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOigDX2ps5IqF_EU4oPNtRfxkcT_Z-dks5r_EU6gaJpZM4NSOyM> .

patrickhulce · 2017-05-30T17:09:08Z

Right but can't we keep the code around for people to run themselves?

Oh sure I have no qualms with that 👍 I'll defer to @brendankenny who expressed disgruntlement at the time when I wanted to keep code around for unused audits :)

patrickhulce added 2 commits May 5, 2017 10:46

perf: consolidate DBW pass into defaultPass

ec9e49a

misc

67f1530

paulirish mentioned this pull request May 5, 2017

[meta] I/O The Final Countdown #2161

Closed

40 tasks

paulirish added the OYB label May 5, 2017

revert mutation events change for now

03bd9c4

paulirish requested changes May 9, 2017

View reviewed changes

patrickhulce added 5 commits May 9, 2017 13:09

feedback

ae75237

delete unused

ba03531

update tests

8f1c016

Merge branch 'master' into consolidate_dbw_pass

10f7619

more test updates

4a0b672

paulirish approved these changes May 10, 2017

View reviewed changes

paulirish merged commit d99778b into master May 10, 2017

paulirish deleted the consolidate_dbw_pass branch May 10, 2017 00:24

patrickhulce mentioned this pull request May 10, 2017

followup: makes tests pass #2199

Merged

ebidel reviewed May 29, 2017

View reviewed changes

antross mentioned this pull request Feb 20, 2019

Documentation on website doesn't seem to be udpated to v4 #7282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: consolidate DBW pass into defaultPass #2160

perf: consolidate DBW pass into defaultPass #2160

patrickhulce commented May 5, 2017 •

edited

Loading

patrickhulce commented May 9, 2017

paulirish left a comment

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 10, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 10, 2017

ebidel May 29, 2017

ebidel May 29, 2017

patrickhulce May 30, 2017

ebidel commented May 30, 2017 via email

patrickhulce commented May 30, 2017

perf: consolidate DBW pass into defaultPass #2160

perf: consolidate DBW pass into defaultPass #2160

Conversation

patrickhulce commented May 5, 2017 • edited Loading

patrickhulce commented May 9, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented May 30, 2017 via email

patrickhulce commented May 30, 2017

patrickhulce commented May 5, 2017 •

edited

Loading