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: round metric savings to remove false precision #15721

Merged
merged 8 commits into from
Jan 4, 2024
Merged

core: round metric savings to remove false precision #15721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1dca7bf
core: normalize metric savings
adamraine Jan 2, 2024
d6cea5d
test
adamraine Jan 2, 2024
f632bb0
sample
adamraine Jan 2, 2024
68eb7ce
pr
adamraine Jan 2, 2024
c7c49aa
Merge branch 'main' into normalize-metric-savings
adamraine Jan 4, 2024
9757c3a
inp
adamraine Jan 4, 2024
34b4779
nits
adamraine Jan 4, 2024
b83a126
nit logic
adamraine Jan 4, 2024
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
43 changes: 41 additions & 2 deletions core/audits/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ import {Util} from '../../shared/util.js';

const DEFAULT_PASS = 'defaultPass';

/** @type {LH.Audit.MetricSavings} */
const METRIC_SAVINGS_PRECISION = {
FCP: 50,
LCP: 50,
INP: 50,
TBT: 50,
CLS: 0.001,
};
adamraine marked this conversation as resolved.
Show resolved Hide resolved

/**
* @typedef TableOptions
* @property {number=} wastedMs
Expand Down Expand Up @@ -339,6 +348,33 @@ class Audit {
return score;
}

/**
* @param {LH.Audit.MetricSavings|undefined} metricSavings
* @return {LH.Audit.MetricSavings|undefined}
*/
static _quantizeMetricSavings(metricSavings) {
if (!metricSavings) return;

/** @type {LH.Audit.MetricSavings} */
const normalizedMetricSavings = {...metricSavings};

for (const key of /** @type {Array<LH.Result.MetricAcronym>} */ (Object.keys(metricSavings))) {
let value = metricSavings[key];
if (value === undefined) continue;

value = Math.max(value, 0);

const precision = METRIC_SAVINGS_PRECISION[key];
if (precision !== undefined) {
value = Math.round(value / precision) * precision;
}

normalizedMetricSavings[key] = value;
}

return normalizedMetricSavings;
}

/**
* @param {typeof Audit} audit
* @param {string | LH.IcuMessage} errorMessage
Expand Down Expand Up @@ -378,10 +414,13 @@ class Audit {
scoreDisplayMode = product.scoreDisplayMode;
}

const metricSavings = Audit._quantizeMetricSavings(product.metricSavings);
const hasSomeSavings = Object.values(metricSavings || {}).some(v => v);

if (scoreDisplayMode === Audit.SCORING_MODES.METRIC_SAVINGS) {
if (score && score >= Util.PASS_THRESHOLD) {
score = 1;
} else if (Object.values(product.metricSavings || {}).some(v => v)) {
} else if (hasSomeSavings) {
score = 0;
} else {
score = 0.5;
Expand Down Expand Up @@ -419,7 +458,7 @@ class Audit {
errorStack: product.errorStack,
warnings: product.warnings,
scoringOptions: product.scoringOptions,
metricSavings: product.metricSavings,
metricSavings,

details: product.details,
guidanceLevel: audit.meta.guidanceLevel,
Expand Down
8 changes: 8 additions & 0 deletions core/test/audits/audit-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,14 @@ describe('Audit', () => {
assert.strictEqual(auditResult.score, 0.3);
});

it('quantizes metric savings', () => {
const auditResult = Audit.generateAuditResult(PassOrFailAudit, {
score: 0,
metricSavings: {LCP: 0.1, FCP: 149, CLS: 0.0015, TBT: -100},
});
assert.deepStrictEqual(auditResult.metricSavings, {LCP: 0, FCP: 150, CLS: 0.002, TBT: 0});
});
adamraine marked this conversation as resolved.
Show resolved Hide resolved

it('chooses the title if score is passing', () => {
const auditResult = Audit.generateAuditResult(PassOrFailAudit, {score: 1});
assert.strictEqual(auditResult.score, 1);
Expand Down
26 changes: 13 additions & 13 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 @@ -529,7 +529,7 @@
"numericUnit": "millisecond",
"displayValue": "1.0 s",
"metricSavings": {
"TBT": 143
"TBT": 150
},
"details": {
"type": "table",
Expand Down Expand Up @@ -594,7 +594,7 @@
"numericUnit": "millisecond",
"displayValue": "0.3 s",
"metricSavings": {
"TBT": 142.50196940171634
"TBT": 150
},
"details": {
"type": "table",
Expand Down Expand Up @@ -1744,7 +1744,7 @@
"scoreDisplayMode": "informative",
"displayValue": "5 elements found",
"metricSavings": {
"CLS": 0.002631263732910156
"CLS": 0.003
},
"details": {
"type": "table",
Expand Down Expand Up @@ -1869,7 +1869,7 @@
"scoreDisplayMode": "informative",
"displayValue": "3 long tasks found",
"metricSavings": {
"TBT": 143
"TBT": 150
},
"details": {
"type": "table",
Expand Down Expand Up @@ -8996,7 +8996,7 @@
"numericUnit": "millisecond",
"displayValue": "0.9 s",
"metricSavings": {
"TBT": 122.83299999999986
"TBT": 100
},
"details": {
"type": "table",
Expand Down Expand Up @@ -9066,7 +9066,7 @@
"numericUnit": "millisecond",
"displayValue": "0.4 s",
"metricSavings": {
"TBT": 106.76186411159928
"TBT": 100
},
"details": {
"type": "table",
Expand Down Expand Up @@ -9874,7 +9874,7 @@
"scoreDisplayMode": "metricSavings",
"displayValue": "1 element found",
"metricSavings": {
"CLS": 0.13125
"CLS": 0.131
},
"details": {
"type": "table",
Expand Down Expand Up @@ -9923,7 +9923,7 @@
"scoreDisplayMode": "informative",
"displayValue": "1 long task found",
"metricSavings": {
"TBT": 122.83299999999986
"TBT": 100
},
"details": {
"type": "table",
Expand Down Expand Up @@ -10713,7 +10713,7 @@
"scoreDisplayMode": "informative",
"displayValue": "60 ms spent on event 'keypress'",
"metricSavings": {
"INP": 64
"INP": 50
},
"details": {
"type": "list",
Expand Down Expand Up @@ -18485,7 +18485,7 @@
"numericUnit": "millisecond",
"displayValue": "0.5 s",
"metricSavings": {
"TBT": 13
"TBT": 0
},
"details": {
"type": "table",
Expand Down Expand Up @@ -18550,7 +18550,7 @@
"numericUnit": "millisecond",
"displayValue": "0.2 s",
"metricSavings": {
"TBT": 25.85177364864864
"TBT": 50
Copy link
Member

Choose a reason for hiding this comment

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

Gah, I can see the appeal of using Math.floor() in examples like this. Because it's potential savings we don't want to overestimate the value. Agreed with @connorjclark that rounding 199 to 100 isn't great, but so is overpromising the savings by 2x here.

OTOH using a precision of 50 and Math.round() means the rounded value is only ever a max of 25ms away from the original value. To do the same with Math.floor() we'd have to drop the granularity to 25, which may be getting too precise again.

IMO in terms of purity (don't overpromise savings you won't be able to get) Math.floor() makes the most sense. However in practice, considering the chosen granularity levels and the lack of actual accuracy in the initial estimate, I think it probably doesn't matter, and so Math.round() works fine and arguably better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think the case I'm more worried about is this audit score goes from 0.5 to 1 just because of 25.x ms of savings. Perhaps a follow-up PR could introduce some minimum threshold for that condition as well.

},
"details": {
"type": "table",
Expand Down Expand Up @@ -19676,7 +19676,7 @@
"scoreDisplayMode": "informative",
"displayValue": "2 long tasks found",
"metricSavings": {
"TBT": 13
"TBT": 0
},
"details": {
"type": "table",
Expand Down Expand Up @@ -20988,7 +20988,7 @@
"warnings": [],
"metricSavings": {
"FCP": 0,
"LCP": 330
"LCP": 350
},
"details": {
"type": "opportunity",
Expand Down
22 changes: 11 additions & 11 deletions core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,8 @@
"numericUnit": "millisecond",
"displayValue": "Root document took 570 ms",
"metricSavings": {
"FCP": 468.46799999999996,
"LCP": 468.46799999999996
"FCP": 450,
"LCP": 450
},
"details": {
"type": "opportunity",
Expand Down Expand Up @@ -1106,7 +1106,7 @@
"numericUnit": "millisecond",
"displayValue": "2.2 s",
"metricSavings": {
"TBT": 1220.691999999999
"TBT": 1200
},
"details": {
"type": "table",
Expand Down Expand Up @@ -1171,7 +1171,7 @@
"numericUnit": "millisecond",
"displayValue": "1.3 s",
"metricSavings": {
"TBT": 1209.4020894768312
"TBT": 1200
},
"details": {
"type": "table",
Expand Down Expand Up @@ -2349,7 +2349,7 @@
"scoreDisplayMode": "informative",
"displayValue": "Third-party code blocked the main thread for 0 ms",
"metricSavings": {
"TBT": 23.424291876693196
"TBT": 0
},
"details": {
"type": "table",
Expand Down Expand Up @@ -2430,7 +2430,7 @@
"scoreDisplayMode": "metricSavings",
"displayValue": "13,320 ms",
"metricSavings": {
"LCP": 10819.961
"LCP": 10800
},
"details": {
"type": "list",
Expand Down Expand Up @@ -2530,7 +2530,7 @@
"scoreDisplayMode": "metricSavings",
"displayValue": "5 elements found",
"metricSavings": {
"CLS": 0.13570762803819444
"CLS": 0.136
},
"details": {
"type": "table",
Expand Down Expand Up @@ -2655,7 +2655,7 @@
"scoreDisplayMode": "informative",
"displayValue": "2 long tasks found",
"metricSavings": {
"TBT": 1220.691999999999
"TBT": 1200
},
"details": {
"type": "table",
Expand Down Expand Up @@ -4609,7 +4609,7 @@
"warnings": [],
"metricSavings": {
"FCP": 0,
"LCP": 610
"LCP": 600
},
"details": {
"type": "opportunity",
Expand Down Expand Up @@ -4919,7 +4919,7 @@
"displayValue": "Potential savings of 143 KiB",
"metricSavings": {
"FCP": 150,
"LCP": 1060
"LCP": 1050
},
"details": {
"type": "opportunity",
Expand Down Expand Up @@ -5195,7 +5195,7 @@
"numericUnit": "element",
"displayValue": "153 elements",
"metricSavings": {
"TBT": 1
Copy link
Member

Choose a reason for hiding this comment

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

lol 1

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, one of the reasons this PR exists

"TBT": 0
},
"details": {
"type": "table",
Expand Down
Loading