From 42d7155e46eba23bda19415653e9d25c284161b7 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 4 Apr 2024 14:39:29 -0700 Subject: [PATCH 1/3] core(trace-processor): correct overlapping tasks --- core/lib/tracehouse/trace-processor.js | 16 +++++++- .../lib/tracehouse/trace-processor-test.js | 41 +++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/core/lib/tracehouse/trace-processor.js b/core/lib/tracehouse/trace-processor.js index 3be137687c69..88de68a8ff1f 100644 --- a/core/lib/tracehouse/trace-processor.js +++ b/core/lib/tracehouse/trace-processor.js @@ -403,6 +403,9 @@ class TraceProcessor { */ static getMainThreadTopLevelEvents(trace, startTime = 0, endTime = Infinity) { const topLevelEvents = []; + /** @type {ToplevelEvent|undefined} */ + let prevToplevel = undefined; + // note: mainThreadEvents is already sorted by event start for (const event of trace.mainThreadEvents) { if (!this.isScheduleableTask(event) || !event.dur) continue; @@ -411,11 +414,20 @@ class TraceProcessor { const end = (event.ts + event.dur - trace.timeOriginEvt.ts) / 1000; if (start > endTime || end < startTime) continue; - topLevelEvents.push({ + // Temporary fix for a Chrome bug where some RunTask events can be overlapping. + // We correct that here be ensuring each RunTask ends at least 1 microsecond before the next + // https://crbug.com/40825479 + if (prevToplevel && start < prevToplevel.end) { + prevToplevel.end = start - 0.001; + } + + prevToplevel = { start, end, duration: event.dur / 1000, - }); + }; + + topLevelEvents.push(prevToplevel); } return topLevelEvents; diff --git a/core/test/lib/tracehouse/trace-processor-test.js b/core/test/lib/tracehouse/trace-processor-test.js index 5ddd589b7c07..c07371621081 100644 --- a/core/test/lib/tracehouse/trace-processor-test.js +++ b/core/test/lib/tracehouse/trace-processor-test.js @@ -331,6 +331,47 @@ describe('TraceProcessor', () => { assert.equal(ret[1].end, 2000); assert.equal(ret[1].duration, 1000); }); + + it('corrects overlapping tasks', () => { + const baseTime = 20_000_000; + const name = 'RunTask'; + const processedTrace = { + timeOriginEvt: {ts: baseTime}, + mainThreadEvents: [ + // 10ms to 100ms + {ts: baseTime + 10_000, dur: 90_000, name}, + // 40ms to 60ms + {ts: baseTime + 40_000, dur: 20_000, name}, + // 70ms to 90ms + {ts: baseTime + 70_000, dur: 20_000, name}, + // 100ms to 120ms + {ts: baseTime + 100_000, dur: 20_000, name}, + ], + }; + + const ret = TraceProcessor.getMainThreadTopLevelEvents( + processedTrace, + 0, + 2000 + ); + assert.equal(ret.length, 4); + + assert.equal(ret[0].start, 10); + assert.equal(ret[0].end, 39.999); + assert.equal(ret[0].duration, 90); + + assert.equal(ret[1].start, 40); + assert.equal(ret[1].end, 60); + assert.equal(ret[1].duration, 20); + + assert.equal(ret[2].start, 70); + assert.equal(ret[2].end, 90); + assert.equal(ret[2].duration, 20); + + assert.equal(ret[3].start, 100); + assert.equal(ret[3].end, 120); + assert.equal(ret[3].duration, 20); + }); }); describe('getMainThreadTopLevelEventDurations', () => { From 00f895f6b586090a9a8e2784ee1c0f65939bfe03 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 4 Apr 2024 14:41:39 -0700 Subject: [PATCH 2/3] ope --- core/lib/tracehouse/trace-processor.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/lib/tracehouse/trace-processor.js b/core/lib/tracehouse/trace-processor.js index 88de68a8ff1f..e145066c62dc 100644 --- a/core/lib/tracehouse/trace-processor.js +++ b/core/lib/tracehouse/trace-processor.js @@ -416,7 +416,8 @@ class TraceProcessor { // Temporary fix for a Chrome bug where some RunTask events can be overlapping. // We correct that here be ensuring each RunTask ends at least 1 microsecond before the next - // https://crbug.com/40825479 + // https://github.com/GoogleChrome/lighthouse/issues/15896 + // https://issues.chromium.org/issues/329678173 if (prevToplevel && start < prevToplevel.end) { prevToplevel.end = start - 0.001; } From ec4da48f799c9b1560f47a00e7a4701557e58fb2 Mon Sep 17 00:00:00 2001 From: Adam Raine Date: Thu, 4 Apr 2024 14:56:16 -0700 Subject: [PATCH 3/3] tests --- core/test/audits/long-tasks-test.js | 2 +- core/test/audits/mainthread-work-breakdown-test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/test/audits/long-tasks-test.js b/core/test/audits/long-tasks-test.js index fe87d5a496af..08f89373cdde 100644 --- a/core/test/audits/long-tasks-test.js +++ b/core/test/audits/long-tasks-test.js @@ -304,7 +304,7 @@ describe('Long tasks audit', () => { }], }, }); - expect(result.metricSavings.TBT).toBeApproximately(353.53); + expect(result.metricSavings.TBT).toBeApproximately(171.95); const debugData = result.details.debugData; expect(debugData).toStrictEqual({ diff --git a/core/test/audits/mainthread-work-breakdown-test.js b/core/test/audits/mainthread-work-breakdown-test.js index 6866139de349..3a5f7e50ea90 100644 --- a/core/test/audits/mainthread-work-breakdown-test.js +++ b/core/test/audits/mainthread-work-breakdown-test.js @@ -120,7 +120,7 @@ Object { expect(Math.round(output.numericValue)).toMatchInlineSnapshot(`979`); assert.equal(output.details.items.length, 7); assert.equal(output.score, 1); - expect(output.metricSavings.TBT).toBeCloseTo(353.5, 0.1); + expect(output.metricSavings.TBT).toBeCloseTo(171.95, 0.1); }); it('should compute the correct values for the load trace (legacy)', async () => {