core(legacy-javascript): exclude header size for estimating wasted bytes

[connorjclark]

legacy-javascript uses ByteEfficiencyAudit.estimateTransferSize to get a transfer ratio for a given script resource, in order to convert the estimated byte savings from being in terms of raw resource size, into network transfer size (taking into account content encoding so as not to overstate the savings).

There is no direct information from a network request to get compression ratio of the request body (we have neither the ratio directly, nor the size of the compressed body). All we have is resource size (uncompressed bytes of the body) and transfer size (potentially compressed bytes of the entire response, which includes headers, the body, and potentially other stuff like SSL I think). Despite that, we still use transfer size to get the compression ratio (transferSize / resourceSize).

I noticed that for small requests (thanks Smokerider!), where the headers are a significant portion of the total bytes for a response, this results in a compression ratio that is significantly off. To reduce this, we can get a bit closer to the truth by marking the transfer size at the time of Network.responseReceived (which is right after all headers, but before the response body), and subtracting that from the total transfer size to get just the content portion (this is either the network size of the headers, or possibly headers+TLS+SSL...). Note that headers in h2+ may be potentially compressed, and that this property covers that too. You can confirm the correctness of this with the h2load tool (brew install nghttp2, h2load https://blog.cloudflare.com | tail -6 |head -1), though I've found it to be off by ~bytes compared to Chrome (likely just b/c SSL/TSL, or a different compression table being using for HPACK).

Further, I looked at transfer size up to Network.responseReceived for google.com (h3) and saw it super low at ~32 bytes.... I can only assume that h3 has an even better compression method for headers, or Chrome has finetuned the headers compression table it uses for google.com

Lastly, if the response is not encoded this estimation function now uses the resource size directly, since that is something that explicitly excludes all the junk we are trying to remove when encoded.

Good entry point to look at Chromium encodedDataLength

---

Following up on this PR: Do same for duplicated-javascript, dedupe the code, and check if other usages of ByteEfficiencyAudit.estimateTransferSize should be moved to new ByteEfficiencyAudit.estimateCompressedContentSize

[connorjclark]

name is maybe not best, as this (I'm pretty sure) includes SSL/TSL transferred bytes...

[benschwarz]

It's descriptive, SGTM

[adamraine]

LGTM, good moves

[adamraine]

nit: could have a resusable function like estimateGzipSizeByType ore something

[connorjclark]

very strange bug...

diff --git a/core/test/network-records-to-devtools-log.js b/core/test/network-records-to-devtools-log.js
index f3d15e0c0..aee9e4158 100644
--- a/core/test/network-records-to-devtools-log.js
+++ b/core/test/network-records-to-devtools-log.js
@@ -288,7 +288,7 @@ function getResponseReceivedEvent(networkRecord, index, normalizedTiming) {
         connectionId: networkRecord.connectionId || 140,
         fromDiskCache: networkRecord.fromDiskCache || false,
         fromServiceWorker: networkRecord.fetchedViaServiceWorker || false,
-        encodedDataLength: networkRecord.responseHeadersTransferSize || networkRecord.transferSize,
+        encodedDataLength: networkRecord.responseHeadersTransferSize || networkRecord.transferSize || 0,
         timing: {...normalizedTiming.timing},
         protocol: networkRecord.protocol || 'http/1.1',
       },

without this change, the matching part of expect(roundTrippedNetworkRecords).toMatchObject(networkRecords); results in a stack overflow! 😕 Cause is some bad handling of recursive objects in toMatchObject, but for some reason when the objects are equal (the above patch was a real oversight on my part), it doesn't overflow.

jestjs/jest#14734