Added audit for request compression (gzip & br) #1513

wardpeet · 2017-01-23T22:45:09Z

Still need to add some unit tests but you can have a look if this is something we need.

patrickhulce · 2017-01-23T22:55:25Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+      category: 'Performance',
+      name: 'uses-request-compression',
+      description: 'Site uses GZIP/BROTLI compression',
+      helpText: 'Requests should be optimized to save network bytes.',


maybe add a learn more link? https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/optimize-encoding-and-transfer or something?

patrickhulce · 2017-01-23T23:04:15Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+    // Filter requests that are on the same host as the page and not over h2.
+    const resources = networkRecords.filter(record => {
+      return record._resourceType && record._resourceType._isTextType &&
+        !record._responseHeaders.find(header =>


I believe I was once told we try to have continuations like this at 4 spaces. Is that right @brendankenny ?

patrickhulce · 2017-01-23T23:05:51Z

lighthouse-core/config/test.json

+    "gatherers": []
+  }],
+
+  "audits": [


was this meant to be included?

patrickhulce · 2017-01-23T23:06:56Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+    if (resources.length > 1) {
+      displayValue = `${resources.length} requests were not handled with gzip/brottli compression`;
+    } else if (resources.length === 1) {
+      displayValue = `${resources.length} request was not handled with gzip/brottli compression`;


It'd be nice if we could provide an estimate of potential byte and time savings. Even something like https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js?q=f:AuditRules+gzip&sq=package:chromium&l=97 SGTM as a start

patrickhulce · 2017-01-23T23:07:55Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+  static audit(artifacts) {
+    const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS];
+
+    // Filter requests that are on the same host as the page and not over h2.


oh wait does h2 compress everything by default @brendankenny @ebidel ? this might be a redundant audit then...

I'm not aware of the two being linked. Why not look at http1.1 responses too?

looks like h2 still shows content-encoding: gzip. I should check all http requests, it's a comment that I forgot to change 😛

patrickhulce · 2017-01-23T23:25:53Z

good stuff thanks! yay for more byte efficiency audits :)

patrickhulce · 2017-01-23T23:26:22Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+ * limitations under the License.
+ */
+/*
+ * @fileoverview This audit determines if the images used are sufficiently larger


oh and remove this :)

ebidel · 2017-01-24T00:43:38Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+    return {
+      category: 'Performance',
+      name: 'uses-request-compression',
+      description: 'Site uses GZIP/BROTLI compression',


"Server responses are compressed using GZIP or BROTLI."

ebidel · 2017-01-24T00:44:15Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+  static audit(artifacts) {
+    const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS];
+
+    // Filter requests that are on the same host as the page and not over h2.


I'm not aware of the two being linked. Why not look at http1.1 responses too?

ebidel · 2017-01-24T00:54:15Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+      return record._resourceType && record._resourceType._isTextType &&
+        !record._responseHeaders.find(header =>
+          header.name.toLowerCase() === 'content-encoding' &&
+          (header.value === 'gzip' || header.value === 'br')


Should we check for deflate too. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js?q=f:AuditRules+gzip&sq=package:chromium&l=121 does.

ebidel · 2017-01-24T00:55:37Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+const Audit = require('../audit');
+const Formatter = require('../../formatters/formatter');
+
+class UsesRequestCompression extends Audit {


WDYT about CompressesResponses as a name? Or ReponsesAreCompressed

wardpeet · 2017-01-24T07:11:04Z

Thanks for reviewing!

I think this is my first audit 🎉

ebidel · 2017-01-24T18:38:37Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

-          header.name.toLowerCase() === 'content-encoding' &&
-          (header.value === 'gzip' || header.value === 'br')
-        );
+          record._resourceSize > 1000 &&


should we show violations for all sizes?

I'm interested in nuking this condition and instead filtering out when the savings are under some millisecond threshold.

here's WPT's logic on filtering out small wins: " allow a pass if we don't get 10% savings or less than 1400 bytes"
https://github.com/WPO-Foundation/webpagetest/blob/master/agent/wpthook/optimization_checks.cc#L225

I interpreted this as a safeguard against small files that might increase in file size due to gzipping since we aren't actually gzipping them to see if they'd be smaller, so I'm still in favor of having some sort of threshold on the size.

Ah that would make sense. Let's add a comment if that's the case.

If we go the threshold route, should we standardize on the same threshold as the image opt audit (and future audits). That's also 10%?

fwiw over in https://discuss.httparchive.org/t/file-size-and-compression-savings/145 doug sillars used 500 bytes for the same purpose.

should we standardize on the same threshold as the image opt audit (and future audits). That's also 10%?

well, technically the difference between 49kb and 51kb can be an extra round trip (like 30ms) whereas there shouldn't be any difference between 51kb and 99kb. but that's assuming lots of things. let's start simple, sure. :)

This might be "hard" to measure.. When using h2 the tcp connection is already open which gives us much more bandwidth at a start so the slow start is not applicable, then again we might calculate it based on network packets and time it took.

ebidel · 2017-01-24T18:40:19Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+          record._resourceSize > 1000 &&
+          !record._responseHeaders.find(header =>
+             header.name.toLowerCase() === 'content-encoding' &&
+             compressionTypes.includes(header.value)


node 4 will have problems with .includes. You'll have to use indexOf :(

ebidel · 2017-01-24T18:40:36Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

 */
 'use strict';

 const Audit = require('../audit');
 const Formatter = require('../../formatters/formatter');

-class UsesRequestCompression extends Audit {
+const KB_IN_BYTES = 1024;
+const compressionTypes = ['gzip', 'br', 'deflate']


travis is complaining about a missing ;

ebidel · 2017-01-24T18:41:23Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

    }).map(record => {
+      const originalKb = record._resourceSize / KB_IN_BYTES;
+      const savings = originalKb * 2 / 3;


can you explain this savings calculation?

that was me referencing the devtools gzip audit rule estimation, +1 to adding comment to explain :)

ebidel · 2017-01-24T18:42:43Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

  /**
   * @return {!AuditMeta}
   */
  static get meta() {
    return {
      category: 'Performance',
      name: 'uses-request-compression',
-      description: 'Site uses GZIP/BROTLI compression',
-      helpText: 'Requests should be optimized to save network bytes.',
+      description: 'Server responses are compressed using GZIP or BROTLI.',


should we include DEFLATE in all the text strings?

patrickhulce · 2017-01-24T17:17:31Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+          );
+    }).map(record => {
+      const originalKb = record._resourceSize / KB_IN_BYTES;
+      const savings = originalKb * 2 / 3;


Maybe add a quick comment that GZIP savings is relatively predictable so we aren't confused later :)

patrickhulce · 2017-01-24T17:18:11Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+
+    let displayValue = '';
+    if (resources.length > 1) {
+      displayValue = `${resources.length} requests were not handled with GZIP/BROTTLI compression`;


we're moving in all of the byte efficiency audits to report something like XXKB (~XXms) potential savings, I'm working on generic time estimate but for now just sum up the the byte savings of each record and report that?

sure thing!

paulirish · 2017-01-24T18:53:00Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+      const savings = originalKb * 2 / 3;
+      const percent = Math.round(savings / originalKb * 100);
+
+      const label = `${Math.round(originalKb)} KB total, GZIP savings: ${percent}%`;


I'd like to compute the time waste measured by these requests. Take a look at computeWaste within patrick's #1497 for an idea of bytes -> milliseconds in download time.

I think your byte savings calculation is the right kind of start, but we can bring it all the way to "total download time wasted."

paulirish · 2017-01-24T18:54:10Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+          );
+    }).map(record => {
+      const originalKb = record._resourceSize / KB_IN_BYTES;
+      const savings = originalKb * 2 / 3;


agree with eric, citation needed here. :)
can you find any bulk data about this.. maybe https://discuss.httparchive.org/ has something?

found this https://discuss.httparchive.org/t/file-size-and-compression-savings/145

check out the potential savings column. It might be interesting to look into how it was calculated, but at first glance it indicates we may want to use these numbers based on resource type rather than a broad generalization.

I do like this approach. We could keep some kind of mapping table with these values, we could also checkout brottli and show both of them like we do with images.

paulirish · 2017-01-24T18:55:43Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+    // Filter requests that are text based and have gzip/br encoding.
+    const resources = networkRecords.filter(record => {
+      return record._resourceType && record._resourceType._isTextType &&
+          record._resourceSize > 1000 &&


if you want to only consider these, let's add a comment why. (also, good to note this is an uncompressed size)

paulirish · 2017-01-24T18:59:15Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+    const networkRecords = artifacts.networkRecords[Audit.DEFAULT_PASS];
+
+    // Filter requests that are text based and have gzip/br encoding.
+    const resources = networkRecords.filter(record => {


first, technically there can be multiple requests for a single resource, so i'd like to use resource and request and response names correctly.

but let's split the filter and the map.

const uncompressedResponses = networkRecords.filter(fn); const recordsWithWastage = uncompressedResponses.map(this.computeWaste);

wardpeet · 2017-01-29T19:19:15Z

Updated the audit :)

patrickhulce · 2017-01-29T20:15:38Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+          tableHeadings: {
+            url: 'URL',
+            total: 'Original (KB)',
+            gzipSavings: 'GZIP Savings (%)',


since we're showing a static 66.6% it might make more sense to show in KB :)

👍 .
also, Is it straightforward to use num.toLocaleString() ? would be nice to get thousands delimiters on these numbers.

also,, that is a BIG page you're looking at. 😀

calculations where wrong forgot to divide 1024 for bytes.

Yes will update with some string numbers

wardpeet · 2017-01-30T06:00:50Z

Update

paulirish · 2017-01-29T21:32:39Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+const KB_IN_BYTES = 1024;
+const TOTAL_WASTED_BYTES_THRESHOLD = 100 * KB_IN_BYTES;
+
+class CompressesResponses extends Audit {


name is a little awkward. how about "ResponseCompression"

"ResponsesAreCompressed"?

paulirish · 2017-01-29T21:37:08Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+  static filterUnoptimizedResponses(networkRecords) {
+    return networkRecords.reduce((prev, record) => {
+      const isTextBasedResource = record._resourceType && record._resourceType._isTextType;
+      const isContentEncoded = isTextBasedResource &&


i'd be in favor of just checking the response headers for this boolean. leave isTextBasedResource and recordSize out of it.

return early if its not text based or it is but no recordsize.

paulirish

coming along nicely. :)

paulirish · 2017-01-30T18:45:50Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+   * @return {!Array<{url: string, isBase64DataUri: boolean, mimeType: string, resourceSize: number}>}
+   */
+  static filterUnoptimizedResponses(networkRecords) {
+    return networkRecords.reduce((prev, record) => {


is it important to do this inside of a reduce?

that was probably copied from my optimized-images gatherer where it's also become unnecessary since going map -> array :)

yeah indeed, wanted to keep it the same

It looks like this reduce might be a problem.. perhaps the return prev below?

lighthouse --perf --view http://www.al.com/news/huntsville/index.ssf/2017/03/nasa_gives_mars_orbiter_a_boos.html

see the duplicate entries? AFAICT these requests do not repeat, so we're incorrect.

When you figure out what's happening here, can you add a test to cover it?

edit: i ran again on this URL and got some repeats, but it wasn't nearly as bad..

How did you find this site this is crazy!

My findings: the reduce is not the problem assuming DevTools is not broken, every request has a unique request ID and all requests for the duplicate urls have different start or end times.

Furthermore, loading the page in a clean profile with the same emulation settings yields entirely different network records for me! The triplicate URLs aren't even loaded once. Lighthouse shows ~600 network records while DevTools I can only get max ~370.

My money is on the site actually requesting the same resource more than once, this does not have preserveQuery enabled which masks more errors, and somehow Chrome is making distinct requests (or DevTools is reporting erroneous information) that look in every other respect to be identical, but I have no idea why there's such a stark difference in LH loading the page and vanilla Chrome.

Now i remove all duplicates that have the same display name and kb.

k thx for looking into this. :)

ditto on the reduce, again.

paulirish · 2017-01-30T19:10:48Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+    const textRecords = ResponseCompression.filterUnoptimizedResponses(networkRecords);
+
+    return textRecords.map(record => {
+      // GZIP saving is predictable (@see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js?q=f:AuditRules+gzip&sq=package:chromium&l=97)


I'd change "gzip saving is predictable" to "Compression savings can be roughly estimated"

And lets link to the discuss.httparchive thread instead, as that clearly had more research involved. :)

paulirish · 2017-01-30T19:14:12Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+      // GZIP saving is predictable (@see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js?q=f:AuditRules+gzip&sq=package:chromium&l=97)
+      const gzipSize = record.resourceSize * 2 / 3;
+
+      return Object.assign({}, {


arr.map + object.assign for a single property seems like some overkill. let's make this a tad more simple/

paulirish · 2017-01-30T19:21:25Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+
+    return textRecords.map(record => {
+      // GZIP saving is predictable (@see https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js?q=f:AuditRules+gzip&sq=package:chromium&l=97)
+      const gzipSize = record.resourceSize * 2 / 3;


The data from https://discuss.httparchive.org/t/file-size-and-compression-savings/145 shows this

Type Potential Savings

css 47%

html 32%

json 50%

plain -2%

script 27%

xml 35%

By comparison we're saying everything should expect 33%.

I think we should use these numbers for our estimation. (Select an expected savings based on request.resourceType). We're still hand-wavy but it's nice to be on the right side of data.

This is awesome! I'd love to be able to report more accurate estimates based on the makeup of the file types although I'm not sure I trust the methodology behind these numbers and think we'll end up underreporting savings (aside: the current AuditRules.js also says 66% savings not 33%).

27% for script sounds really low especially when you look around at the compression ratios for various libraries and Ilya's table in the read more link.

Specifically, I distrust this assumption.

If we assume that the files >500 bytes are evenly distributed between compressed/uncompressed

I would hope, though this may very naive, that larger, more sophisticated applications with lots of script use GZIP at a higher rate than that of smaller applications with less script. (example: new developer copy/pastes the CDN tag for jQuery/Angular/etc which will be gzipped and then writes a couple of his own files not gzipped at 1/10th the size of the library, which leads to the incorrect conclusion that gzip increases file size). Imprecise methodology still is better than no methodology at all though, just wanted to flag it as needing further investigation.

I can use zlib which is included in node.
so I can do gzip compression in node

paulirish · 2017-01-30T19:22:45Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+   */
+  static filterUnoptimizedResponses(networkRecords) {
+    return networkRecords.reduce((prev, record) => {
+      const isTextBasedResource = record._resourceType && record._resourceType._isTextType;


record.resourceType().isTextType() should be all you need here. ;)

paulirish · 2017-01-30T19:24:20Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+      const isTextBasedResource = record._resourceType && record._resourceType._isTextType;
+      const isContentEncoded = isTextBasedResource &&
+        record._resourceSize &&
+        !record._responseHeaders.find(header =>


record.responseHeaders.find(

paulirish · 2017-01-30T19:24:34Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+    return networkRecords.reduce((prev, record) => {
+      const isTextBasedResource = record._resourceType && record._resourceType._isTextType;
+      const isContentEncoded = isTextBasedResource &&
+        record._resourceSize &&


record.resourceSize

paulirish · 2017-01-30T19:29:30Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+
+      if (isTextBasedResource && isContentEncoded) {
+        prev.push({
+          url: record._url,


record.url(), record.mimeType(), record.resourceSize (note firs two are methods, last is a getter ;)

paulirish · 2017-01-30T19:38:27Z

lighthouse-core/test/gather/gatherers/dobetterweb/response-compression-test.js

+const traceData = {
+  networkRecords: [
+    {
+      _url: 'http://google.com/index.js',


I see now that the public accessors for the properties I recommended above would make it more difficult for your tests. Hmmm.

TBH it would be best if you construct these via new SDK.NetworkRequest(). There are some examples of this in the devtools own tests: https://cs.chromium.org/search/?q=%22new+SDK.NetworkRequest%22+f:layouttests&sq=package:chromium&type=cs

This will be a little overhead for you, but good in the long run in case devtools internals change and our tests assume old APIs. (hard to detect that. :)

All I want is to learn new stuff so 👍

@paulirish yeah I've run into this a number of times now too, at first I was following the existing pattern of the private variables, but have mixed in the public accessors in later audits. Are we trying to standardize on the public ones now?

it's a little unfair to make you do this @wardpeet since every other change using network records mixes these regularly, including one landed yesterday :P But if you're willing, that would be great to have this solved.

If it gets too involved feel free to push off to a followup PR so you can get this one in.

Also, a little unfair that devtools is inconsistent with what is a getter property and what is a getter method, leading to confusing mismatches. example: it's record.url and record.mimeType but record.resourceType()

It's a bit confusing but I don't mind. After I get it right I can convert the other audits ^^

i think we can continue to use the private _ props. :)

so should I move my code around to use privates again? :)

paulirish · 2017-02-08T00:17:37Z

@wardpeet heyoooo how we doing here? looks like recent commit uses zlib, although its not yet in package.json. are you ready for review?

wardpeet · 2017-02-10T06:36:29Z

@paulirish zlib is inside nodejs :) so no package.json necessary.

Yeah ready to review

wardpeet · 2017-02-26T18:44:38Z

pinging @paulirish @patrickhulce @ebidel @brendankenny for a review

patrickhulce

Looks good overall just mostly rebase needed for a few things

patrickhulce · 2017-02-27T17:10:06Z

lighthouse-core/audits/dobetterweb/uses-request-compression.js

+const KB_IN_BYTES = 1024;
+const TOTAL_WASTED_BYTES_THRESHOLD = 100 * KB_IN_BYTES;
+
+class ResponsesAreCompressed extends Audit {


It's been a while and this might be a pain (sorry!), but there's a new byte-efficiency folder and base audit you can extend that should eliminate a bit of duplicated code

no problem 👍

patrickhulce · 2017-02-27T17:10:45Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+'use strict';
+
+const Gatherer = require('../gatherer');
+const zlib = require('zlib');


How does the built-in browserify? Do we need to do anything special for extension/DevTools?

i'll have a look and report back

Looks all right
Without zlib:

With zlib:

I was more speaking to the check that there's no random native dependency that won't be bundle-able but good to be aware of ~100k increase too :)

Looks like its 171kb for pako (which browserify swaps in for zlib).

so should I look into other waters to see how we can switch pako? Or is 171kb something we wont care about?

we can manage with 171k IMO.

@wardpeet bonus points if you can pass pako through a very basic minification before it gets bundled up. (assuming there's savings there)

patrickhulce · 2017-02-27T17:14:34Z

lighthouse-core/config/default.json

@@ -297,7 +299,8 @@
      "audits": {
        "unused-css-rules": {},
        "uses-optimized-images": {},
-        "uses-responsive-images": {}
+        "uses-responsive-images": {},
+        "uses-request-compression": {}


This needs a rebase since the objects here are required now for the auto-expanding of categories to work (this also got moved to perf recently I believe)

wardpeet · 2017-03-01T22:10:53Z

@patrickhulce done with rebase :)

Now the threshold is 100kb but maybe it's too little? It's around 250ms of waste

patrickhulce · 2017-03-02T00:45:42Z

Now the threshold is 100kb but maybe it's too little? It's around 250ms of waste

I'm in favor of making the threshold for this one lower than the others since it's basically a no-brainer compared to the pain of webp specific loading, finding unused rules, and generating many different size of images, etc.

patrickhulce · 2017-03-02T00:48:07Z

lighthouse-core/audits/byte-efficiency/uses-request-compression.js

+      const gzipSavings = originalSize - gzipSize;
+
+      // allow a pass if we don't get 10% savings or less than 1400 bytes
+      if (gzipSize / originalSize > 0.9 || gzipSavings < IGNORE_THRESHOLD_IN_BYTES) {


nit: want to throw the ratio into a const too? IGNORE_THRESHOLD_IN_PERCENT or something?

patrickhulce · 2017-03-02T00:49:44Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+        }
+
+        return new Promise((resolve) => {
+          return zlib.gzip(content, (err, res) => {


reject on error?

patrickhulce · 2017-03-02T00:52:12Z

lighthouse-core/lib/web-inspector.js

    // Mocked-up WebInspector Target for NetworkManager
    const fakeNetworkAgent = {
-      enable() {}
+      enable() {},
+      getResponseBody(requestId, onComplete) {


kinda wonky that we have to do this opaquely through the resourceContent() instead of explicitly in the gatherer that we need it :/ @paulirish the decision was based on wanting to use NetworkRequest from devtools more aggressively right?

yah. i think we'd end up with a helper method in driver otherwise.. and it seems slightly more convenient to call a method on the record itself rather than driver.requestContent(networkRecord).then(..

so i'm supportive of mocking the real API here, yeah. :)

googlebot · 2017-03-03T01:36:10Z

deleted stuff about double commits

wardpeet · 2017-03-24T21:11:26Z

@patrickhulce @paulirish any luck you guys could give this a spin again?

patrickhulce

sorry I've been so lagging! everything a-ok on my end, but I'll defer to @paulirish for final 👍

patrickhulce · 2017-04-01T00:08:13Z

lighthouse-core/audits/byte-efficiency/uses-request-compression.js

+
+const IGNORE_THRESHOLD_IN_BYTES = 1400;
+const IGNORE_THRESHOLD_IN_PERCENT = 0.9;
+const TOTAL_WASTED_BYTES_THRESHOLD = 10 * 1024; // 5KB


nit: 10KB? :)

patrickhulce · 2017-04-01T00:12:44Z

lighthouse-core/test/audits/byte-efficiency/uses-response-compression-test.js

+/* eslint-env mocha */
+
+describe('Page uses optimized responses', () => {
+  it('fails when reponses are collectively unoptimized', () => {


nit: s/reponses/responses/, same below :)

patrickhulce · 2017-04-01T00:14:13Z

lighthouse-core/test/gather/gatherers/dobetterweb/response-compression-test.js

+    return optimizedResponses.afterPass(options, createNetworkRequests(traceData))
+      .then(artifact => {
+        assert.equal(artifact.length, 2);
+        checkSizes(artifact[0], 6, 26);


nit: since this is only used twice and not in a bunch of functions i'd have a preference to switch to straight up asserts so it's a little clearer what's happening. two fewer lines too 🎉

patrickhulce · 2017-04-01T00:15:16Z

lighthouse-core/test/audits/byte-efficiency/uses-response-compression-test.js

+      ],
+    });
+
+    assert.equal(auditResult.passes, false);


nit: assert a length on the extended info to make sure the ones you want filtered are filtered?

paulirish

pretty sure we're good to go after this round.

paulirish · 2017-03-25T01:35:17Z

lighthouse-core/audits/byte-efficiency/uses-request-compression.js

+
+      // allow a pass if we don't get 10% savings or less than 1400 bytes
+      if (
+          gzipSize / originalSize > IGNORE_THRESHOLD_IN_PERCENT ||


little odd to flip the > on these two lines. can we keep it consistent? (probably should change the 0.9 above)

Is this ok?
IGNORE_THRESHOLD_IN_PERCENT = 0.1
1 - gzipSize / originalSize < IGNORE_THRESHOLD_IN_PERCENT

paulirish · 2017-03-25T01:35:47Z

lighthouse-core/audits/byte-efficiency/uses-request-compression.js

+    const uncompressedResponses = artifacts.ResponseCompression;
+
+    let totalWastedBytes = 0;
+    const results = uncompressedResponses.reduce((results, record) => {


i know i mentioned it before but this reduce() really kills me. let's forEach.
all patrick's fault. :)

paulirish · 2017-03-25T01:37:56Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+'use strict';
+
+const Gatherer = require('../gatherer');
+const zlib = require('zlib');


we can manage with 171k IMO.

@wardpeet bonus points if you can pass pako through a very basic minification before it gets bundled up. (assuming there's savings there)

paulirish · 2017-03-25T01:39:19Z

lighthouse-core/gather/gatherers/dobetterweb/response-compression.js

+   * @return {!Array<{url: string, isBase64DataUri: boolean, mimeType: string, resourceSize: number}>}
+   */
+  static filterUnoptimizedResponses(networkRecords) {
+    return networkRecords.reduce((prev, record) => {


k thx for looking into this. :)

ditto on the reduce, again.

paulirish · 2017-04-01T01:33:53Z

lighthouse-core/audits/byte-efficiency/uses-request-compression.js

+      const gzipSize = record.gzipSize;
+      const gzipSavings = originalSize - gzipSize;
+
+      // allow a pass if we don't get 10% savings or less than 1400 bytes


lets be more clear here

we require at least 10% savings off the original size AND at least 1400 bytes
if the savings is smaller than either, we don't care

paulirish · 2017-04-05T20:01:41Z

lighthouse-extension/gulpfile.js

@@ -118,7 +118,8 @@ gulp.task('browserify-lighthouse', () => {
      .ignore('source-map')
      .ignore('whatwg-url')
      .ignore('url')
-      .ignore('debug/node');
+      .ignore('debug/node')
+      .ignore('pako/lib/zlib/inflate.js');


can you also ignore pako/lib/inflate.js ?

I might be looking wrong but I don't see it being injected.

fair. I was just looking at the source tree of pako. I guess the require that get done in browserify is selective already.

:)

he said to.

- Moved to wastedBytes approach and wastedTime

googlebot · 2017-04-14T19:18:09Z

CLAs look good, thanks!

patrickhulce reviewed Jan 23, 2017

View reviewed changes

ebidel added the new_audit label Jan 24, 2017

ebidel suggested changes Jan 24, 2017

View reviewed changes

wardpeet closed this Jan 24, 2017

wardpeet reopened this Jan 24, 2017

ebidel previously requested changes Jan 24, 2017

View reviewed changes

patrickhulce reviewed Jan 24, 2017

View reviewed changes

paulirish requested changes Jan 24, 2017

View reviewed changes

wardpeet force-pushed the gzip-br-check branch from a31bd90 to 091ed8e Compare January 29, 2017 19:18

patrickhulce reviewed Jan 29, 2017

View reviewed changes

paulirish reviewed Jan 30, 2017

View reviewed changes

paulirish requested changes Jan 30, 2017

View reviewed changes

patrickhulce mentioned this pull request Jan 31, 2017

refactor: use public accessors for network records #1588

Closed

ebidel mentioned this pull request Feb 3, 2017

Audits Meta: Feature parity with PSI, MFT, DevTools #909

Closed

16 tasks

wardpeet force-pushed the gzip-br-check branch from 209d8cd to 5dfe5ce Compare February 6, 2017 20:10

patrickhulce suggested changes Feb 27, 2017

View reviewed changes

wardpeet force-pushed the gzip-br-check branch 2 times, most recently from 250839b to 848e173 Compare March 1, 2017 21:54

patrickhulce reviewed Mar 2, 2017

View reviewed changes

paulirish mentioned this pull request Mar 30, 2017

[meta] I/O Burndown #1937

Closed

52 tasks

patrickhulce reviewed Apr 1, 2017

View reviewed changes

paulirish requested changes Apr 1, 2017

View reviewed changes

paulirish reviewed Apr 1, 2017

View reviewed changes

wardpeet force-pushed the gzip-br-check branch from 866b635 to e6aaf49 Compare April 5, 2017 19:27

paulirish reviewed Apr 5, 2017

View reviewed changes

paulirish approved these changes Apr 14, 2017

View reviewed changes

wardpeet added 14 commits April 14, 2017 20:57

Added audit for request compression (gzip & br)

4a098a3

Review fixes ^^

094f0ea

Review updates

0640468

- Moved to wastedBytes approach and wastedTime

Added unit tests

a587c33

Added toLocalString for better number formatting

6f6d410

Add zlib to do gzip compression

5ea1b75

Rebased and use byte-efficiency audit

87584ea

Review changes

b916692

Remove duplicates

2485b92

Change waste threshold to 10kb

3f79981

Review changes

1e1eb2a

Ignore pako inflate from browserify

7c10492

Put debug flag back on browserify

1df3627

Update nit on web-inspector

b3bfe77

patrickhulce approved these changes Apr 14, 2017

View reviewed changes

Rebase stuff

c31228c

wardpeet force-pushed the gzip-br-check branch from 765b4e6 to c31228c Compare April 14, 2017 19:18

wardpeet merged commit ba01e2a into master Apr 14, 2017

wardpeet deleted the gzip-br-check branch May 11, 2017 18:39

connorjclark mentioned this pull request Aug 23, 2024

core(uses-text-compression): ignore percent threshold for large savings #16165

Merged

Added audit for request compression (gzip & br) #1513

Added audit for request compression (gzip & br) #1513

Conversation

wardpeet commented Jan 23, 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

wardpeet Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

patrickhulce commented Jan 23, 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

wardpeet commented Jan 24, 2017 • 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

paulirish Jan 24, 2017 • 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

patrickhulce Jan 24, 2017 • 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

wardpeet commented Jan 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

patrickhulce Mar 3, 2017 • 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

Choose a reason for hiding this comment

patrickhulce Jan 31, 2017 • 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

paulirish commented Feb 8, 2017

wardpeet commented Feb 10, 2017

wardpeet Jan 24, 2017 •

edited

Loading

wardpeet commented Jan 24, 2017 •

edited

Loading

paulirish Jan 24, 2017 •

edited

Loading

patrickhulce Jan 24, 2017 •

edited

Loading

patrickhulce Mar 3, 2017 •

edited

Loading

patrickhulce Jan 31, 2017 •

edited

Loading