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

Allow CSS larger than 50k if 90% used #4555

Open
jpettitt opened this Issue Aug 17, 2016 · 46 comments

Comments

@jpettitt
Copy link
Collaborator

commented Aug 17, 2016

We'd like to see the CSS limit increased from the current 50K to 100K.

Background - we're automatically converting sites, including all their look and feel to AMP. Many sites have 3-400k of CSS which we prune down to only the rules that are used on the page. This Typically yields between 30 and 80k of CSS.

This means that on the larger files we need to very aggressively optimize the CSS size by rewriting extensively which is a) computationally expensive, b) tends to break things in unexpected ways and c) means we're dropping /*! comments containing rights info which puts us out of license compliance. Increasing the limit to 100K would make all those issues go away for many publishers who are converting existing templates.

Here is an example page that's right up against the current limit https://cdn.relaymedia.com/amp/www.niemanlab.org/2016/08/that-friends-and-family-facebook-algorithm-change-doesnt-seem-to-be-hurting-traffic-to-news-sites/

@erwinmombay

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Aug 17, 2016

@Gregable Do we have some quick stats of CSS size distribution in AMP pages overall?

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 17, 2016

@dvoytenko and @Gregable it's not just the current size, as we move beyond using AMP for phones and start using it for responsive pages like the one I linked above the CSS size jumps markedly.

@dknecht

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2016

Maybe we can have sections that aren't delivered to mobile. The cache can remove and vary on mobile vs desktop?

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 17, 2016

@dknecht The issue show up when you're converting existing content - for new designs we can hand build CSS and get it under the limit. However that doesn't scale to 10's of thousands of sites. As automatic amp converters like ours get better (see link above) we're starting to bring in the whole page rather than just fragments of it.

Apart from the initial cost of processing the CSS it adds very little to the page cost weight overall (particularly since AMP doesn't load any graphics in areas hidden by responsive layout). The page I linked above is 22.2k on the wire despite having 46k of CSS and an overall size of 102k uncompressed.

@Gregable

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

@dvoytenko, I don't have anything at hand. I suspect it wouldn't be all that interesting though since folks have been optimizing against the current rules.

@jpettitt, at a quick glance, that css example looks pretty bloated. I count 8 different rules targeting .simple-rightsidebar which is on exactly 1 tag that has 6 other classes on it.

@pdufour

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2016

I'm a fan of keeping it at 50k personally. The fact the spec is causing you to rewrite your CSS is a good sign, it's making you keep it lightweight.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 17, 2016

@Gregable sadly that's the nature of converting existing templates. If you look closer a lot of the rules you cite are inside media queries. AMP forces bloat when converting existing templates - All the rules that start ._RM were created to replace style attributes on elements and all the rules that start #_RM were created to replace !important tags in the original CSS. This particular example bloats a lot becasue they were liberal with the !important and style attribute.

I'll send you a link on slack to an example that's over the limit (not a live customer so I can't post it here)

@pdufour rewriting CSS is fine if you have the resources but it doesn't scale to 10's of thousands of sites. If we stick at 50K makes automated conversion withe full look and feel far harder. As is we can optimize heavily - the example linked had 183584 bytes of css reduced to 47612 with 2225 rules cut down to 709.

I'm trying to avoid having to rip out all the media queries and treat the page as phone only. That would work for now, however if we want AMP to be usable as more general fast web page framework we need to allow enough space for a responsive design.

@jmadler

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2016

My understanding of the 50k selection as a CSS limit was that it was believed to be enough to fit nearly all cases, but not enough to just copy/paste the existing CSS implementation over, for the reasons that @pdufour described.

Lifting it could remove that performance improvement (negligible though it may be), but it could be replaced by other solutions. Maybe a cache optimization that removes any unused CSS rules based on selectors that don't apply, or a cache optimization that intelligently compresses CSS rules/selectors.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2016

@jmadler we're already removing unused CSS and, if it still doesn't fit, renaming all the classes and ID's to short names. We still see CSS over 50k (keep in mind with some sites we're starting with as much as 800k of CSS). We're also stripping data url's. We've yet to find one we can't squeeze under 100k. AMP itself creates some of the issues by banning '*', !important and inline styles.

I think this goes to a bigger question of is AMP a mobile only and article only standard, in which case we could pre-render all the media queries and strip out anything wider than a phone. Or is it (or will it become) a more generalized acceleration framework. If the latter, particularity if we're going to do highly structured responsive pages like product pages, home pages etc then 50k become a real obstacle.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 25, 2016

So where are we with this?

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Aug 25, 2016

Let me pass it on to @cramforce. He is back next week and can provide a definitive answer.

@cramforce

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

It would be good to get a good sample of pages that run up to the limit. I expected this to be controversial, but haven't seen it come up anywhere else, yet.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 29, 2016

@cramforce I'll send you a customer example over slack (not a live site yet so I can't post it here).

@ericlindley-g ericlindley-g added this to the Pending Triage milestone Aug 31, 2016

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 7, 2016

ping.

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

Here is my recommendation:

  • leave limit unchanged for now
  • not count whitespace
  • no longer count non-data URIs in limit
@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 8, 2016

Not perfect but better than nothing. If we could do that and bump to 75K that would be perfect.

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

I've not heard any other parties to ask for an increase in size. This seems to come up when transcoding pages to AMP, but AMP is not designed to be a transcoding target for non-AMP pages.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 8, 2016

I get that's it's not designed to be a transcoding target - however in the real world automatically translated pages mostly look like sh*t (eg wordpress plugin) and the vast majority of sites don't have developers on staff. Those that do have developers have a todo list 50 items long and only the top one or two items on the list will ever happen. If AMP is to achieve parity in UX with existing sites and spread beyond the minority of publishers who have both the technical staff and the free resources to do decent amp conversion auto transcoding is pretty much essential.

Publishers we talk to complain that AMP doesn't monetize well, this will kill AMP. Much of the reason it's not monetizing is they have abysmal AMP conversions that don't support the full ad map, lack navigation, and lack recirc elements.

I think this comes down to letting the perfect be the enemy of the good. Yes in a perfect world we'd have all AMP pages designed from scratch, avoiding all the bad practices. This isn't a perfect world and there is an installed base running to billions of pages on millions of sites. Allowing for a simple path from there to here will speed AMP adoption, improve monetization and therefor help the AMP ecosystem.

I'm having a hard time seeing why, what is basically a number pulled out of thin air, is so important that it's worth making people spend, cumulatively across all sites, million of $ on rewrites.

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

@jpettitt Does your current output only include selectors that actually match on the pages they apply to?

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 9, 2016

Yes we compare every CSS rule to the actual page content and drop all those that don't apply. If that doesn't get it under the limit we rename all the classes and id's. Finally if that doesn't do it we start pre-computing media queries and stripping out content for wider pages, this last step is what we'd like to avoid.

I'm most cases we're cutting the original CSS by ~80%. Sometimes as much at 90%.

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

I sympathize, but we cannot go higher without messing up our performance model.

Maybe we need easily statically separable CSS sections per device type.

@dknecht

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

@cramforce Much earlier in thread i proposed "Maybe we can have sections that aren't delivered to mobile. The cache can remove and vary on mobile vs desktop?"

Instead of having publisher setup vary correctly we can just have cache hide sections not needed for the requesting device

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

Yep, unfortunately such mechanisms rely on the cache to be fast.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 9, 2016

Does it really mess up the model? On the wire we see ~75% compression with gzipped AMP pages and so an extra 50k of CSS is around 12.5k (worst case, probably less) on the wire with AMP pages having an overall weight with the JS, ads, images etc of 1 to 2 MB it's in the noise. As is we're actually slowing the page down as we squish the CSS by using URL shortening on the css url() values and moving data urls, particularly small icon font fragments, to external resources. Not counting non-data urls will help some.

@Gregable

This comment has been minimized.

Copy link
Member

commented Sep 9, 2016

@cramforce , I would push back on your suggestion of choosing bytes that don't count towards the limit. I think the simplicity of a byte limit makes it easier for folks to reason about and develop against. If there is a good reason for additional complexity in the rule, then we should go forward, but this doesn't feel like it falls into that category.

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

OK, I thought about this some more and I think I have a solution that I'd
like and that should make almost everyone happy:

  • leave the existing limit in place unchanged.
  • add an alternative rule (it is enough if one is met) that says: at least
    90% (actual number TBD) of CSS selectors match content in the document. On
    top of that limit total byte count of data URIs.

Of course, this would be a pretty big change on the validator side, so we
need to see how fast this can be implemented, but I think this is a good
mechanism.

  • it captures our primary intent: CSS hygiene.
  • it is easy to automatically enforce by generators (that can avoid any non
    matching selectors).
@jaygray0919

This comment has been minimized.

Copy link

commented Sep 10, 2016

What tool can we use to automatically determine that "90% of CSS selectors match content in the document."

We've search for such a tool but have come up empty.

There are some candidates but they do not handle @media.

/jg

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

@media is irrelevant here. It isn't required that things actually match at
a specific screen size, just that they at least potentially match.

I think mod_pagespeed does such pruning, but it may make sense to create a
few special purpose node, PHP, etc modules.

On Sep 10, 2016 12:06 PM, "jay gray" notifications@github.com wrote:

What tool can we use to automatically determine that "90% of CSS selectors
match content in the document."

We've search for such a tool but have come up empty.

There are some candidates but they do not handle @media.

/jg


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4555 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAFeT8prXLRma3M2rA0ve58cOWRc4O9gks5qov_TgaJpZM4Jl_s5
.

@Gregable

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

@cramforce , perhaps it's my anti-abuse background, but I can see someone doing something like the following:

<style amp-custom>
  .staticBlobOfCssRules {
    display: none;
  }
  ... Another MB of CSS ...
</style>

...

<div class=staticBlobOfCssRules>
  <!-- Set of elements to make sure that every CSS rule in our site-wide rules matches something -->
  <div class="divclassa divclassb divclassc ..."></div>
  <span class="spanclassa spanclassb spanclassc ..."></div>
</div>

If you develop your site-wide CSS at the same time as this static blob of useless divs, you get to make your CSS as complex as you want and always get around the AMP rules.

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 12, 2016

@Gregable we're not trying to make this protected against that type of abuse, just make the path of least resistance lead to good results. People can make large docs in the HTML part anyway, so protecting against that doesn't really help.

In particular, the capability you need to generate the contents of staticBlobOfCssRules are roughly equivalent with what you need to figure out which rules to delete.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 16, 2016

Doing the "selectors match content doc" is quite expensive in terms of CPU (we do this). There are a bunch of optimizations but even serious pre-processing of source CSS file and cache ed results it's ~35% of our page generation CPU time. I like the idea but it's going to be expensive to implement, particularly on a page by page basis.

@julien51

This comment has been minimized.

Copy link

commented Sep 27, 2016

I am not sure lifting the 50k limit to 100k would make things much better.
I understand that the goal of using <style amp-custom> is to reduce latency but at the same time it means that there is no efficient way to cache the styles sheets across multiple documents. If you load 10 articles from the same publishing entity, you load 10 times 50k worth of CSS when you could really have loaded that just once and cached it aggressively.

So rather than changing the 50k limit I would really be in favor of a mechanism to actually remove the CSS from inside the documents. If we want to avoid delaying the CSS request, maybe we could use an HTTP link header to point to it so that the browsers can't start to fetch it before they actually have received and loaded the whole DOM?

@cramforce

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

Other models are certainly possible. E.g. a 30KB site wide CSS file + allowing 20KB inline or some variation thereof.

The real goal is to motivate CSS hygiene. So far, AMP is achieving this goal. No site can make their CSS "append only", which is in stark contrast to most CSS on the internet that rarely gets a rule removed. Want to render that "Christmas Special 2013"? Cool, CSS is still there :)

So, I'm super happy to explore all kinds of proposals that achieve the same goal.

AMP caches are, of course, free to un-inline the CSS, give it a content-addressed URL and then HTTP2 push it to clients instead of doing inlining.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 30, 2016

The hygiene part I get, we have sites we convert that have 5000+ css rules where only 400 are in use on an AMP page. We've seen 800k+ of CSS on a mobile page.

The original motivation for this request was being able to keep the 500 rules that are in use on a complex page so we don't have to get humans involved in rewriting or start cutting functionality. Right now every site we work with fits under 50k thanks to some highly aggressive, and CPU expensive, optimization (the biggest comes in at 48k). AMP itself actually bloats the CSS by not allowing !important or element level styles. We end up applying workarounds in our compression that introduce more rules.

Regarding the on the wire performance cost of inline CSS vs external style sheets. Even with multiplexing in SPDY or HTTP/2 there is at least one extra round trip to request and fetch a style sheet. Assuming 4G (20ms latency, 4Mbit downstream, cumulatively ~60ms TTFB) you could transmit ~30KB compressed equal to 250K uncompressed of inline CSS in the TTFB of your external CSS file.

[CSS compresses really well - 48,693 bytes of CSS from our worst case site gzips to 10,188 bytes]

  1. I don't think going to an external style sheet will actually be a win for most users since the vast majority of AMP hits are one off's.
  2. Moving from 50K of CSS to 100K of CSS is going to add 10 to 15KB on the wire if it's all used (20 to 30ms on 4G).
  3. The don't count the white space idea mentioned up thread only amounts to ~4% on the 48k test file I'm using.

My take, increasing the CSS limit a little, say 75K will, with proper optimization, allow almost any site to be converted to AMP. It will do that at minimal performance cost while still forcing the removal of dead rules which seems to have been the original goal.

@Gregable

This comment has been minimized.

Copy link
Member

commented Jun 6, 2017

I think we are moving forward with #9625

Does this address the primary concerns?

@Gregable Gregable changed the title Increase CSS limit to 100k Allow CSS larger than 50k if 90% used Jun 6, 2017

@lucanovera

This comment has been minimized.

Copy link

commented Jul 3, 2017

Hello, i'm a developing a site that is 100% AMP (much like your website), this includes home, product pages, responsive and integration with a CMS.

I also find the 50K limit to be a problem, and this is no site conversion, it's just a big site developed specifically for AMP. Currently we're selectivly adding css based on the current templates and specific modules within templates.

If you plan on supporting AMP as a framework for all devices i think the limit is not going to work for all sites since you can have larger pages and responsive css.

The solution proposed by @cramforce about checking rule usage seems the best one to me, since it allows the creation of larger pages for desktop. Raising the limit to 100K could work just as a temporary solution.

When you select the % of rules that need to be in used keep in mind the use of CMS and developer friendlyness. For example: in the site i'm developing i see that some 'col-md-x' classes are not being used and also some styles that apply to the articles content (like p, b, bloquote, figure, etc). It would not be very practical to remove those.

@jpettitt

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 8, 2017

@Gregable if we could also add <style amp-data> that only contains rules with url(data ...) values I think we'd be there. I'd even be happy if that had a separate 50k size limit to stop people putting huge images in it.

@Gregable

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

@dvoytenko re <style amp-data>

@dvoytenko

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2017

I think <style amp-data> sounds interesting. In theory, from performance point of view, these are good, since download/parse of these resources will not block rendering. That needs to be tested though, b/c it's hard to predict what browser would do in this case. But it sounds promising.

This could work, as long as we can make clear rules about it. What would they be? Only rules with properties that have values of url()? Or a few specific properties such as background-image, etc?

@ampprojectbot

This comment has been minimized.

Copy link
Collaborator

commented Jan 1, 2018

This issue seems to be in Pending Triage for awhile. @Gregable Please triage this to an appropriate milestone.

@maszynka

This comment has been minimized.

Copy link

commented Mar 30, 2018

I do remember when whole sites had 100kb limit for being called optimal. I would say it's even easier when you have limitations to make good UX, you have to reuse your code, make it repeatable etc. AliExpress fit's in 50kb limit and all other AMP e-commerce do.

It's not a problem about limit, but a problem about creating code that programmers/designers do not control anymore or make to complex. If you feel 50kb it's not enough think about it like 'hygiene' and way to bring back a little bit more focus on code quality ;).

@claudchan

This comment has been minimized.

Copy link

commented Jun 7, 2018

AliExpress, looks like they use AMP only for front page/homepage and profiler not the whole e-commerce site. When navigate a product or any inner page is not AMP page anymore.

@stephengardner

This comment has been minimized.

Copy link

commented Jul 13, 2018

I developed an app that converts Shopify stores/products into AMP e-commerce pages. (apps.shopify.com/amp-google)

While enhancing our parser, I have been working on parsing a shop's theme CSS and applying it to their new AMP page. The 50k limit, however, is a roadblock.

There are very popular page-builders which attribute an id to every generated element, and then apply a css rule to that id. For example: "#s-1254-3241-21312-321" could be an id of an element. As you can imagine, the CSS is quite large.

This 50k limit is stopping me from being able to offer Shopify stores with optimal AMPing of their pages. Instead, since I don't want to confuse users, it's best the app just not support some super-popular Shopify page-builders.

I'd be massively in-favor of a "90%" used limit, and/or raising the limit overall. This would be a pretty large benefit to users who want beautifully AMPed ecommerce pages from within an extremely popular ecommerce platform.

I'm following this topic closely.

@kevinhq

This comment has been minimized.

Copy link

commented Nov 21, 2018

In case anyone still struggling with 50k CSS size, whether you like it or not, you need to follow the rule ( at least for now ), here I wrote a use case how to reduce CSS to be under 50k for AMP

@nainar

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

@leonalicious to add AliExpress' feedback here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.