Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(dom-size): add TBT savings #15307

Merged
merged 10 commits into from
Aug 15, 2023
Merged

core(dom-size): add TBT savings #15307

merged 10 commits into from
Aug 15, 2023

Conversation

adamraine
Copy link
Member

First usage of TBTImpactTasks. We still want this audit available in snapshot mode, so using some optional artifacts to get the savings calculation.

@adamraine adamraine requested a review from a team as a code owner July 26, 2023 00:12
@adamraine adamraine requested review from connorjclark and removed request for a team July 26, 2023 00:12
const tbtImpactTasks = await TBTImpactTasks.request(metricComputationData, context);
for (const task of tbtImpactTasks) {
if (task.group.id !== 'styleLayout') continue;
tbtImpact += task.selfTbtImpact;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would benefit from some comments.

is the idea that the size / depth / whatever of the dom contributes to TBT only as much as style layout tasks do?

This opp can only be zero if you have no doms to style. is that the intention?

Copy link
Member Author

@adamraine adamraine Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would benefit from some comments.

Will do.

is the idea that the size / depth / whatever of the dom contributes to TBT only as much as style layout tasks do

Pretty much. This calculation assumes TBT impact of style/layout stuff is correlated to the overall DOM size.

This opp can only be zero if you have no doms to style. is that the intention?

Every page spends a nonzero amount of main thread time on style/layout but not every page will block the main thread because of it. selfTbtImpact captures the blocking time not duration, so any layout/styles stuff that is not part of a long task will also have 0 impact here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imagine a page that is otherwise scoring a perfect 100, with a large dom. But now add a JS setTimeout that does constant layout thrashing (force style recalcs) - will this result in a dom-size audit showing 100% of TBT being attributed as "reduce your dom size to fix TBT" - when really the problem is in the style recalcs?

Copy link
Member Author

@adamraine adamraine Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this result in a dom-size audit showing 100% of TBT being attributed as "reduce your dom size to fix TBT" - when really the problem is in the style recalcs?

Yes, but I have some thoughts:

  • If the page has a large DOM and it forces style recalcs, then the large DOM is partially to blame for any blocking time IMO
  • We could add a note in this audit's docs like "if a large DOM is necessary, try to minimize forced style recalcs"
  • I'm not sure we can get a better estimate. Even if we had JS profiling data and knew which style/layout tasks were caused by manual style recalcs, my first point still forces us to consider them as impactful for this audit in some way.

@adamraine adamraine merged commit 7915c26 into main Aug 15, 2023
27 checks passed
@adamraine adamraine deleted the dom-size-savings branch August 15, 2023 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants