core(mainthread-work-breakdown): add TBT savings #15201
Conversation
| const tbtResult = await TotalBlockingTime.request(metricComputationData, context); | ||
| tbtSavings = tbtResult.timing; | ||
| } catch (err) { | ||
| if (!/Could not find any top level events/.test(err)) throw err; |
There was a problem hiding this comment.
Added this case to pass the "no events are present" case for this audit. In practices we shouldn't ever hit this error, and if we do then there are probably bigger problems than this audit. I'm open to removing that test case if it cleans up the code here a bit.
There was a problem hiding this comment.
Would it work to make this computed artifact return null in this case, and having callers that would find that exceptional throw themselves?
There was a problem hiding this comment.
I think that's a lot of null handling for an error that generally shouldn't happen. Plus we would have to generalize it for all computed metrics not just TBT since the result type is shared.
TBH I think it would be more reasonable to remove this logic and surface the error in the audit if the main thread doesn't have any tasks. The only reason this exception is made is for one of our test cases that asserts this audit returns no data if there are no tasks.
| @@ -91,47 +119,57 @@ Object { | |||
| expect(Math.round(output.numericValue)).toMatchInlineSnapshot(`979`); | |||
| assert.equal(output.details.items.length, 7); | |||
| assert.equal(output.score, 1); | |||
| }); | |||
|
|
|||
| it('should compute the correct values for the load trace (legacy)', async () => { | |||
There was a problem hiding this comment.
@connorjclark is there a reason we need to keep this around since it wasn't removed in #15005?
There was a problem hiding this comment.
Yes, as long as we still have code handling TracingStartedInPage for back compat.
There was a problem hiding this comment.
Ok. I reintroduced this test case, however the old trace we have stored causes a NO_DCL error when we try and compute the TBT. For this reason, I've changed the error handling logic to bypass all errors when computing TBT and report to sentry.
| } | ||
| `); | ||
| expect(Math.round(output.numericValue)).toMatchInlineSnapshot(`224`); | ||
| expect(Math.round(output.numericValue)).toMatchInlineSnapshot(`775`); |
There was a problem hiding this comment.
Before this patch, throttlingMethod had a value of undefined so we didn't apply the 4x CPU multiplier to the timings.
In this patch we need to specify the same throttlingMethod that was used to collect the trace, otherwise we can't compute TBT. In this case it was simulate
…thouse into main-thread-bd-savings
No description provided.