core(third-party-summary): correct blocking time calculation

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.
-      urlSummary.blockingTime += Math.max(taskDuration - 50, 0);
+      // 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);
     }

core/computed/tbt-impact-tasks.js

@@ -64,24 +64,35 @@ class TBTImpactTasks {
   /**
    * @param {LH.Artifacts.TaskNode[]} tasks
    * @param {Map<LH.Artifacts.TaskNode, number>} taskToImpact
+   * @param {Map<LH.Artifacts.TaskNode, number>} taskToBlockingTime
    */
-  static createImpactTasks(tasks, taskToImpact) {
+  static createImpactTasks(tasks, taskToImpact, taskToBlockingTime) {
     /** @type {LH.Artifacts.TBTImpactTask[]} */
     const tbtImpactTasks = [];
 
     for (const task of tasks) {
       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,
+        // 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),
       });
     }
 
@@ -98,6 +109,9 @@ class TBTImpactTasks {
     /** @type {Map<LH.Artifacts.TaskNode, number>} */
     const taskToImpact = new Map();
 
+    /** @type {Map<LH.Artifacts.TaskNode, number>} */
+    const taskToBlockingTime = new Map();
+
     for (const task of tasks) {
       const event = {
         start: task.startTime,
@@ -113,11 +127,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 +147,9 @@ class TBTImpactTasks {
     /** @type {Map<LH.Artifacts.TaskNode, number>} */
     const taskToImpact = new Map();
 
+    /** @type {Map<LH.Artifacts.TaskNode, number>} */
+    const taskToBlockingTime = new Map();
+
     /** @type {Map<LH.Artifacts.TaskNode, {start: number, end: number, duration: number}>} */
     const topLevelTaskToEvent = new Map();
 
@@ -151,19 +170,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 +204,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);
   }
 
   /**

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",

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)');
   });

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 () => {

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';
@@ -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,
         },
       ]);
@@ -241,7 +251,7 @@ describe('TBTImpactTasks', () => {
       expect(tasks.every(t => t.selfTbtImpact >= 0)).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.
@@ -250,7 +260,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);
@@ -281,10 +297,11 @@ describe('TBTImpactTasks', () => {
       };
 
       const tasks = await TBTImpactTasks.request(metricComputationData, context);
+
       expect(tasks.every(t => t.selfTbtImpact >= 0)).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.
@@ -293,7 +310,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);