Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Dec 2, 2023

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Nov 29, 2023

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 connorjclark requested a review from a team as a code owner November 29, 2023 02:27
@connorjclark connorjclark requested review from adamraine and removed request for a team November 29, 2023 02:27
@@ -344,6 +345,7 @@ class NetworkRequest {
this.responseHeadersEndTime = timestamp * 1000;

this.transferSize = response.encodedDataLength;
this.responseHeadersTransferSize = response.encodedDataLength;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's descriptive, SGTM

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good moves

// Otherwise we'll just fallback to the average savings in HTTPArchive
return Math.round(totalBytes * 0.5);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could have a resusable function like estimateGzipSizeByType ore something

core/audits/byte-efficiency/legacy-javascript.js Outdated Show resolved Hide resolved
Co-authored-by: Adam Raine <6752989+adamraine@users.noreply.github.com>
@connorjclark
Copy link
Collaborator Author

connorjclark commented Dec 1, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants