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: split up CSSUsage artifact #15952

Merged
merged 4 commits into from
Apr 17, 2024
Merged

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Apr 16, 2024

Closes #15890

The general problem is that CSS usage tracking has a large performance overhead. Prior to #15865 we were side-stepping the issue by only looking at CSS usage at the end of the navigation. One of the goals if this PR is to revert back to this behavior.

However, it does not make sense to examine CSS usage at a single point in time in timespan mode. So to avoid the performance overhead problem in timespan mode, this PR disables CSS usage tracking in timespan mode. This is achieved by splitting up CSSUsage into two gatherers.

  • CSSUsage only contains CSS usage information (no stylesheet events or contents) and is not available in timespan mode
  • Stylesheets only contains stylesheet events and contents (but no usage information) and is available in all modes

This enables certain audits (e.g. unminified-css) to examine the available stylesheets in timespan mode without needing to run through the expensive rule tracking instrumentation.

User visible changes:

  • unused-css-rules is no longer available in timespan mode. IMO this is a good change because unused CSS shouldn't be a huge issue outside of initial page load anyway.
  • CSS coverage will only include rules that were in use at the end of the navigation, and could exclude rules that were temporarily in use during the navigation. IMO this is suboptimal but necessary because accurate performance metrics are more important than accurate information in unused-css-rules.

@adamraine adamraine marked this pull request as ready for review April 16, 2024 23:12
@adamraine adamraine requested a review from a team as a code owner April 16, 2024 23:12
@adamraine adamraine requested review from connorjclark and removed request for a team April 16, 2024 23:12
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

This is a good refactor, though I'm not sold on the premise that unused-css-rules is not a useful audit in timespan mode. But I think using CDT for purposes of isolating your CSS to what is needed for initial load + common initial interactions is the more reasonable approach for people needing this information.

@adamraine adamraine changed the title core: split up CSSUsage gatherer core: split up CSSUsage artifact Apr 17, 2024
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

yah really like the split of gatherers.

and.. while i'm not jazzed that cssusage (in navigation mode) is underreporting usage because of the timing of our invocation... i'm fine with it, given the confounding factors.

lgtm

@adamraine adamraine merged commit 484c6f2 into main Apr 17, 2024
27 checks passed
@adamraine adamraine deleted the split-css-usage-and-stylesheets branch April 17, 2024 18:46
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.

Excessive style recalcs caused by CSS usage tracking
4 participants