feat: add unused javascript audit #3085

patrickhulce · 2017-08-23T01:10:02Z

fixes #1852
addresses comment from #1449 (comment)

Adds an unused-javascript audit to the full-config
Refactors the totalTransferred calculation from unused-css-rules into an estimateTransferSize method.
Changes the intermediate page from about:blank to a data URI with a blank page. Ran into another issue (http://crbug.com/757998) where navigating to about:blank is not sufficient to clear the previous page but a data URI is.
~~This has led to a inexplicable breaking of screenshots in the trace which I'll need to investigate, but at least the bulk of this can be reviewed.~~ This doesn't seem to be failing on Travis?

ebidel · 2017-08-23T01:39:50Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+
+const ByteEfficiencyAudit = require('./byte-efficiency-audit');
+
+const IGNORE_THRESHOLD_IN_BYTES = 2048;


where does this val come from? Can you add a comment if there's research?

no research it's just in line with the rest of the byte efficiency audits to not be too noisy about small things that have minimal savings (others are also 2kb or 4kb)

patrickhulce · 2017-08-28T19:17:54Z

⛰

brendankenny · 2017-08-28T22:49:36Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

+   * @param {!WebInspector.NetworkRequest} networkRecord
+   * @param {number} totalBytes
+   * @param {string=} resourceType
+   * @param {number=} compressionRatio


any point to compressionRatio as a parameter if no caller uses it?

could also do a local lookup chart based on resourceType (if we're going to do anything but 0.5)

making it controllable via the tests was a good sanity check, but I can remove if you think it's more trouble than its worth

brendankenny · 2017-08-28T22:51:56Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

@@ -58,6 +58,30 @@ class UnusedBytes extends Audit {
  }

  /**
+   * @param {!WebInspector.NetworkRequest} networkRecord


can you add a jsdoc description? It's not immediately obvious what totalBytes is here and what's being estimated from it

brendankenny · 2017-08-28T22:55:28Z

lighthouse-core/gather/gather-runner.js

@@ -10,6 +10,9 @@ const Audit = require('../audits/audit');
 const URL = require('../lib/url-shim');
 const NetworkRecorder = require('../lib/network-recorder.js');

+// Base64 encoded blank page <html></html>
+const BLANK_URL = 'data:text/html;base64,PGh0bWw+PC9odG1sPg==';


we definitely need to look at any other implications of switching to this, but it's encouraging that the smoke tests are ok with it

brendankenny · 2017-08-28T22:55:55Z

lighthouse-cli/test/fixtures/byte-efficiency/script.js

@@ -0,0 +1,64 @@
+/* eslint-disable */


brendankenny · 2017-08-28T22:56:13Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

@@ -0,0 +1,133 @@
+/**
+ * @license


short license?

brendankenny · 2017-08-28T23:13:50Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+
+  /**
+   * @param {!Artifacts} artifacts
+   * @return {{results: !Array<!Object>, tableHeadings: !Object}}


can results be made more specific? It would be nice to have a byte efficiency audit result type around somewhere.

tableHeadings should be headings and is an array of some details type

will do

done

brendankenny · 2017-08-28T23:14:26Z

lighthouse-core/gather/gatherers/js-usage.js

+      .then(_ => options.driver.sendCommand('Profiler.startPreciseCoverage'));
+  }
+
+  afterPass(options) {


return type for artifact would be 👍

brendankenny · 2017-08-28T23:21:03Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+  }
+
+  /**
+   * @param {{functions: !Array<{ranges: {startOffset: number, endOffset: number}}>}} script


also count: number

brendankenny · 2017-08-28T23:22:41Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+  }
+
+  /**
+   * @param {{unusedLength: number, contentLength: number}} wasteData


brendankenny · 2017-08-28T23:50:32Z

lighthouse-core/audits/byte-efficiency/unused-javascript.js

+      for (const range of func.ranges) {
+        if (range.count === 0) {
+          for (let i = range.startOffset; i < range.endOffset; i++) {
+            unusedByIndex[i] = 1;


this seems really inefficient, but it's nice that it's so straightforward :) I guess we're only talking about 10s of MB even when uncompressed, so should be ok-ish until/if we hit some performance issues with it (or we want more out of coverage information)

discussed in person, but it's actually much more efficient since flattening the many duplicated used ranges alone usually takes more time than iterating through the unused ranges (since there's only one range reporting the unused range)

brendankenny · 2017-08-28T23:57:14Z

not seeing any screenshot issues on my machine, either 👍

addyosmani · 2017-08-29T06:52:52Z

Great work. Overall, this looks like it's headed in the right direction. It would be great if we could link to a doc or include a note with the caveat around "potential savings" via coverage (e.g check to see the impact immediate interactions may have on these numbers, take a closer look at the coverage panel for more insights etc).

brendankenny

looking good (while we figure out the about:blank thing)

brendankenny · 2017-08-29T18:14:58Z

lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js

@@ -58,6 +58,9 @@ class UnusedBytes extends Audit {
  }

  /**
+   * Estimates the number of bytes this network record consumed on the network based on the
+   * uncompressed size, uses the actual transfer size from the network record if applicable.


maybe something like Estimates the number of bytes this network record would have consumed on the network based on the uncompressed size totalBytes, ...? What totalBytes is is probably the thing that needs the most explanation

brendankenny · 2017-08-29T18:18:24Z

lighthouse-core/gather/gatherers/js-usage.js

+module.exports = JsUsage;
+
+/** @typedef {{functions: !Array<{ranges: {count: number, startOffset: number, endOffset: number}}>}} */
+JsUsage.JsUsageArtifact; // eslint-disable-line no-unused-expressions


this could also live in the Artifacts.js typedefs, but it's also nice here, local to where it's created, until/if we ever need a lot of references to it (and other artifacts) elsewhere.

patrickhulce · 2017-08-30T00:15:46Z

I reverted the about:blank change for now since the impact is unknown and risky. The original bug that needed the workaround has been fixed in Canary anyway, so we're not any worse off than we were before.

brendankenny

🤖 📜 🔢 ⬇️
LGTM!

* feat: add unused javascript audit * update smokehouse to support stable * feedback * scratch * consistent return typedef * revert about:blank changes * update estimateTransferSize comment * update for stable issue

martinschierle · 2018-10-18T13:00:55Z

I can't seem to get the 'unused-javascript' audit anymore in newer Lighthouse versions - but also can't see any hint that (or why) it was removed - is this still available, and if yes, why not?

patrickhulce · 2018-10-18T14:40:28Z

@martinschierle it's only in the "full config" right now. You can only get it currently by running lighthouse --preset=full <url>

martinschierle · 2018-10-19T08:02:37Z

Thanks for the quick response - great to know, I wasn't even aware that there is a 'full' option, thanks! ;-)

feat: add unused javascript audit

1593b46

ebidel reviewed Aug 23, 2017

View reviewed changes

update smokehouse to support stable

2da4dbe

brendankenny requested changes Aug 28, 2017

View reviewed changes

patrickhulce added 3 commits August 28, 2017 18:08

feedback

b7ad94d

scratch

313880e

consistent return typedef

0f58a89

brendankenny reviewed Aug 29, 2017

View reviewed changes

patrickhulce added 2 commits August 29, 2017 17:12

revert about:blank changes

4f1a6e5

update estimateTransferSize comment

3c8f2d8

brendankenny approved these changes Aug 30, 2017

View reviewed changes

update for stable issue

0d9e2c5

patrickhulce merged commit 5bf4a0f into master Aug 30, 2017

patrickhulce deleted the js_coverage branch August 30, 2017 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add unused javascript audit #3085

feat: add unused javascript audit #3085

patrickhulce commented Aug 23, 2017 •

edited

ebidel Aug 23, 2017

patrickhulce Aug 23, 2017 •

edited

patrickhulce commented Aug 28, 2017

brendankenny Aug 28, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny Aug 28, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny Aug 28, 2017

patrickhulce Aug 29, 2017

brendankenny commented Aug 28, 2017 •

edited

addyosmani commented Aug 29, 2017

brendankenny left a comment

brendankenny Aug 29, 2017

patrickhulce Aug 30, 2017

brendankenny Aug 29, 2017

patrickhulce commented Aug 30, 2017

brendankenny left a comment

martinschierle commented Oct 18, 2018

patrickhulce commented Oct 18, 2018

martinschierle commented Oct 19, 2018


		const ByteEfficiencyAudit = require('./byte-efficiency-audit');

		const IGNORE_THRESHOLD_IN_BYTES = 2048;

feat: add unused javascript audit #3085

feat: add unused javascript audit #3085

Conversation

patrickhulce commented Aug 23, 2017 • edited

Choose a reason for hiding this comment

patrickhulce Aug 23, 2017 • edited

Choose a reason for hiding this comment

patrickhulce commented Aug 28, 2017

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

brendankenny commented Aug 28, 2017 • edited

addyosmani commented Aug 29, 2017

brendankenny 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

patrickhulce commented Aug 30, 2017

brendankenny left a comment

Choose a reason for hiding this comment

martinschierle commented Oct 18, 2018

patrickhulce commented Oct 18, 2018

martinschierle commented Oct 19, 2018

patrickhulce commented Aug 23, 2017 •

edited

patrickhulce Aug 23, 2017 •

edited

brendankenny commented Aug 28, 2017 •

edited