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.

Sign up for GitHub

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

Jump to bottom

core(duplicated-javascript): exclude header size for estimating wasted bytes #15667

Merged
merged 3 commits into from
Dec 7, 2023
Merged

core(duplicated-javascript): exclude header size for estimating wasted bytes #15667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a95ab7
core(duplicated-javascript): exclude header size for estimating waste…
connorjclark Dec 6, 2023
05afd21
del old var
connorjclark Dec 6, 2023
df14b98
rm dead code
connorjclark Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 9 additions & 37 deletions core/audits/byte-efficiency/duplicated-javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import {ByteEfficiencyAudit} from './byte-efficiency-audit.js';
import {ModuleDuplication} from '../../computed/module-duplication.js';
import * as i18n from '../../lib/i18n/i18n.js';
import {estimateTransferSize, getRequestForScript} from '../../lib/script-helpers.js';
import {estimateCompressionRatioForContent} from '../../lib/script-helpers.js';

const UIStrings = {
/** Imperative title of a Lighthouse audit that tells the user to remove duplicate JavaScript from their code. This is displayed in a list of audit titles that Lighthouse generates. */
Expand Down Expand Up @@ -103,16 +103,6 @@
return groupedDuplication;
}

/**
*
* @param {LH.Artifacts.NetworkRequest|undefined} networkRecord
* @param {number} contentLength
*/
static _estimateTransferRatio(networkRecord, contentLength) {
const transferSize = estimateTransferSize(networkRecord, contentLength, 'Script');
return transferSize / contentLength;
}

/**
* This audit highlights JavaScript modules that appear to be duplicated across all resources,
* either within the same bundle or between different bundles. Each details item returned is
Expand All @@ -131,7 +121,7 @@
await DuplicatedJavascript._getDuplicationGroupedByNodeModules(artifacts, context);

/** @type {Map<string, number>} */
const transferRatioByUrl = new Map();
const compressionRatioByUrl = new Map();

/** @type {Item[]} */
const items = [];
Expand All @@ -144,10 +134,10 @@
for (const [source, sourceDatas] of duplication.entries()) {
// One copy of this module is treated as the canonical version - the rest will have
// non-zero `wastedBytes`. In the case of all copies being the same version, all sizes are
// equal and the selection doesn't matter. When the copies are different versions, it does
// matter. Ideally the newest version would be the canonical copy, but version information
// is not present. Instead, size is used as a heuristic for latest version. This makes the
// audit conserative in its estimation.
// equal and the selection doesn't matter (ignoring compression ratios). When the copies are
// different versions, it does matter. Ideally the newest version would be the canonical
// copy, but version information is not present. Instead, size is used as a heuristic for
// latest version. This makes the audit conserative in its estimation.

Check warning on line 140 in core/audits/byte-efficiency/duplicated-javascript.js

View check run for this annotation

Codecov / codecov/patch

core/audits/byte-efficiency/duplicated-javascript.js#L137-L140 

Added lines #L137 - L140 were not covered by tests

/** @type {SubItem[]} */
const subItems = [];
Expand All @@ -158,27 +148,9 @@
const scriptId = sourceData.scriptId;
const script = artifacts.Scripts.find(script => script.scriptId === scriptId);
const url = script?.url || '';

/** @type {number|undefined} */
let transferRatio = transferRatioByUrl.get(url);
if (transferRatio === undefined) {
if (!script || script.length === undefined) {
// This should never happen because we found the wasted bytes from bundles, which required contents in a Script.
continue;
}

const contentLength = script.length;
const networkRecord = getRequestForScript(networkRecords, script);
transferRatio = DuplicatedJavascript._estimateTransferRatio(networkRecord, contentLength);
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
transferRatioByUrl.set(url, transferRatio);
}

if (transferRatio === undefined) {
// Shouldn't happen for above reasons.
continue;
}

const transferSize = Math.round(sourceData.resourceSize * transferRatio);
const compressionRatio = await estimateCompressionRatioForContent(
compressionRatioByUrl, url, artifacts, networkRecords);
const transferSize = Math.round(sourceData.resourceSize * compressionRatio);

Check warning on line 153 in core/audits/byte-efficiency/duplicated-javascript.js

View check run for this annotation

Codecov / codecov/patch

core/audits/byte-efficiency/duplicated-javascript.js#L151-L153 

Added lines #L151 - L153 were not covered by tests

subItems.push({
url,
Expand Down
5 changes: 4 additions & 1 deletion core/lib/network-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,7 +614,10 @@ class NetworkRequest {
* @return {boolean}
*/
static isContentEncoded(record) {
return record.responseHeaders.some(item => item.name === 'Content-Encoding');
// FYI: older devtools logs (like our test fixtures) seems to be lower case, while modern logs
// are Cased-Like-This.
const pattern = /^content-encoding$/i;
return record.responseHeaders.some(item => item.name.match(pattern));
Copy link
Member

Choose a reason for hiding this comment

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

nit: item.name.toLowerCase() has worked in the past. Just easier to read than a regex IMO but up to you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lots of headers / churn with temporary strings, so I wanted to avoid

}

/**
Expand Down
5 changes: 4 additions & 1 deletion core/test/audits/byte-efficiency/duplicated-javascript-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,9 @@ describe('DuplicatedJavascript computed artifact', () => {

it('.audit', async () => {
// Use a real trace fixture, but the bundle stuff.
// Note: this mixing of data from different sources makes the exact results
// of this audit pretty meaningless. The important part of this test is that
// `wastedBytesByUrl` is functioning.
const bundleData1 = loadSourceMapFixture('coursehero-bundle-1');
const bundleData2 = loadSourceMapFixture('coursehero-bundle-2');
const artifacts = {
Expand Down Expand Up @@ -363,7 +366,7 @@ describe('DuplicatedJavascript computed artifact', () => {
const results = await DuplicatedJavascript.audit(artifacts, context);

// Without the `wastedBytesByUrl` this would be zero because the items don't define a url.
expect(results.details.overallSavingsMs).toBe(300);
expect(results.details.overallSavingsMs).toBe(1830);
});

it('_getNodeModuleName', () => {
Expand Down
26 changes: 6 additions & 20 deletions core/test/fixtures/user-flows/reports/sample-flow-result.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3252,7 +3252,7 @@
"scoreDisplayMode": "metricSavings",
"numericValue": 0,
"numericUnit": "millisecond",
"displayValue": "Potential savings of 0 KiB",
"displayValue": "",
"metricSavings": {
"FCP": 0,
"LCP": 0
Expand Down Expand Up @@ -3286,7 +3286,7 @@
"items": [
{
"url": "https://www.mikescerealshack.co/_next/static/chunks/commons.49455e4fa8cc3f51203f.js",
"wastedBytes": 167,
"wastedBytes": 0,
"subItems": {
"type": "subitems",
"items": [
Expand All @@ -3306,7 +3306,7 @@
}
],
"overallSavingsMs": 0,
"overallSavingsBytes": 167,
"overallSavingsBytes": 0,
"sortedBy": [
"wastedBytes"
],
Expand Down Expand Up @@ -8031,14 +8031,6 @@
"core/audits/byte-efficiency/legacy-javascript.js | description": [
"audits[legacy-javascript].description"
],
"core/lib/i18n/i18n.js | displayValueByteSavings": [
{
"values": {
"wastedBytes": 167
},
"path": "audits[legacy-javascript].displayValue"
}
],
"core/lib/i18n/i18n.js | columnWastedBytes": [
"audits[legacy-javascript].details.headings[2].label"
],
Expand Down Expand Up @@ -20850,7 +20842,7 @@
"scoreDisplayMode": "metricSavings",
"numericValue": 0,
"numericUnit": "millisecond",
"displayValue": "Potential savings of 0 KiB",
"displayValue": "",
"metricSavings": {
"FCP": 0,
"LCP": 0
Expand Down Expand Up @@ -20884,7 +20876,7 @@
"items": [
{
"url": "https://www.mikescerealshack.co/_next/static/chunks/commons.49455e4fa8cc3f51203f.js",
"wastedBytes": 167,
"wastedBytes": 0,
"subItems": {
"type": "subitems",
"items": [
Expand All @@ -20904,7 +20896,7 @@
}
],
"overallSavingsMs": 0,
"overallSavingsBytes": 167,
"overallSavingsBytes": 0,
"sortedBy": [
"wastedBytes"
],
Expand Down Expand Up @@ -25629,12 +25621,6 @@
"wastedBytes": 52102
},
"path": "audits[uses-responsive-images].displayValue"
},
{
"values": {
"wastedBytes": 167
},
"path": "audits[legacy-javascript].displayValue"
}
],
"core/lib/i18n/i18n.js | columnResourceSize": [
Expand Down
Loading