From 5e55a642401cd8387cc31d45f27608e7be8df33a Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 16 Jul 2024 17:39:06 -0500 Subject: [PATCH 1/6] core(third-party-summary): correct blocking time calculation --- core/audits/third-party-summary.js | 2 +- core/computed/tbt-impact-tasks.js | 28 +++++++++-- core/lib/tracehouse/main-thread-tasks.js | 2 + .../third-party-summary-test.js.snap | 46 +++++++++---------- core/test/audits/third-party-summary-test.js | 8 ++-- core/test/computed/tbt-impact-tasks-test.js | 12 ++++- types/artifacts.d.ts | 2 +- 7 files changed, 66 insertions(+), 34 deletions(-) diff --git a/core/audits/third-party-summary.js b/core/audits/third-party-summary.js index bb2fc70d3073..1df99923a9c6 100644 --- a/core/audits/third-party-summary.js +++ b/core/audits/third-party-summary.js @@ -104,7 +104,7 @@ class ThirdPartySummary extends Audit { // The amount of time spent *blocking* on main thread is the sum of all time longer than 50ms. // Note that this is not totally equivalent to the TBT definition since it fails to account for FCP, // but a majority of third-party work occurs after FCP and should yield largely similar numbers. - urlSummary.blockingTime += Math.max(taskDuration - 50, 0); + urlSummary.blockingTime += task.selfBlockingTime; urlSummary.tbtImpact += task.selfTbtImpact; byURL.set(attributableURL, urlSummary); } diff --git a/core/computed/tbt-impact-tasks.js b/core/computed/tbt-impact-tasks.js index 1a8097cff1f7..279b0fcb8810 100644 --- a/core/computed/tbt-impact-tasks.js +++ b/core/computed/tbt-impact-tasks.js @@ -64,8 +64,9 @@ class TBTImpactTasks { /** * @param {LH.Artifacts.TaskNode[]} tasks * @param {Map} taskToImpact + * @param {Map} taskToBlockingTime */ - static createImpactTasks(tasks, taskToImpact) { + static createImpactTasks(tasks, taskToImpact, taskToBlockingTime) { /** @type {LH.Artifacts.TBTImpactTask[]} */ const tbtImpactTasks = []; @@ -73,15 +74,22 @@ class TBTImpactTasks { const tbtImpact = taskToImpact.get(task) || 0; let selfTbtImpact = tbtImpact; + const blockingTime = taskToBlockingTime.get(task) || 0; + let selfBlockingTime = blockingTime; + for (const child of task.children) { const childTbtImpact = taskToImpact.get(child) || 0; selfTbtImpact -= childTbtImpact; + + const childBlockingTime = taskToBlockingTime.get(child) || 0; + selfBlockingTime -= childBlockingTime; } tbtImpactTasks.push({ ...task, tbtImpact, selfTbtImpact, + selfBlockingTime, }); } @@ -98,6 +106,9 @@ class TBTImpactTasks { /** @type {Map} */ const taskToImpact = new Map(); + /** @type {Map} */ + const taskToBlockingTime = new Map(); + for (const task of tasks) { const event = { start: task.startTime, @@ -113,11 +124,13 @@ class TBTImpactTasks { }; const tbtImpact = calculateTbtImpactForEvent(event, startTimeMs, endTimeMs, topLevelEvent); + const blockingTime = calculateTbtImpactForEvent(event, -Infinity, Infinity, topLevelEvent); taskToImpact.set(task, tbtImpact); + taskToBlockingTime.set(task, blockingTime); } - return this.createImpactTasks(tasks, taskToImpact); + return this.createImpactTasks(tasks, taskToImpact, taskToBlockingTime); } /** @@ -131,6 +144,9 @@ class TBTImpactTasks { /** @type {Map} */ const taskToImpact = new Map(); + /** @type {Map} */ + const taskToBlockingTime = new Map(); + /** @type {Map} */ const topLevelTaskToEvent = new Map(); @@ -151,19 +167,21 @@ class TBTImpactTasks { }; const tbtImpact = calculateTbtImpactForEvent(event, startTimeMs, endTimeMs); + const blockingTime = calculateTbtImpactForEvent(event, -Infinity, Infinity); const task = traceEventToTask.get(node.event); if (!task) continue; topLevelTaskToEvent.set(task, event); taskToImpact.set(task, tbtImpact); + taskToBlockingTime.set(task, blockingTime); } // Interpolate the TBT impact of remaining tasks using the top level ancestor tasks. // We don't have any lantern estimates for tasks that are not top level, so we need to estimate // the lantern timing based on the task's observed timing relative to it's top level task's observed timing. for (const task of tasks) { - if (taskToImpact.has(task)) continue; + if (taskToImpact.has(task) || taskToBlockingTime.has(task)) continue; const topLevelTask = this.getTopLevelTask(task); @@ -183,11 +201,13 @@ class TBTImpactTasks { }; const tbtImpact = calculateTbtImpactForEvent(event, startTimeMs, endTimeMs, topLevelEvent); + const blockingTime = calculateTbtImpactForEvent(event, -Infinity, Infinity, topLevelEvent); taskToImpact.set(task, tbtImpact); + taskToBlockingTime.set(task, blockingTime); } - return this.createImpactTasks(tasks, taskToImpact); + return this.createImpactTasks(tasks, taskToImpact, taskToBlockingTime); } /** diff --git a/core/lib/tracehouse/main-thread-tasks.js b/core/lib/tracehouse/main-thread-tasks.js index 8496dc40109f..eb9df2dec2ab 100644 --- a/core/lib/tracehouse/main-thread-tasks.js +++ b/core/lib/tracehouse/main-thread-tasks.js @@ -37,6 +37,7 @@ import {taskGroups, taskNameToGroup} from './task-groups.js'; * @prop {number} endTime * @prop {number} duration * @prop {number} selfTime + * @prop {number} selfBlockingTime * @prop {string[]} attributableURLs * @prop {TaskGroup} group */ @@ -73,6 +74,7 @@ class MainThreadTasks { attributableURLs: [], group: taskGroups.other, selfTime: NaN, + selfBlockingTime: NaN, }; return newTask; diff --git a/core/test/audits/__snapshots__/third-party-summary-test.js.snap b/core/test/audits/__snapshots__/third-party-summary-test.js.snap index 88264c1716c6..3cb28ba2f1f6 100644 --- a/core/test/audits/__snapshots__/third-party-summary-test.js.snap +++ b/core/test/audits/__snapshots__/third-party-summary-test.js.snap @@ -3,13 +3,13 @@ exports[`Third party summary surface the discovered third parties 1`] = ` Array [ Object { - "blockingTime": 223.56, + "blockingTime": 225.30612021679366, "entity": "Wunderkind", "mainThreadTime": 879.9769999999949, "subItems": Object { "items": Array [ Object { - "blockingTime": 223.56, + "blockingTime": 225.30612021679366, "mainThreadTime": 287.06299999999925, "tbtImpact": 225.30612021679366, "transferSize": 16051, @@ -547,13 +547,13 @@ Array [ "transferSize": 450838, }, Object { - "blockingTime": 21.476, + "blockingTime": 64.66899999999367, "entity": "Google/Doubleclick Ads", "mainThreadTime": 347.9769999999987, "subItems": Object { "items": Array [ Object { - "blockingTime": 21.476, + "blockingTime": 64.66899999999367, "mainThreadTime": 339.7179999999987, "tbtImpact": 10.898000000000135, "transferSize": 145049, @@ -1118,6 +1118,25 @@ Array [ "tbtImpact": 10.898000000000135, "transferSize": 571412, }, + Object { + "blockingTime": 8.445999999999897, + "entity": "script.ac", + "mainThreadTime": 462.9409999999911, + "subItems": Object { + "items": Array [ + Object { + "blockingTime": 8.445999999999897, + "mainThreadTime": 462.9409999999911, + "tbtImpact": 8.445999999999897, + "transferSize": 50609, + "url": "https://cadmus.script.ac/d2uap9jskdzp2/script.js", + }, + ], + "type": "subitems", + }, + "tbtImpact": 8.445999999999897, + "transferSize": 50609, + }, Object { "blockingTime": 0, "entity": "gobankingrates.com", @@ -3343,25 +3362,6 @@ Array [ "tbtImpact": 0, "transferSize": 68466, }, - Object { - "blockingTime": 0, - "entity": "script.ac", - "mainThreadTime": 462.9409999999911, - "subItems": Object { - "items": Array [ - Object { - "blockingTime": 0, - "mainThreadTime": 462.9409999999911, - "tbtImpact": 8.445999999999897, - "transferSize": 50609, - "url": "https://cadmus.script.ac/d2uap9jskdzp2/script.js", - }, - ], - "type": "subitems", - }, - "tbtImpact": 8.445999999999897, - "transferSize": 50609, - }, Object { "blockingTime": 0, "entity": "myfinance.com", diff --git a/core/test/audits/third-party-summary-test.js b/core/test/audits/third-party-summary-test.js index cf88472fc139..f8fffb49fd49 100644 --- a/core/test/audits/third-party-summary-test.js +++ b/core/test/audits/third-party-summary-test.js @@ -26,10 +26,10 @@ describe('Third party summary', () => { settings.throttlingMethod = 'devtools'; const results = await ThirdPartySummary.audit(artifacts, {computedCache: new Map(), settings}); - expect(results.score).toBe(1); + expect(results.score).toBe(0); expect(results.metricSavings).toEqual({TBT: 245}); expect(results.displayValue).toBeDisplayString( - 'Third-party code blocked the main thread for 250 ms' + 'Third-party code blocked the main thread for 300 ms' ); expect(results.details.items).toMatchSnapshot(); }); @@ -49,9 +49,9 @@ describe('Third party summary', () => { expect(results.metricSavings).toEqual({TBT: 2570}); expect(results.details.items).toHaveLength(145); expect(Math.round(results.details.items[0].mainThreadTime)).toEqual(3520); - expect(Math.round(results.details.items[0].blockingTime)).toEqual(1103); + expect(Math.round(results.details.items[0].blockingTime)).toEqual(1182); expect(Math.round(results.details.items[1].mainThreadTime)).toEqual(1392); - expect(Math.round(results.details.items[1].blockingTime)).toEqual(659); + expect(Math.round(results.details.items[1].blockingTime)).toEqual(508); }); it('be not applicable when no third parties are present', async () => { diff --git a/core/test/computed/tbt-impact-tasks-test.js b/core/test/computed/tbt-impact-tasks-test.js index bfed8fff3ffc..b40b42939d69 100644 --- a/core/test/computed/tbt-impact-tasks-test.js +++ b/core/test/computed/tbt-impact-tasks-test.js @@ -115,23 +115,28 @@ describe('TBTImpactTasks', () => { expect(tbtImpactTasks).toMatchObject([ { tbtImpact: 3750, // 4000 (dur) - 200 (FCP cutoff) - 50 (blocking threshold) + selfBlockingTime: 2962.5, // 4000 (dur) - 50 (blocking threshold) - 493.75 - 493.75 selfTbtImpact: 2862.5, // 3750 - 393.75 - 493.75 }, { tbtImpact: 393.75, // 500 (dur) - 100 (FCP cutoff) - 6.25 (50 * 500 / 4000) + selfBlockingTime: 493.75, // 500 (dur) - 6.25 (50 * 500 / 4000) selfTbtImpact: 393.75, // No children }, { tbtImpact: 493.75, // 500 (dur) - 6.25 (50 * 500 / 4000) + selfBlockingTime: 493.75, // 500 (dur) - 6.25 (50 * 500 / 4000) selfTbtImpact: 493.75, // No children }, { tbtImpact: 950, // 3000 (dur) - 2000 (TTI cutoff) - 50 + selfBlockingTime: 2950, // 3000 (dur) - 50 (blocking threshold) selfTbtImpact: 950, // No children }, { // Included in test trace by default tbtImpact: 0, + selfBlockingTime: 0, selfTbtImpact: 0, }, ]); @@ -203,23 +208,28 @@ describe('TBTImpactTasks', () => { expect(tbtImpactTasks).toMatchObject([ { tbtImpact: 15_150, // 16_000 (dur) - 800 (FCP cutoff) - 50 (blocking threshold) - selfTbtImpact: 11562.5, // 15_150 - 1593.75 - 1993.75 + selfBlockingTime: 11_962.5, // 16_000 (dur) - 50 (blocking threshold) - 1993.75 - 1993.75 + selfTbtImpact: 11_562.5, // 15_150 - 1593.75 - 1993.75 }, { tbtImpact: 1593.75, // 2000 (dur) - 400 (FCP cutoff) - 6.25 (50 * 2000 / 16_000) + selfBlockingTime: 1993.75, // 2000 (dur) - 6.25 (50 * 2000 / 16_000) selfTbtImpact: 1593.75, // No children }, { tbtImpact: 1993.75, // 2000 (dur) - 6.25 (50 * 2000 / 16_000) + selfBlockingTime: 1993.75, // 2000 (dur) - 6.25 (50 * 2000 / 16_000) selfTbtImpact: 1993.75, // No children }, { tbtImpact: 3950, // 12_000 (dur) - 8000 (TTI cutoff) - 50 + selfBlockingTime: 11_950, // 12_000 (dur) - 50 selfTbtImpact: 3950, // No children }, { // Included in test trace by default tbtImpact: 0, + selfBlockingTime: 0, selfTbtImpact: 0, }, ]); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 1dd3faddb62d..6748d35c0fd1 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -163,7 +163,7 @@ declare module Artifacts { type NetworkRequest = _NetworkRequest; type TaskNode = _TaskNode; - type TBTImpactTask = TaskNode & {tbtImpact: number, selfTbtImpact: number}; + type TBTImpactTask = TaskNode & {tbtImpact: number, selfTbtImpact: number, selfBlockingTime: number}; type MetaElement = Artifacts['MetaElements'][0]; interface URL { From aedc566134bcf9e2bd93294991fbfb333e9d0916 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 16 Jul 2024 17:50:59 -0500 Subject: [PATCH 2/6] cnn test --- core/test/computed/tbt-impact-tasks-test.js | 31 +++++++++++++++------ 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/core/test/computed/tbt-impact-tasks-test.js b/core/test/computed/tbt-impact-tasks-test.js index b40b42939d69..f7212f4c7e61 100644 --- a/core/test/computed/tbt-impact-tasks-test.js +++ b/core/test/computed/tbt-impact-tasks-test.js @@ -12,8 +12,8 @@ import {createTestTrace, rootFrame} from '../create-test-trace.js'; import {networkRecordsToDevtoolsLog} from '../network-records-to-devtools-log.js'; import {MainThreadTasks} from '../../computed/main-thread-tasks.js'; -const trace = readJson('../fixtures/traces/lcp-m78.json', import.meta); -const devtoolsLog = readJson('../fixtures/traces/lcp-m78.devtools.log.json', import.meta); +const trace = readJson('../fixtures/artifacts/cnn/defaultPass.trace.json.gz', import.meta); +const devtoolsLog = readJson('../fixtures/artifacts/cnn/defaultPass.devtoolslog.json.gz', import.meta); describe('TBTImpactTasks', () => { const mainDocumentUrl = 'https://example.com'; @@ -248,10 +248,11 @@ describe('TBTImpactTasks', () => { }; const tasks = await TBTImpactTasks.request(metricComputationData, context); - expect(tasks.every(t => t.selfTbtImpact >= 0)).toBeTruthy(); + // Should be >= 0 but provide some wiggle room for double precision + expect(tasks.every(t => t.selfTbtImpact >= -0.00001)).toBeTruthy(); const tasksImpactingTbt = tasks.filter(t => t.tbtImpact); - expect(tasksImpactingTbt.length).toMatchInlineSnapshot(`59`); + expect(tasksImpactingTbt.length).toMatchInlineSnapshot(`7374`); // Only tasks with no children should have a `selfTbtImpact` that equals `tbtImpact` if // `tbtImpact` is nonzero. @@ -260,7 +261,13 @@ describe('TBTImpactTasks', () => { expect(tasksWithNoChildren).toEqual(tasksWithAllSelfImpact); const totalSelfImpact = tasksImpactingTbt.reduce((sum, t) => sum += t.selfTbtImpact, 0); - expect(totalSelfImpact).toMatchInlineSnapshot(`1234`); + expect(totalSelfImpact).toMatchInlineSnapshot(`2819.9999999999577`); + + // Total self blocking time is just the total self impact without factoring in the TBT + // bounds, so it should always be greater than or equal to the total TBT self impact. + const totalSelfBlockingTime = tasksImpactingTbt + .reduce((sum, t) => sum += t.selfBlockingTime, 0); + expect(totalSelfImpact).toBeGreaterThanOrEqual(totalSelfBlockingTime); // The total self TBT impact of every task should equal the total TBT impact of just the top level tasks. const topLevelTasks = tasksImpactingTbt.filter(t => !t.parent); @@ -291,10 +298,12 @@ describe('TBTImpactTasks', () => { }; const tasks = await TBTImpactTasks.request(metricComputationData, context); - expect(tasks.every(t => t.selfTbtImpact >= 0)).toBeTruthy(); + + // Should be >= 0 but provide some wiggle room for double precision + expect(tasks.every(t => t.selfTbtImpact >= -0.00001)).toBeTruthy(); const tasksImpactingTbt = tasks.filter(t => t.tbtImpact); - expect(tasksImpactingTbt.length).toMatchInlineSnapshot(`5`); + expect(tasksImpactingTbt.length).toMatchInlineSnapshot(`1722`); // Only tasks with no children should have a `selfTbtImpact` that equals `tbtImpact` if // `tbtImpact` is nonzero. @@ -303,7 +312,13 @@ describe('TBTImpactTasks', () => { expect(tasksWithNoChildren).toEqual(tasksWithAllSelfImpact); const totalSelfImpact = tasksImpactingTbt.reduce((sum, t) => sum += t.selfTbtImpact, 0); - expect(totalSelfImpact).toMatchInlineSnapshot(`333.0050000000001`); + expect(totalSelfImpact).toMatchInlineSnapshot(`400.039`); + + // Total self blocking time is just the total self impact without factoring in the TBT + // bounds, so it should always be greater than or equal to the total TBT self impact. + const totalSelfBlockingTime = tasksImpactingTbt + .reduce((sum, t) => sum += t.selfBlockingTime, 0); + expect(totalSelfImpact).toBeGreaterThanOrEqual(totalSelfBlockingTime); // The total self TBT impact of every task should equal the total TBT impact of just the top level tasks. const topLevelTasks = tasksImpactingTbt.filter(t => !t.parent); From 44ffd49cb3b597ba8d934a3da3988928e90062f7 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 16 Jul 2024 18:00:21 -0500 Subject: [PATCH 3/6] ope --- core/lib/tracehouse/main-thread-tasks.js | 2 -- core/test/audits/third-party-facades-test.js | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/core/lib/tracehouse/main-thread-tasks.js b/core/lib/tracehouse/main-thread-tasks.js index eb9df2dec2ab..8496dc40109f 100644 --- a/core/lib/tracehouse/main-thread-tasks.js +++ b/core/lib/tracehouse/main-thread-tasks.js @@ -37,7 +37,6 @@ import {taskGroups, taskNameToGroup} from './task-groups.js'; * @prop {number} endTime * @prop {number} duration * @prop {number} selfTime - * @prop {number} selfBlockingTime * @prop {string[]} attributableURLs * @prop {TaskGroup} group */ @@ -74,7 +73,6 @@ class MainThreadTasks { attributableURLs: [], group: taskGroups.other, selfTime: NaN, - selfBlockingTime: NaN, }; return newTask; diff --git a/core/test/audits/third-party-facades-test.js b/core/test/audits/third-party-facades-test.js index cbf6f8c3334f..a503faf65ad5 100644 --- a/core/test/audits/third-party-facades-test.js +++ b/core/test/audits/third-party-facades-test.js @@ -468,7 +468,7 @@ Array [ expect(results.score).toBe(0); expect(results.metricSavings).toEqual({TBT: 145}); expect(results.displayValue).toBeDisplayString('1 facade alternative available'); - expect(results.details.items[0].blockingTime).toEqual(134.076); // TBT impact is not equal to the blocking time + expect(results.details.items[0].blockingTime).toBeCloseTo(145); expect(results.details.items[0].product) .toBeDisplayString('Intercom Widget (Customer Success)'); }); From fe14152965699df4b94331764243fbe6206354a2 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Tue, 16 Jul 2024 18:22:26 -0500 Subject: [PATCH 4/6] sample --- .../reports/sample-flow-result.json | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/core/test/fixtures/user-flows/reports/sample-flow-result.json b/core/test/fixtures/user-flows/reports/sample-flow-result.json index c6ff0b94bb24..f6252e5e984e 100644 --- a/core/test/fixtures/user-flows/reports/sample-flow-result.json +++ b/core/test/fixtures/user-flows/reports/sample-flow-result.json @@ -1509,7 +1509,7 @@ "description": "Third-party code can significantly impact load performance. Limit the number of redundant third-party providers and try to load third-party code after your page has primarily finished loading. [Learn how to minimize third-party impact](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/loading-third-party-javascript/).", "score": 1, "scoreDisplayMode": "informative", - "displayValue": "Third-party code blocked the main thread for 30 ms", + "displayValue": "Third-party code blocked the main thread for 40 ms", "metricSavings": { "TBT": 50 }, @@ -1547,7 +1547,7 @@ "items": [ { "mainThreadTime": 163.40800000000056, - "blockingTime": 33.06, + "blockingTime": 39.99999999999996, "transferSize": 98875, "tbtImpact": 39.99999999999996, "entity": "Google Tag Manager", @@ -1557,7 +1557,7 @@ { "url": "https://www.googletagmanager.com/gtag/js?id=G-RTW9M3W5HC", "mainThreadTime": 163.40800000000056, - "blockingTime": 33.06, + "blockingTime": 39.99999999999996, "transferSize": 98875, "tbtImpact": 39.99999999999996 } @@ -1619,7 +1619,7 @@ ], "summary": { "wastedBytes": 116396, - "wastedMs": 33.06 + "wastedMs": 39.99999999999996 }, "isEntityGrouped": true }, @@ -7347,7 +7347,7 @@ "core/audits/third-party-summary.js | displayValue": [ { "values": { - "timeInMs": 33.06 + "timeInMs": 39.99999999999996 }, "path": "audits[third-party-summary].displayValue" } @@ -19199,7 +19199,7 @@ "description": "Third-party code can significantly impact load performance. Limit the number of redundant third-party providers and try to load third-party code after your page has primarily finished loading. [Learn how to minimize third-party impact](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/loading-third-party-javascript/).", "score": 1, "scoreDisplayMode": "informative", - "displayValue": "Third-party code blocked the main thread for 30 ms", + "displayValue": "Third-party code blocked the main thread for 70 ms", "metricSavings": { "TBT": 50 }, @@ -19237,7 +19237,7 @@ "items": [ { "mainThreadTime": 217.02400000000034, - "blockingTime": 31.823999999999998, + "blockingTime": 65.0000000000002, "transferSize": 0, "tbtImpact": 65.0000000000002, "entity": "Google Tag Manager", @@ -19247,7 +19247,7 @@ { "url": "https://www.googletagmanager.com/gtag/js?id=G-RTW9M3W5HC", "mainThreadTime": 217.02400000000034, - "blockingTime": 31.823999999999998, + "blockingTime": 65.0000000000002, "transferSize": 0, "tbtImpact": 65.0000000000002 } @@ -19309,7 +19309,7 @@ ], "summary": { "wastedBytes": 17, - "wastedMs": 31.823999999999998 + "wastedMs": 65.0000000000002 }, "isEntityGrouped": true }, @@ -25241,7 +25241,7 @@ "core/audits/third-party-summary.js | displayValue": [ { "values": { - "timeInMs": 31.823999999999998 + "timeInMs": 65.0000000000002 }, "path": "audits[third-party-summary].displayValue" } From ed29b83d419156e469f968d2e2e8a9a20ad9521c Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Wed, 17 Jul 2024 11:32:50 -0500 Subject: [PATCH 5/6] comment --- core/audits/third-party-summary.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/core/audits/third-party-summary.js b/core/audits/third-party-summary.js index 1df99923a9c6..d0898f256a50 100644 --- a/core/audits/third-party-summary.js +++ b/core/audits/third-party-summary.js @@ -101,10 +101,13 @@ class ThirdPartySummary extends Audit { const taskDuration = task.selfTime * cpuMultiplier; // The amount of time spent on main thread is the sum of all durations. urlSummary.mainThreadTime += taskDuration; - // The amount of time spent *blocking* on main thread is the sum of all time longer than 50ms. - // Note that this is not totally equivalent to the TBT definition since it fails to account for FCP, - // but a majority of third-party work occurs after FCP and should yield largely similar numbers. + // Blocking time is the amount of time spent on the main thread *over* 50ms. + // This value is interpolated because not all tasks attributed to this URL are at the top level. + // + // Note that this is not totally equivalent to the TBT definition since it fails to account for + // the FCP&TTI bounds of TBT. urlSummary.blockingTime += task.selfBlockingTime; + // TBT impact is similar to blocking time, but it accounts for the FCP&TTI bounds of TBT. urlSummary.tbtImpact += task.selfTbtImpact; byURL.set(attributableURL, urlSummary); } From 7e38a5106b560d9f6db8439a1bdf5e87ba9ca5be Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Fri, 2 Aug 2024 16:01:31 -0700 Subject: [PATCH 6/6] coerce to 0 --- core/computed/tbt-impact-tasks.js | 9 ++++++--- core/test/computed/tbt-impact-tasks-test.js | 6 ++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/core/computed/tbt-impact-tasks.js b/core/computed/tbt-impact-tasks.js index 279b0fcb8810..0d5a9ab636b7 100644 --- a/core/computed/tbt-impact-tasks.js +++ b/core/computed/tbt-impact-tasks.js @@ -87,9 +87,12 @@ class TBTImpactTasks { tbtImpactTasks.push({ ...task, - tbtImpact, - selfTbtImpact, - selfBlockingTime, + // Floating point numbers are not perfectly precise, so the subtraction operations above + // can sometimes output negative numbers close to 0 here. To prevent potentially confusing + // output we should bump those values to 0. + tbtImpact: Math.max(tbtImpact, 0), + selfTbtImpact: Math.max(selfTbtImpact, 0), + selfBlockingTime: Math.max(selfBlockingTime, 0), }); } diff --git a/core/test/computed/tbt-impact-tasks-test.js b/core/test/computed/tbt-impact-tasks-test.js index f7212f4c7e61..3a2d9c8c79d1 100644 --- a/core/test/computed/tbt-impact-tasks-test.js +++ b/core/test/computed/tbt-impact-tasks-test.js @@ -248,8 +248,7 @@ describe('TBTImpactTasks', () => { }; const tasks = await TBTImpactTasks.request(metricComputationData, context); - // Should be >= 0 but provide some wiggle room for double precision - expect(tasks.every(t => t.selfTbtImpact >= -0.00001)).toBeTruthy(); + expect(tasks.every(t => t.selfTbtImpact >= 0)).toBeTruthy(); const tasksImpactingTbt = tasks.filter(t => t.tbtImpact); expect(tasksImpactingTbt.length).toMatchInlineSnapshot(`7374`); @@ -299,8 +298,7 @@ describe('TBTImpactTasks', () => { const tasks = await TBTImpactTasks.request(metricComputationData, context); - // Should be >= 0 but provide some wiggle room for double precision - expect(tasks.every(t => t.selfTbtImpact >= -0.00001)).toBeTruthy(); + expect(tasks.every(t => t.selfTbtImpact >= 0)).toBeTruthy(); const tasksImpactingTbt = tasks.filter(t => t.tbtImpact); expect(tasksImpactingTbt.length).toMatchInlineSnapshot(`1722`);