core(tbt): Clean up total blocking time code #9409
Conversation
deepanjanroy
requested review from
brendankenny,
exterkamp,
patrickhulce and
paulirish
as code owners
deepanjanroy
changed the title
|
I'd like to get this into |
There was a problem hiding this comment.
thanks @deepanjanroy, I'm really excited about this metric! 😃
lighthouse-core/test/computed/metrics/total-blocking-time-test.js
Outdated
Show resolved
Hide resolved
lighthouse-core/test/computed/metrics/total-blocking-time-test.js
Outdated
Show resolved
Hide resolved
lighthouse-core/test/computed/metrics/total-blocking-time-test.js
Outdated
Show resolved
Hide resolved
| @@ -35,5 +34,6 @@ Object { | |||
| "observedTraceEndTs": 225426711887, | |||
| "speedIndex": 1657, | |||
| "speedIndexTs": undefined, | |||
| "totalBlockingTime": 726, | |||
There was a problem hiding this comment.
now that the clipping order has moved back to the original, the fact that this changes is unexpected to me.
could we perhaps add a new unit test case that would be different between the two versions to capture this more explicitly?
from my reading it should only be different in cases that I thought to be impossible
There was a problem hiding this comment.
Hm very good point - thanks for flagging this. I looked into it, and looks like the impossible is happening - TTI is in the middle of a task for the lantern test case (and it's not at FMP or DCL). Right now I'm trying to understand why that is the case.
Side note: It looks like lantern TTI does not consider DCL. I see this at line 70 of lantern-interactive.js. Is this expected?
const lastTaskAt = LanternInteractive.getLastLongTaskEndTime(simulationResult.nodeTimings);
const minimumTime = extras.optimistic
? extras.fmpResult.optimisticEstimate.timeInMs
: extras.fmpResult.pessimisticEstimate.timeInMs;
return {
timeInMs: Math.max(minimumTime, lastTaskAt),
nodeTimings: simulationResult.nodeTimings,
};There was a problem hiding this comment.
Oh, right because we are using the TTI estimate timeInMs of a potentially different graph so it is possible.
Re: DCL, Yeah there's not a reliable way we have to predict when DCL will occur under a hypothetical connection type so it's not considered.
AI from my perspective: we add a test case just exercising the TTI clipping behavior with a comment about why this can happen
There was a problem hiding this comment.
Ah right. That sgtm: added test case just for TTI clipping.
lighthouse-core/test/computed/metrics/total-blocking-time-test.js
Outdated
Show resolved
Hide resolved
| {start: 1000, end: 1110, duration: 110}, // Contributes 10ms. | ||
| {start: 2000, end: 2200, duration: 200}, // Contributes 50ms. | ||
| // The clipping is done first, so the task becomes [150, 200] after clipping and contributes | ||
| // 0ms of blocking time. This is in contrast to first calculating the blocking region ([100, |
There was a problem hiding this comment.
would you mind adding a few other edge cases?
The fact that I can't figure out why this new version is different from before is my primary motivation :)
I'm thinking...
FCP at 1000 ms
|
===== Task >50ms ends before 1050 ms
====== Task >50ms ends at 1050 ms
======= Task >50ms starts before 1000 ms and ends after 1050 ms
There was a problem hiding this comment.
Added these cases.
| @@ -35,5 +34,6 @@ Object { | |||
| "observedTraceEndTs": 225426711887, | |||
| "speedIndex": 1657, | |||
| "speedIndexTs": undefined, | |||
| "totalBlockingTime": 726, | |||
There was a problem hiding this comment.
Ah right. That sgtm: added test case just for TTI clipping.
| {start: 1000, end: 1110, duration: 110}, // Contributes 10ms. | ||
| {start: 2000, end: 2200, duration: 200}, // Contributes 50ms. | ||
| // The clipping is done first, so the task becomes [150, 200] after clipping and contributes | ||
| // 0ms of blocking time. This is in contrast to first calculating the blocking region ([100, |
There was a problem hiding this comment.
Added these cases.
|
@brendankenny did you explicitly not want it to land for 5.2? |
It depends on how @paulirish wants to do the branch? Presumably it could branch from any commit... |
|
Yeah I got that part :) (via chat) I'm asking why you wouldn't want it to be included in 5.2 so we don't have to rush out a 5.3 before August 1? |
|
lol oh I see, yeah that makes sense. Let's land :) |
This PR cleans up Total Blocking Time code in lighthouse. It