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

I2I: Relax CSS Limit from 50000 bytes to 75000 bytes #26466

Closed
kristoferbaxter opened this issue Jan 24, 2020 · 24 comments
Closed

I2I: Relax CSS Limit from 50000 bytes to 75000 bytes #26466

kristoferbaxter opened this issue Jan 24, 2020 · 24 comments

Comments

@kristoferbaxter
Copy link
Contributor

@kristoferbaxter kristoferbaxter commented Jan 24, 2020

Summary

Increase the allowed bytes for a valid AMP document from 50000 to 75000.

Design document

The change itself is quite straightforward, change max_bytes from 50000 to 75000 here.

css_length_spec: {
  html_format: AMP
  max_bytes: 50000
  max_bytes_per_inline_style: 1000
  spec_url: "https://amp.dev/documentation/guides-and-tutorials/learn/spec/amphtml#maximum-size"
}

Motivation

Many web developers have encountered the existing limit of 50000 with even quite advanced CSS minification techniques.

AMP's goal with a limit is to encourage hygiene, not to reduce capabilities. Increasing the limit slightly will allow a larger set of documents to be considered valid AMP.

In tandem, we will explore a general CSS minification technique for all AMP documents to help achieve the hygiene goal (remove unused CSS) without sacrificing readability.

Alternatives Considered

Keep existing limit while creating a more general CSS minifier that can ensure a larger number of documents can meet the existing limit.

While this meets the goal of focusing on user-experience first, the risk is a smaller number of valid documents while teams and developers wait for this solution.

Implement a limit based on document complexity

Theoretically this would be a best of both worlds solution, allowing documents that need additional CSS size to use the space. However, the complexity of the solution requires additional consideration. Moving to 75000 bytes allows for this exploration to continue without causing document developers to wait for a potentially complex solution that is difficult to communicate and potentially easy to game.

Additional context

Work is beginning on a more general CSS minifier as part of the wg-outreach group. If you'd like to participate, please join in!

Launch tracker

https://docs.google.com/spreadsheets/d/1Aw7YeIHvPbuR8WFgciPodBMXdsmI3FZLsu8-iX0sPGk

/cc @ampproject/wg-approvers, @ampproject/wg-outreach, @ampproject/wg-performance, @ampproject/wg-caching

@kristoferbaxter kristoferbaxter changed the title I2I: Relax CSS Limit from 50kb to 75kb I2I: Relax CSS Limit from 50000 bytes to 75000 bytes Jan 24, 2020
@nainar

This comment has been minimized.

Copy link
Contributor

@nainar nainar commented Jan 24, 2020

This addresses #4555

@morsssss

This comment has been minimized.

Copy link
Contributor

@morsssss morsssss commented Jan 24, 2020

Woo hoo!

😀😀😀😀😀😀😀

@sebastianbenz

This comment has been minimized.

Copy link
Contributor

@sebastianbenz sebastianbenz commented Jan 24, 2020

@cramforce

This comment has been minimized.

Copy link
Member

@cramforce cramforce commented Jan 24, 2020

I had one follow-up question: Could we do a quick evaluation as to whether switching to a selector-count limit may be a better strategy?

@kristoferbaxter

This comment has been minimized.

Copy link
Contributor Author

@kristoferbaxter kristoferbaxter commented Jan 24, 2020

Chatting with @westonruter offline to see if we can capture this from Wordpress examples.

@westonruter

This comment has been minimized.

Copy link
Member

@westonruter westonruter commented Jan 24, 2020

From analyzing all ~4,000 WordPress.org themes:

  • Average initial selector count: 3,640
  • Average selector count after minification: 2,733
  • Average reduction: -30%

For the CSS byte size:

  • Average initial size: 244KB
  • Average minified size: 64KB
  • Average reduction: -70%

This data obtained from rendering a single post in a WordPress theme that is maximally-populated with all the content from the WordPress theme unit test data, as well as fully-populating all widget sidebars and nav menus. The result an attempt to obtain the worst case for each theme, as the tree-shaker effectiveness is reduced when more selectors are used on the page. The CSS minification here includes:

  1. Removal of whitespace and comments.
  2. Removal of selectors which don't apply to the document (tree shaking).
  3. Removal of declaration blocks which have no remaining selectors present.
  4. Removal of any resulting empty at-rules.

For the full data, see the public Google Sheet. It was generated using amp-wp-theme-compat-analysis. (Beware: The dataset it generated is a 835MB gzipped tarball and it takes a few days to complete on a MBP.)

@kristoferbaxter

This comment has been minimized.

Copy link
Contributor Author

@kristoferbaxter kristoferbaxter commented Jan 24, 2020

Spoke offline with @cramforce and @Gregable about "a quick evaluation as to whether switching to a selector-count limit may be a better strategy."

We did a quick analysis and didn't find a compelling correlation worth pursuing.

As a result, we agreed that a straight increase to 75KB makes the most sense right now.

@morsssss

This comment has been minimized.

Copy link
Contributor

@morsssss morsssss commented Jan 24, 2020

@westonruter , that data is super interesting to me - particularly the bit about excluded selectors.

I assume those selectors are what was excluded by dead-CSS elimination? If so, the savings is far less than what's implied by Chrome's Code Coverage tool.

@westonruter

This comment has been minimized.

Copy link
Member

@westonruter westonruter commented Jan 24, 2020

@morsssss yes, the AMP plugin determines whether a given selector will match the current document, and if not, it is removed from the amp-custom stylesheet. We actually just merged the ability to inspect the result of tree shaking in a view similar to the code coverage in DevTools: ampproject/amp-wp#4026 (comment)

@morsssss

This comment has been minimized.

Copy link
Contributor

@morsssss morsssss commented Jan 24, 2020

Code Coverage often tells me a given site's CSS is 85% unused, 90% unused, or even higher. Your quite advanced dead-code eliminator doesn't get rid of anything near that percentage of selectors. This leads me to believe that the gains we'd get from creating a publicly available, easy to use, highly reliable dead-code eliminator are significant, but not as great as we'd hoped. And that's assuming we could do that.

@westonruter

This comment has been minimized.

Copy link
Member

@westonruter westonruter commented Jan 24, 2020

Right, it doesn't remove that percentage of selectors. But it does remove on average ~70% of the overall CSS. When all selectors for a declaration block are removed, then the declaration block is also removed. I suppose this is why the overall code reduction is 70% even though only 30% of the selectors were removed (as removing the last selector from a declaration block causes a lot more to be removed).

@jpettitt

This comment has been minimized.

Copy link
Contributor

@jpettitt jpettitt commented Jan 25, 2020

The other thing to look at is class renaming. When I was building the CSS compressor for Relay Media renaming classes was a huge win. It's tricky to get right because you have to make exceptions for things that might get manipulated by extensions but both renaming and merging identical css blocks can be a big win. [Class renaming is a bit easier if you just add the new short classes to the html and leave the old long ones so extensions can still use them]

@Gregable

This comment has been minimized.

Copy link
Member

@Gregable Gregable commented Jan 25, 2020

It's worth noting that class renaming doesn't help much with the actual problem that the byte limit tries to fix because of compression. It helps you meet the AMP metric, but the user experience is only minimally improved.

@westonruter

This comment has been minimized.

Copy link
Member

@westonruter westonruter commented Jan 25, 2020

Some drawbacks I see to CSS class renaming:

  1. Added difficulty for devs to debug CSS issues. Overall this makes CSS more opaque and is a detriment to view source and copy/paste.
  2. Increased opportunities for CSS selectors to fail to apply to the document for dynamic content. I'm thinking of amp-live-list where the elements in updates would need to make sure they had the class names renamed the same as on the original page, but this can be tricky to make consistent. This is even more of a concern perhaps now with styles targeting amp-script.
  3. Additional runtime processing costs (in the WordPress context at least). Since WordPress generates pages dynamically, the class name reduction would need to be done at serve time, depending on the approach taken.

For the WordPress plugin, here's the issue to track this: ampproject/amp-wp#1518.

@stephengardner

This comment has been minimized.

Copy link

@stephengardner stephengardner commented Jan 25, 2020

This is a big help for more complex themes that we're developing/have developed with AMP for Shopify.

To add to @westonruter's point, we have considered class renaming, but this does present a hurdle if a user wants to do something like copy/paste their existing theme CSS into the AMP theme CSS, or debug the AMP theme CSS in a way that seems parallel to their existing architecture. As we integrate with more and more third-party apps, a 1-1 CSS mapping helps keeps things sane for all parties.

Following this closely/looking forward to release!

@asadkn

This comment has been minimized.

Copy link

@asadkn asadkn commented Jan 25, 2020

It's quite odd to continue using an arbitrary bytes size limit. One of the most basic thing almost everyone does lately is to have gzip compression active, if not brotli.

Is there a reason why the compressed size can't be considered? 400KB of CSS can be 40KB gzipped.

@cramforce

This comment has been minimized.

Copy link
Member

@cramforce cramforce commented Jan 26, 2020

We care about a proxy for browser render time, not so much time on the wire. In any case compressed size is relatively proportional to non-compressed size, so it really doesn't matter so much which you pick.

@asadkn

This comment has been minimized.

Copy link

@asadkn asadkn commented Jan 26, 2020

Thanks for the comment. I am aware of the reason for the metric but the way it is now, using good development practices like BEM is punished, unnecessarily.

Even on something that doesn't use BEM, for example T20 WordPress theme, the style.css goes down from 123KB to 98KB by minifying the class names. However, with BEM usage, I have seen results from 80KB to 45KB by just minifying class names to generated 3 characters.

Ignoring the verbosity of CSS can be done either by selector-counting (ideally) or considering compressed size (say, since repetition is negated by gzip in all those BEM selectors).

@Gregable

This comment has been minimized.

Copy link
Member

@Gregable Gregable commented Jan 28, 2020

@asadkn We considered compressed size very early in the validator spec:

  • As Malte points out, the compressed and non-compressed size are nearly proportional.
  • Compression is less predictable for developers. For example, it is possible that adding new CSS could reduce compression size. It's simpler to work with an uncompressed length limit.
  • The actual amount that CSS is compressed depends on other parts of the document, which contributes more to how unpredictable it is.
  • There is no native gzip / brotli api in JavaScript. The AMP Validator would need to ship with one and it would likely be pretty slow.

For curiosity sake, if you want to consider whether gzip would help your case or not: Looking at some typical AMP documents, CSS-only gzip compresses to about 20% of the original. So, the 50,000 and 75,000 byte uncompressed limits would map to something around 10,000 and 15,000 compressed bytes respectively.

@Gregable

This comment has been minimized.

Copy link
Member

@Gregable Gregable commented Jan 28, 2020

user wants to do something like copy/paste their existing theme CSS into the AMP theme CSS

My understanding is that the primary reason AMP implements CSS limits is to encourage CSS hygiene rather than just copy/pasting lots of potentially unused CSS into a document.

@stephengardner

This comment has been minimized.

Copy link

@stephengardner stephengardner commented Jan 28, 2020

user wants to do something like copy/paste their existing theme CSS into the AMP theme CSS

My understanding is that the primary reason AMP implements CSS limits is to encourage CSS hygiene rather than just copy/pasting lots of potentially unused CSS into a document.

Your understanding seems correct. I wasn't inferring lots of unused CSS. This CSS can either be optimized beforehand, or optimized by the afformentioned tree-shaking after the user copy/pastes it. All CSS on the pages we generate is tree-shaken before rendering, regardless of how much it started with. If you clobber the CSS , then the user has no mapping during debug time. If they want to copy/paste 3,000 characters of CSS, for a footer section, for example, we don't find that too egregious (especially if they started with something like only 20k total), yet they might have a hard time doing such a task, depending on how the class-minification works. Perhaps minifying class names is possible, and then keeping them un-minified during the page-builder/editor phase, but I'm just expressing some concerns with thinking that this is a proper blanket-solution. I think this thread and the linked threads have done a good job of trying to be practical rather than clever.

@jaygray0919

This comment has been minimized.

Copy link

@jaygray0919 jaygray0919 commented Feb 4, 2020

Without knowing about cache issues or being able to compress CSS (class renaming), we strongly support a 90% rule. We routinely use "unCSS" tools to purge unused CSS with the goal of 0% unused CSS. We have some pages with more than 75000 bytes of CSS (shared with @jpettitt who proposed reasonable class renaming techniques). IOHO the 50000 byte limitation is unreasonable. A 90% rule would allow us to deploy CSS-heavy (JS-free) pages that currently fail the validator.

@kristoferbaxter

This comment has been minimized.

Copy link
Contributor Author

@kristoferbaxter kristoferbaxter commented Feb 11, 2020

We've merged the PR that addresses this I2I.

@honeybadgerdontcare

This comment has been minimized.

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare commented Feb 18, 2020

The increase is now live in AMP Validator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.