To sum up issue #81835, my previous patch that split up drupal.css resulted in a number of smaller CSS files, which dramatically increases the HTTP requests per *INITIAL* page load--slowly down the user experience up to 70%.

On recent work on a VERY MAJOR website, we confirmed that each HTTP request can be vital--these separate files are killing us from both sides of the story.

This patch resolves this issue.

How does it work?

Well at *INITIAL* page load, if the files directory exists and is writeable (e.g., the user has gone to the admin/settings/files page), it will create a single, aggregate CSS, and write this to files/css/md5_name.css This file is then loaded once for each request, reducing HTTP requests by 95%+ for HTTP requests. Since the file is only dynamically created *once*, and not on the fly, the file is cached by user's browsers as well.

Additionally, because we're doing this, we can introduce some basic compression, removing whitespace in the CSS. I've also introduced a new hook, hook_compress_css(), for modules or sites that wish to integrate various CSS compression libraries to really compress things.

Additionally, I've set a new variable, "cache_css" that any site $conf or devel module or other module could make use of as option to turn off CSS caching and it will fallback to the old system.

And of course, if the user can't create a files directory or has write permissions, it falls back to the old system.

And there is a drupal_css_cache_flush() to flush this cache, which is done automatically anytime you visit the themes page. Devel module could make use of this as well :-)

That should cover all bases.

But now what needs to be done--thorough testing, does it work for all themes? I've tested it very thoroughly and so far, so good.

Can it be optimized even more? Maybe, let's see.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m3avrck’s picture

I should note -- we have the same problem with Javascript and should implement the same solution with it, especially as jQuery plugins become rampant...

m3avrck’s picture

FileSize
4.63 KB

Here's an updated patch, after a review from chx on IRC. Also fixed another URL bug I noticed as well.

m3avrck’s picture

FileSize
4.66 KB

isset() is faster than is_null()

hunmonk’s picture

Status: Needs review » Needs work

this cache should also be flushed after any visit to update.php. if the css filenames don't change, but the contents do, will the md5 hash be different? i am unable to tell from a cursory look at the patch.

moshe weitzman’s picture

what about dynamically generated stylesheets? not sure we have accounted for those. they don't need to participate in the static file generation but they do need to participate the hierarchy stuff in drupal_add_css. seems like we need a boolean flag for that function for $cache. should default to true.

not sure how to handle hunmonk's case where module code has an update. seems like update.php is the best we can do.

jacauc’s picture

commenting to subscribe, sorry/thanks

FiReaNGeL’s picture

Sounds good, but I have a question. If I modify my CSS files directly and reupload them, I will HAVE to visit the theme page to have the master CSS rebuilt? This could be annoying. What about rebuilding it each time an admin visit a page? Would it be too much? The file would keep the same name / md5 and would still be cached for users if it didnt change, and if im doing theming work, i wont have to reload the theme page for each change.

Of course, I could just turn the system off to do theming, and turn it on when the site goes in production. This should be made very clear to users, to avoid the flood of 'I changed my CSS but the page is the same!'.

Great patch, much appreciated. As you said, the same could be made for javascript files.

chx’s picture

Moshe, dynamic was ruled out in the linked older issue.

Dries’s picture

I can't help but think this is a hack-y solution. It's going to be a pain to edit CSS files -- while the original patch meant to make it easier.

m3avrck’s picture

FileSize
6.74 KB

Here's an updated patch, changes:

  • Clear the CSS cache when visiting update.php. This is because we encourage users to visit this page after they updated their modules. If they update their modules and the CSS changes, we should flush the CSS cache here, thanks Chad!
  • Talked with Dries and introduced a new cache setting on the cache settings page: Cache CSS files. What this does is allow you to turn CSS caching and off. It's off by default and you can turn it on at anytime. When you turn it on and off the CSS cache is flushed so it's updated correctly.
  • In regards to "hack-y" this is not hacky. It's very important. Split CSS files make its very easy to alter CSS and with drupal_get_css() unset() different CSS files. VERY useful.

    The problem with that is then you have an extra dozen HTTP requests per page.

    So now after you're down fiddling with your site, you can turn this option on and all of those CSS files are merged AND compressed. The net result: we have sites that are performing even faster than 4.7 ones. I've used a similar technique on a site I just built that is getting 2-3 million hits a day, this technique helped greatly.

    Not hack-y at all -- this is a nice performance hook, not a hack.

m3avrck’s picture

In regards to Moshe's comment, that's not a bad idea. There could be an extra param for drupal_add_css() if you want to say whether or not your CSS file should be cached/merged to the others or spit out normally.

I'm not opposed to that but I'm not sure if that is overkill or not. Thoughts?

Wouldn't be too hard to add...

chx’s picture

Status: Needs work » Needs review
eaton’s picture

Status: Needs review » Needs work

So now after you're down fiddling with your site, you can turn this option on and all of those CSS files are merged AND compressed. The net result: we have sites that are performing even faster than 4.7 ones. I've used a similar technique on a site I just built that is getting 2-3 million hits a day, this technique helped greatly.

This is the kicker -- like caching, it can make things a pain if you turn it on while you're developing without realizing what's going on.

The option to specify that a given CSS file *shouldn't* be cached would be handy. I'm not sure it would be used a lot, but in the cases where it's needed, I can imagine it being a real pain without the option...

FiNeX’s picture

The idea to add an option for enable/disable CSS caching is very good!

m3avrck’s picture

Status: Needs work » Needs review
FileSize
7.54 KB

Here's and updated patch. Changes:

  • I agree with Moshe and Eaton--we need a setting to able to cache CSS files on a per file basis. New patch adds this feature in, when you call drupal_add_css() you can specify whether the CSS file should be cached or not
  • Various code optimizations and consolidations
FiReaNGeL’s picture

The optional per-file caching is good. The only nitpick I had (see above comment) is gone now that I know that the caching is disabled by default and that a clear message about potential issues is displayed ("It is recommended to only turn this option on when your site is in production, as leaving it on can interfere with theming development.").

Great work!

dvessel’s picture

subscribing.

moshe weitzman’s picture

Seems to me like we should default this to on. my .02

m3avrck’s picture

It's not defaulted to "on" because it 1) requires files/ be writeable and 2) most sites are going to be changing CSS a bit in the beginning after being setup. This is also consistent with the fact that "page caching" is disabled by default as well.

This makes sense, since enable either one can cause some oddities if you're not familiar with how it works.

dopry’s picture

I've tested the patch it seems to work as advertised.
I installed it and set up css caching.

I got my cached css file.. everything looked the same..
so the merging seems to work fine.

I converted init_theme to make the theme stylesheets non cacheable.
The theme style sheets started showing up in the html, and the styles they defined were no
longer in the cached css file.

so the cached css flushing seems to work.

It might be nice to not include the theme css in the css cache by default in theme.inc:init_theme.

jacauc’s picture

FileSize
129.29 KB

Installed the patch on a test site I have and everything seems to work fine.
Ran some tests of websiteopimization.com and thought i'd share them.
1) before I enabled CSS caching
2) directly after enabling.
3) ...and after a couple of pageloads and refreshes.

See the attachment

jacauc’s picture

Sorry if this is a bit ignorant, but I want to just figure out what's changing here:
Let's say I run the Gallery module, which has a CSS file called drupal_g2.css in the modules/gallery folder.
If I need to edit this particular CSS file, will I do it there, or create a copy in my theme folder and include it somehow?

...Or did nothing change with regard to that?

m3avrck’s picture

Just edit the file there and then you'll need to turn CSS caching off and on to grab the new version and cache it.

This patch doesn't affect the editing of CSS files--it's still the same. This patch automatically moves and consolidates things.

chx’s picture

Status: Needs review » Reviewed & tested by the community

The patch worked for several reviewers and it seems to do what we want.

dvessel’s picture

Status: Reviewed & tested by the community » Needs work

Hold on a moment. I love the new feature but 'core' is sorted after the 'module' group. Checked in my theme and inside Garland.

Dries’s picture

I'd love to have additional benchmark results to better understand the behavior/impact.

ak’s picture

Great feature, may be one thing to consider in real live situations for theme developers. A developer may have a stabil theme on the front end of his site for the normal visitors and at the same time, applied to a single admin user a theme in development for improvements. When the development theme is ready for production you can just switch themes for the end users.

When I understand this right by now you've got the possibility to switch caching on or off for the themes css, this is great but this is applied site wide. Correct me if I'm wrong.

It would make sense if this feature could be applied on individual theme basis so visitors get the benefits of the caching mechanism but a developer is still able to work on a development theme without caching at the same time as well. Hope this makes sense.

dvessel’s picture

Another issue, if a style has an image attached and is referenced within quotes, this patch breaks that image.

For example, the admin.css has this style:

table.system-status-report tr.error th {
  background-image: url('../../misc/watchdog-error.png');
}

It'll turn it into this in the compressed style:

table.system-status-report tr.error th {
  background-image: url(/projects/drupal5dev/modules/system/'../../misc/watchdog-error.png');
}

Also, images referenced by my theme's css file which was doesn't get it's path modified. This might not be a bug though since I included them manually through my template file. i.e..
$hook > case 'page':

if (file_exists($lumen_style = path_to_theme() .'/styles/base/lumen.css')) {
  $vars['css']['all']['theme'][$lumen_style] = TRUE;
}

Wish there was an easier way to include or unset css files.

robertDouglass’s picture

The image issue is interesting: I thought that having all included images in one monolithic CSS file was bad because the browser called all the image files it finds no matter whether they're actually used or not. In benchmarking, I suppose we need to test with CSS that is very image rich, and has different image sets for different areas or sections of the site. If there are 5-10 different layouts and each has 20-30 images in the CSS, what are the implications? (Let me know if I'm talking out of my depth here =) Otherwise, this is such a great concept, I'm very excited about it.

dvessel’s picture

The browsers I've tried load the image only when it's needed. Try changing a background image on :hover and notice how it displays after another http request a split second after the hover.

Regarding my previous comment on my theme style not getting its' paths modified.. It looks like it only checks the 'modules' & the base 'theme' directory. Would it be too much to have it modify the path wherever it's located? Hard coding for just two locations could cause more confusion.

m3avrck’s picture

FileSize
8.13 KB

Here's and updated patch, changes:

  • Fix incorrect cascading order of style sheets, core should be first, then modules and then themes

@dvessel - the image problem you speak off is to Drupal's inconsistent use of url() in CSS files. I wrote a patch to fix this, small patch, please set RTBC, that will fix the image problems you are reporting.

@dvessel - see the docs on using drupal_get_css() for why you're new file is not being cached.

@ak - this feature is sitewide and works the same way as Drupal's general cache mechanism. I see no reason to make this work on a per theme basis -- that's extra overhead that is not needed. The sites that need to do that, e.g., the one that are editing the theme on the live server, can turn off CSS caching. In those cases the site is going to be big enough that you'll see the huge effect of cached CSS files -- this patch really helps HUGE sites where every HTTP request is crucial.

@dopry - I'm not sure I agree with *not* caching theme CSS files for the same reason I just mentioned. Extra resources for little gain, if any.

@dries - this patch, when used, is going to lower the number of HTTP requests dramatically. Every site WILL speed up, as @jacuac pointed out. But what's the catch? Sure nothing is free, the catch is that there is some extra overhead when calling drupal_get_css() on each page now. This is close to minimal, but might require an extra CPU cycle or so. That would be the catch but that is outweighed by the benefits.

@everyone - There are currently *no* known bugs with this patch. Once this goes in, we need to do the same thing for JS files --- we're going to have the same problem in 5.0+ because of how easy it is to include jQuery problems and we already have this slightly on certain edit pages that include like 6 JS files.

m3avrck’s picture

Status: Needs work » Needs review
webchick’s picture

Status: Needs review » Needs work

Stupid question, but regarding ak's findings... does this mean this patch will continue to always choke on ' or " urls? I realize from reading the referenced issue that best practice is to leave those off, but:

a) there is no way to address a URL with a space in it without them, right? so we'd be placing limitations on theme authors in terms of where they could store their image files (and yes I know that %20 looks dorky in a URL but hey ;))
b) it's also fairly common practice to enclose in quotes so it seems like a reasonable thing for a theme author to do.

It seems like the best thing to do would be make whatever code robust enough that it could handle url(blah) or url('blah') or url("blah") but I might also be totally misunderstanding the issue. ;)

webchick’s picture

Status: Needs work » Needs review

Sorry. Didn't mean to change the status.

m3avrck’s picture

@webchick -- misunderstanding yes. This patch does address url(), url(''), and url("") ... the problem was for the ../../misc directory ONLY! And this was for core CSS files only, the extra patch in the other issue makes it consistent so my patch doesn't need a fancy regex in that *special* case. That's all.

So yes, no new issue here ;-)

webchick’s picture

Cool, thanks for clearing that up! :) Will try and review after some grub, hehe. ;)

rbrooks00’s picture

I tested this on a 5.0 site I'm working on right now and it seems to work as advertised. Reading through the thread it doesn't look like anyone has experienced any problems (i.e. bugs) with the patch and I'd have to say the evidence for improving performance is pretty clear too.

Given that, what is the issue preventing this from being RTBC?

Crell’s picture

I've not tested the patch yet (it looks like it's getting heavily tested by commenters above), but I did just read through the code. How does this handle @import statements within a CSS file? The designer I'm working with on a project right now had a half-dozen @imports in her style.css file until I told her that was bad for performance. It doesn't look to me like it addresses those at all. Will we just have to tell theme developers not to use @import?

Steven’s picture

Status: Needs review » Needs work

This patch seems to break color.module. Also, I don't think there is any problem with prefixing relative CSS urls such as '../../misc' with the full base path. The browser should resolve it correctly. The current fix is a really ugly hack.

m3avrck’s picture

Status: Needs work » Needs review
FileSize
8.91 KB

Updated patch, changes:

  • Fix paths to be more robust and work in all cases now
  • Fixes color module to work with new $css array
  • Clear cache when you enable/disable modules

Changes reflect talking with Steven on IRC.

@Crell -- if you haven't tested yet, don't start complaining ;-) This patch should work fine with @import() within CSS files

m3avrck’s picture

Err, Crell, depending on certain uses of @import, there might be an oddity. Needs some testing in that regard then to confirm.

dvessel’s picture

Few more problems I ran into. Most themers might not run into this but if I dynamically add a new css file *without* caching, a new cache file is created anyway with identical contents. Something about messing with the $css array it doesn't like.

This one isn't specific to the patch but the comment.css contains a single style rule and it's only included when viewing a full node. This needlessly causes another cache file to be recreated.

These little quirks can add up to more data being transferred. Besides that, the odd url() issues are spot on.

dvessel’s picture

I mean the url() issues are fixed. Solid!!

m3avrck’s picture

FileSize
9.32 KB

@dvessel -- another cache file is like 2kb, and it's part of the cache, that's how it works, no way around it. It's most efficient that way for speed but may use up some more space (like a couple kb).

Here's an updated patch which now properly handles @import correctly.

That was the last issue with this patch. As of now, *no* bugs are known and *no* outstanding issues.

dvessel’s picture

Status: Needs review » Reviewed & tested by the community

Few quirks aside from my bad theming practice, I think this is a must. It looks ready to me.

anders.fajerson’s picture

Does the default compression (removing whitespace) really gain that much in terms of speed?

When visiting a site I like to peak at the CSS, if the compression doesn't achieve much (benchmarks?) then it will mostly serve as obfuscating. And if you do wan't to compress the file, other more aggressive algorithms would probably be used instead, which was mentioned to be possible with outside hooks.

Steven’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
39.38 KB

Here's an improvement. It replaces the simple str_replace CSS compressor with a preg_replace based one, which strips out all non-essential whitespace and linebreaks.

// Perform some safe CSS optimizations
$data = preg_replace('<
\s*([@{}:;\)])\s* |          # Remove whitespace around separators.
/\*([^*\\\\]|\*(?!/))+\*/ |  # Remove comments that are not CSS hacks
[\n\r]                       # Remove line breaks
>xi', '\1', $data);
Steven’s picture

The savings gained from CSS compression tend to be about 10-15%. This is usually something like 3-4k and that's on top of the HTTP traffic that we save. This will make a big impact on busy sites.

m3avrck’s picture

New CSS compressor looks a lot better.

However, it seems that patch is a bit borked, with the removal of Garland in there ;-)

ByteEnable’s picture

I would also like to see module CSS that are for admin only not be loaded. For example, no normal user will ever see watchdog.css. Maybe modules that are admin level only should call drupal_add_css() if the current user is admin. Another admin only CSS that comes to mind is help.css.

Dries’s picture

m3avrck: I realize that this will speed performance, however, I'd like to know how much. We need to balance elegance and performance gain. This is one ugly hack and a usability regression so it better be blazing fast.

To me, it sounds like we don't really understand the performance implication/gain.

moshe weitzman’s picture

I can't think of a way to measure the end user's perceived speed benefit from the fewer http requests. it depends heavily on the latency of the user's connection, and the responsiveness of the web server.

perhaps we could put a comment at top of the generated css file with an inventory of the files that are included (including paths). that would help a designer find the source ... or maybe thats there already

BioALIEN’s picture

Coming from the mouth of a theme designer here. I havent yet tested this patch, but it seems to be related to the issue I opened up here after creating a new theme:
http://drupal.org/node/99470

1) Looking at this from a birds eye view, would you admit that splitting up the CSS into loads of small files was a bad idea?
2) Designers would rather work with fewer CSS files not 10+ opened up at the same time and having to find the cross references and classes between these CSS files.
3) As was pointed out by ByteEnable, there are lots of CSS files that shouldnt be preloaded. CSS files for admins and users shouldnt be presented to anonymous users. CSS files for admins shouldnt be presented for authenticated users.
4) Site in offline mode should ONLY have maintenance.css (this is crucial as it's breaking my themes) however, user/login should ship with the required CSS files preloaded (with taking point 3 into consideration of course).

I am not sure if this is related to this issue, but I was directed to this thread.

anders.fajerson’s picture

About the subject of what this will actually mean in real numbers: Safari has a feature, “Show Page Load Test Window”, which should be useful. I don't run OS X myself but I know some who does (Steven, ?). The feature and how to use it is mentioned on the Safari blog: http://webkit.org/blog/?p=75

From the article: "We’ve found that it’s a lot easier to improve performance when you have a precise way to measure it".

dvessel’s picture

Spitting up the styles it seems was just an unfinished idea. The 2 paths I'm aware of is leaving everything separated and conditionally loading only the needed styles or this patch to aggregate the styles.

Honestly, I'd be happy either way but I am leaning towards this solution. The less linked styles the better and the new ability to manage separate styles was always a good idea for development. Deployment is the other side of this and this patch does a fine job.

sime’s picture

Regarding performance.

The total number of imported css files on acko.net/garland is 16. If latency is 0.1s (conservative?) then you have over 1 extra second of load time for the user.

I hunted around the internet for some meaningful analysis, but I only found recommendations from obscure sites. (One of those sites put average latency at 0.2s) It would have been nice to post something from alistapart.com, or something. Anyone?

PS. I didn't look for examples of compression performance, because I think compression is not relevant to this issue and should not be in the patch. :-)

dvessel’s picture

There's a link to an article by a Google engineer from the link made by fajerstarter. He claims that the more requests for *small* files are made, there becomes a bottleneck from the clients upload bandwidth. Even he has to give guesstimates. I don't think you'll find hard benchmarks on this.

http://www.die.net/musings/page_load_time/

And yeah, Steve's latest patch fails on Garland's style.css.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
8.72 KB

I think Steven made a small mistake with uploading the compressed version of Garland's style.css and it's not his intention to distribute it that way. So I removed that hunk. Minor other changes I made is moving the @import regexp into a variable, and removed the i modifier from the whitespace preg as there is no alphanumeric char in that regex.

Otherwsie, what I see is a elegant way to make Drupal much faster. As others said, getting exact numbers is not easy, but see what Steven says: the whitespace removal alone decreases the necessary bandwith by 10-15%. Also note that if you have a necessarily complex Drupal site, and you have 40 modules on each with its own CSS and it takes 0.1s to issue a request then only requesting these makes the initial pageload four seconds. Hmm, that number sound familiar to me: http://www.akamai.com/html/about/press/releases/2006/press_110606.html Four seconds is the maximum length of time an average online shopper will wait for a Web page to load before potentially abandoning a retail site. -- the situation is dire. The browser will spend these critical seconds just to issue requests, they need to travel wire, the site needs to serve the objects etc.

So, having a CSS aggregator is crucial now we have a per module CSS files. And, if we do that why not add some whitespace removal?

And also, I fail to see any ugliness in the patch. We cache many things, and this is just caching again.

m3avrck’s picture

Agreed with all of Karoly's comments.

As for conditional CSS loading, that is a discussion left for another issue and patch and is a harder problem to solve.

Perhaps that may come in Drupal 6 with Karoly's improved menu system, we shall see.

bdragon’s picture

Since I'm on dialup, I experience bottlenecks like this more acutely.

Tests performed using Fiddler and IE6. The user-defined column is programmed to store the amount of time the request took, in milliseconds.

Theme is Garland, custom logo (6913 bytes). Drupal page cache is OFF.

I modified the patch to NOT perform the first optimization (the whitespace removal), as that was breaking Garland.

Pipelining is ON, two connections/server (My regular settings)

Before patch, force refresh

#	Result	Host	URL	Body	Caching	Content-Type	User-defined	
57	200	pw.rtk0.net	/index.php?q=tracker	7,710	no-store, no-cache, must-revalidate  Expires: Sun, 19 Nov 1978 05:00:00 GMT	text/html; charset=utf-8	2,384	
58	200	pw.rtk0.net	/modules/system/defaults.css	737		text/css	490	
59	200	pw.rtk0.net	/misc/jquery.js	19,077		application/octet-stream	4,947	
60	200	pw.rtk0.net	/modules/system/system.css	5,901		text/css	3,776	
61	200	pw.rtk0.net	/modules/user/user.css	605		text/css	660	
62	200	pw.rtk0.net	/modules/block/block.css	302		text/css	400	
63	200	pw.rtk0.net	/modules/help/help.css	166		text/css	410	
64	200	pw.rtk0.net	/modules/node/node.css	717		text/css	431	
65	200	pw.rtk0.net	/modules/menu/menu.css	804		text/css	531	
66	200	pw.rtk0.net	/modules/search/search.css	567		text/css	370	
67	200	pw.rtk0.net	/modules/watchdog/watchdog.css	519		text/css	381	
68	200	pw.rtk0.net	/sites/all/modules/thickbox/thickbox.css	1,611		text/css	541	
69	200	pw.rtk0.net	/sites/default/modules/devel/devel.css	566		text/css	510	
70	200	pw.rtk0.net	/themes/garland/style.css	15,998		text/css	2,414	
71	200	pw.rtk0.net	/misc/drupal.js	5,624		application/octet-stream	1,252	
72	200	pw.rtk0.net	/sites/all/modules/thickbox/thickbox.js	11,247		application/octet-stream	1,932	
73	200	pw.rtk0.net	/files/logo.png	6,913		image/png	4,045	
74	200	pw.rtk0.net	/themes/garland/fix-ie.css	1,053		text/css	3,014	
75	200	pw.rtk0.net	/themes/garland/images/body.png	712		image/png	761	
76	200	pw.rtk0.net	/themes/garland/images/menu-leaf.gif	175		image/gif	361	
77	200	pw.rtk0.net	/themes/garland/images/bg-content.png	485		image/png	481	
78	200	pw.rtk0.net	/themes/garland/images/bg-content-right.png	3,169		image/png	1,422	
79	200	pw.rtk0.net	/themes/garland/images/bg-content-left.png	3,275		image/png	2,324	
80	200	pw.rtk0.net	/themes/garland/images/bg-navigation.png	104		image/png	1,011	
81	200	pw.rtk0.net	/themes/garland/images/bg-navigation-item.png	502		image/png	491	

Total time for CSS files was 490 + 3776+660+400+410+431+531+370+381+541+510+2414+3014 = 13.438 seconds

Before patch, rerequest (all caching honored)

#	Result	Host	URL	Body	Caching	Content-Type	User-defined	
86	200	pw.rtk0.net	/index.php?q=tracker	7,710	no-store, no-cache, must-revalidate  Expires: Sun, 19 Nov 1978 05:00:00 GMT	text/html; charset=utf-8	2,013	

After patch, force refresh

#	Result	Host	URL	Body	Caching	Content-Type	User-defined	
182	200	pw.rtk0.net	/index.php?q=tracker	6,827	no-store, no-cache, must-revalidate  Expires: Sun, 19 Nov 1978 05:00:00 GMT	text/html; charset=utf-8	2,233	
183	200	pw.rtk0.net	/misc/jquery.js	19,077		application/octet-stream	5,017	
184	200	pw.rtk0.net	/files/css/20b18575da53c2e799860b63e05a5800.css	24,854		text/css	6,389	
185	200	pw.rtk0.net	/misc/drupal.js	5,624		application/octet-stream	2,204	
186	200	pw.rtk0.net	/sites/all/modules/thickbox/thickbox.js	11,247		application/octet-stream	2,093	
187	200	pw.rtk0.net	/files/logo.png	6,913		image/png	2,634	
188	200	pw.rtk0.net	/themes/garland/fix-ie.css	1,053		text/css	771	
189	200	pw.rtk0.net	/themes/garland/images/body.png	712		image/png	610	
190	200	pw.rtk0.net	/themes/garland/images/menu-leaf.gif	175		image/gif	381	
191	200	pw.rtk0.net	/themes/garland/images/bg-content.png	485		image/png	481	
192	200	pw.rtk0.net	/themes/garland/images/bg-content-right.png	3,169		image/png	2,623	
193	200	pw.rtk0.net	/themes/garland/images/bg-content-left.png	3,275		image/png	2,113	
194	200	pw.rtk0.net	/themes/garland/images/bg-navigation.png	104		image/png	371	
195	200	pw.rtk0.net	/themes/garland/images/bg-navigation-item.png	502		image/png	491	

Total time for CSS files was 6389+771 = 7.160 seconds

After patch, rerequest (all caching honored)

#	Result	Host	URL	Body	Caching	Content-Type	User-defined	
196	200	pw.rtk0.net	/index.php?q=tracker	6,827	no-store, no-cache, must-revalidate  Expires: Sun, 19 Nov 1978 05:00:00 GMT	text/html; charset=utf-8	2,193	

Before: 13.438 seconds
After: 7.160 seconds.
Time savings: 6.278 seconds
% Improvement: 46.72%

Dries’s picture

I'm going to give this some more thought. I'm still not fully convinced that this is the better solution. Rolling back the original patch and going back to drupal.css might make things easier (KISS) ... IMHO. This patch adds a lot of ugliness and inconveniences for what is only a relative small gain -- and for some people a pain.

BioALIEN’s picture

Title: CSS caching + aggregation » CSS caching + aggregation + the future?

I was waiting for Dries comments on this and there we have it. Surprisingly not far off from my views on the state of Drupal 5.x with regards to this subject.

Rolling back this change will at least enable themes compatibility until this issue is fully tackled and implemented.

However, the other side of the coin is that this patch couldnt really be avoided as it relates to the CSS and (possibly?) the JS files too (rolling back would only solve the CSS side, what about JS?). The side advantage is the whitespace removal which is an arguable benefit with this patch. Combine this patch with Gzip and you have a very fast Drupal site indeed.

The current issue of loading times shouldnt be taken lightly as has been stated many times. But the overall picture is how can we make Drupal run faster in general which comes down to the number of queries per page, etc. There are other nodes which discuss this in detail and a variety of other alternatives e.g. file based caching and serving those files if the db is down for anonymous users.

The current patch does indeed serve its purpose, but my fear is more to do with the concept of separating the css files (for the sake of theme designers?) then trying to regroup them server side. Either way, this whole issue seems to go back to Drupal's cach system which IMO benefits either way as it should allow admins to specify the following:
- allow for CSS caching (if we remain with multiple css files) using this patch
- allow for JS caching (similar to above but applicable to JS files)
- web based caching (ala http://drupal.org/project/fastpath_fscache)
- databased caching (which is already present)

So whether you base it on benchmark results or evaluate concepts based on vision for the future, a decision does need to be taken either directly from the top or as a community. My 0.02p is this may be the right time to start different distros of Drupal based on group visions rather than a general one size fits all as Dries covered this in his announcement.

Sorry if this is going completely of topic, but I tried to put all my arguments from all sides regarding this issue and the general scheme of things.

moshe weitzman’s picture

we are defaulting this to off. the only themers who could be confused by this have actively chosen to use it. and takes one click to shut it off again. i don't buy the argument that we are confusing themers. to me, this is a performance optimization like aggressive caching (which also has some negative side effects). i liked that patch, and i like this one too.

alternatively, i'd be ok if we put in enough hooks for a contrib module to implement this aggregation.

rolling back the split patch solves the issue for core but not for contrib. those css files will not get aggregated.

m3avrck’s picture

Title: CSS caching + aggregation + the future? » CSS caching + aggregation

Ok, let me play devil advocate here.

Fine, you revert what is in core and make it drupal.css again. Sure, you're going to make a lot of designers angry once again, but let's push that fact aside.

We now have drupal.css back. Cool, but now I enabled 15 modules on my site, each one had a CSS file.

We're now loading drupal.css + 15 CSS files = 16 http requests, it is STILL slow. This is how 4.7 works.

This patch fixes that prolem. I've used a similar, but more hackish approach on a 4.7 site that is receiving 3 million hits a day, to condense all of that into one file. It was THAT important to do so. It improved server load and page load times significantly.

Reverting the core CSS into drupal.css doesn't solve any issue in effect. drupal.css is *not* the problem, it only merely exacerbates the problem. In reality, drupal.css is a hack itself, it doesn't follow conventions on how we tell authors to write modules.

The issue is that drupal is modular--each module can load it's own CSS, JS files, and so forth. Any high performance site is going to need to merge any of these external files into a couple files for maximum performance, on both the client and server side.

If you want to solve this problem at the root, get rid of modules.

Yeah, I didn't think that would fly either :-p

This patch solves a problem that is a heart of Drupal--it merges all of these modular files into one file, that improves performance across the board. Doing the same thing for JS files will have the same effect. This is NOT a hack in any fashion. It is fixing the problem that is a result of a modular framework by unifying things.

Steven’s picture

I agree with Ted. Having separate .css files per module is cleaner, as it avoids unnecessary styles. Having Drupal be smart about the resources it uses is rarely a bad idea.

However, I only just discovered something which I hadn't seen mentioned, and which should've been caught in the original drupal.css patch. All core .css files are added unconditionally for every request, in hook_menu! This is lunacy and contributes to the inefficiency of the current situation.

Just like .js files, .css files should only be added when content they affect is being inserted into the page. This usually comes down to adding the relevant statement in menu callbacks and such.

ac’s picture

subscribing

Caleb G2’s picture

Someone please explain clearly how splitting css into multiple files helps themers. I can't for the life of me figure out how having to edit multiple files instead of one helps the theming process (not to mention the additional configuration options OR the performance issues).

Caleb G2’s picture

Missed this when I put my first reply together:

We're now loading drupal.css + 15 CSS files = 16 http requests, it is STILL slow. This is how 4.7 works.

How are we counting this? Where do the 15 css files come from? I'm sure I'm off base here, but I'm only figuring 4-5 css files tops for a 4.7 install including the theme's style.css. (are you counting the css defined in modules, and if so do those count exactly in the same way in terms of http request and such)

Also, even if 4.7 has 16 separate css files (a fact I'm still unsure of), how many separate css files does 5.0 have right now?

For someone who was doing a mass deployment of Drupal sites, and planning on supporting newbies/end-users with the sites - are the gains to be made with this patch worth the complications in usability (e.g, newbie doesn't understand caching or how to turn it off) - OR the performance hit that might occur if newbies/end-users just turned off all caching (this seems like it could really add up if you're hosting several non-cached/compressed 5.0 instances on a shared server)?

Actually I DO support end-users with Drupal, and right now I don't see any of gain to them in terms of anything (9 out of 10 NEVER want to touch any files anyway - even if it is "easier"), but I do see a lot of support/performance issues coming out of this for us.

What about an option whereby we go back to less css files, but then give the option to compress those or not? That way people who want to run it uncompressed aren't suffering a performance hit compared to 4.7 (since now there are more css files), and the people who know how and/or want to can run it cached/compressed to speed things up? That way it's not 'all or nothing' for newbies or for more advanced users.

Dries’s picture

Fine, you revert what is in core and make it drupal.css again. Sure, you're going to make a lot of designers angry once again, but let's push that fact aside.

I'm not sure this is the case. A lot of designers have voted against 20+ individual CSS files. There arguments make sense to. It looks, like you have two camps here: themers who prefer fewer/bigger CSS files, and themers who prefer more/smaller CSS files.

Dries’s picture

we are defaulting this to off. the only themers who could be confused by this have actively chosen to use it. and takes one click to shut it off again. i don't buy the argument that we are confusing themers. to me, this is a performance optimization like aggressive caching (which also has some negative side effects). i liked that patch, and i like this one too.

I think you need to look at this from another angle.

Most people use Drupal for small sites on cheap hosting accounts. For these people, turning on the CSS cache is difficult because they need to configure the files directory. Because of the extra work, and the complexity of those steps, a lot of small Drupal sites won't enable CSS caching. Result: a _lot_ of Drupal 5 sites will be a _lot_ slower than Drupal 4.7 sites. This isn't going to bode well with mass hosting companies.

I think we need to optimize Drupal for cheap hosting companies, not the other way around. This patch is for Drupal experts, and doesn't degrade well if you're a total newbie. That's bad, and it is going to backfire. People will claim that Drupal is slow, and mass hosting companies are going to drop Drupal from their offerings.

I am a performance addict, and I do like the performance gain introduced by this patch. Unfortunately, it comes at a performance cost for those who are not technically adept -- and that happens to be the majority of our users.

On top of that, this patch is quite a hack. People who say this is simpler, haven't looked at the code and regex hell required to support this.

Dries’s picture

Note that I'm not rejecting this patch. I'm still making up my mind.

Here is a quick review:

+    '#options' => array(t('No'), t('Yes')),

We never use 'No' and 'Yes'. For consistency, use 'Enabled' and 'Disabled'.

which can slow down the <strong>initial</strong> page load for a user browsing the site.

This is technically not correct. It affects all page loads. Even if the browser caches the CSS files, it needs to do a HEAD request to see if its local cache is still valid.

Third, what bothers me with this patch is that it doesn't play nice with the existing cache mechanism. It doesn't use database caching when I set my page cache to database caching. So now, I'm using both database caching (for pages) and file caching (for CSS files). I'm not sure this is bad, but it weird, and makes me wonder.

sime’s picture

I think this is my rant.

What I've come to terms with:

  1. If Steven is right at #65, and 16 files per page is a bug, then we don't need aggregation.
  2. Why do we want to cache something that is already "cached" in the file system?
  3. Plus, I've always thought that compression is a separate issue.

So why are people so passionate about this?

Assumption. Supporters of this thread are like me: Hybrid Dev-Designers. The developer (me), also wants better tools for the designer (also me). To develop these tools, I want information stored in the database!!

Seems irrelevant?

Let's say that this patch goes through and we have a way to aggregate separate CSS files. Well, the next logical step is to be able to combine normal CSS files with CSS from the database. Because, if we can serve chunks of CSS from the database, without worrying about file permissions, or suffering a performance loss serving it as separate files... then we will suddenly be able to create nifty solutions for various design UI problems, without having to be Steven Wittens at the same time.

So I really think aggregation/caching is fantastic - because it creates the possibility of having CSS in the database without performance penalty. However, I am also starting to see why it's not adding up for Dries.

Enough crap from me. Just no more arguments that "designers want to edit drupal.css". Real (non-programming) designers want User Interface. CSS stored in files is never going to deliver that - CSS in files is KISS like index.html. :-/

Frando’s picture

Someone please explain clearly how splitting css into multiple files helps themers. I can't for the life of me figure out how having to edit multiple files instead of one helps the theming process (not to mention the additional configuration options OR the performance issues).

Hmm, when I do a drupal theme, I don't modify any files. I just overwrite the styles that I want to change in my theme's css file. So out of a themer's perspective, it doesn't matter *at all* wether there are 20 or 1 css files in core.

I'm totally in favor of this patch, not only because of core, but if you take contrib into consideration, you can easily end up with a site that has 20+ css files, which then is *really* slow. So drupal is modular, which is a great thing, and one of the purposes of drupal core should be to unify these modules where unification is needed. IMO it is needed here.

The big problem with caching the css in the database is that we'd need another drupal bootstrapping and database connection for the css. This will have a negative impact on loading time whenever the database is the bottleneck. In my opinion, it sounds much more logical to do file caching for css and then include the aggregated style sheet in a regular way (direct HTTP file request) instead of having to do a drupal bootstrap and establishing a database connection just for the stylesheet.

What we could do is to add a little comment to the top of the aggregated stylesheet that lists all the files that are added to the cached version.

moshe weitzman’s picture

One more argument for multiple css files versus one big one: less work for the browser. If we conditionally include css files, we are including less css code that the browser has to parse and then apply during rendering. With one big file, browser does unneeded work on every request. This is independant of browser caching and so on.

We should do a better job of conditionally including these small files (as Steven and others mentioned), but thats a separate issue, and not so hard to solve.

jacauc’s picture

Why do we want to cache something that is already "cached" in the file system?

Just in answer to that question: The original issue was created paying special attention to Initial/First load... The First time a visitor comes to the site (ever). So it is not cached in the filesystem yet.

For what it's worth, I also support the patch. No idea how it's done in the back-end with regard to dirty/clean fix though, But i support the idea 100%!

dvessel’s picture

I didn't make this clear before but going beyond developing themes is a bit foggy to me. So, to answer this from my perspective..

Someone please explain clearly how splitting css into multiple files helps themers. I can't for the life of me figure out how having to edit multiple files instead of one helps the theming process

It's easier to manage little chunks associated with a module. The monolithic drupal.css file is more difficult to wrap my head around. It's not as easy to know what styles are meant for which circumstances. Grouping them into individual files differentiates them clearly and as long as each style associated with a module is consistent, I don't have to second guess myself on if I used the correct selector to override a particular style.

It's also trivial to unset a file and include my own then work off a copy of the style. This is the beauty of making it modular.

When I work with drupal.css, I have to make sure every property is overridden when using the same ruleset. It makes it overly verbose or the property inside drupal.css takes over. Either that or I disable drupal.css altogether and try to clean it up directly which is also tedious.

Whatever is decided upon, please keep the styles separated. This is a big deal to me and a lot of other theme devs.

Morbus Iff’s picture

I'm a bit confused. Why are we making a gigantic workaround (and, to echo others, a rather hackish/unfriendly one, IMO) when modules are incorrectly sticking their CSS in hook_menu()? The only time this should ever happen is if the module comes with a block, and that particular CSS could be shunted into another file (making the file size, and thus savings caused by compression/whitespace, quite small). Modules should stop being lazy and move their CSS includes to the time they actually use them.

Morbus Iff’s picture

Actually, ignore my comments about blocks in #77. A module would add its CSS on an $op 'view', which would only ever happen if the block was actually enabled and intended for display (which may not be on every page with PHP or role-based booleans).

BioALIEN’s picture

FileSize
22.99 KB

I'm going to have to agree with the last comment, having just created a theme for Drupal 4.7.x and for Drupal 5.x-beta so I've experienced this from both sides.

This discussion is really getting difficult, I'd hate to be the one who makes the final decision ;)
For the record, I still stand by my notion posted earlier, but to address Dries's concerns, would it not be possible to complement Drupal's cache settings?

I've wipped up a screenshot to illustrate my point. Since Drupal is shipped with Caching = Disabled, it shouldnt present a problem. Also, its important to mention that on a new drupal install as soon as you visit the Admin page, you're immediately being shouted at that "/files" directory doesnt exist and/or is not writable. So from a newbie point of view, this is already the case with the existing Drupal 4.x and it also partially solves this patch (if filesystem catch is enabled).

Of course, you can bypass all this and choose the Database method and be on your way also.

BioALIEN’s picture

Oops, by last comment I was referring to #76. Morbus Iff jumped ahead of me to reply :)

meba’s picture

+1 to go back to drupal.css as one big file. This patch will be VERY confusing for all experienced users and theme developers. I think the result will be that all users will start editing md5.css file in files/css because they will find it in HTML source.

let's make one file and comment it properly (divide into sections)

Morbus Iff’s picture

-1 on drupal.css. There were numerous reasons we got rid of that. Going back there would be starting over. Could you explain why you don't want modules to intelligently add their CSS to the output, as opposed to "always on", as most of core is now?

eaton’s picture

+1 to go back to drupal.css as one big file. This patch will be VERY confusing for all experienced users and theme developers. I think the result will be that all users will start editing md5.css file in files/css because they will find it in HTML source.

I'm still baffled by the people who say this will be confusing. It's a caching feature that will be turned off by default, that theme developers can turn off while developing their themes. 'All' experienced users? The ones smart enough to go in and turn on CSS caching, but not smart enough to realize that means the CSS will be put into a cached file?

The monolithic drupal.css was a constant nonstop source of complains by the experienced theme developers. They wanted it broken up. One of them put a bunch of time into making that happen. And now there's a need for a follow-up speed optimization. This is a good patch, no more hacky than Drupal's existing cache system.

pwolanin’s picture

@Dries- re: your comments in #70 about the ugly regex required. I assume you mean the regex for the whitespace removal?

In general, I think this aggregation is a good idea (based on the above comments and benchmarks). At the least, a mechanism should be implemented for aggregating the CSS and JS files without performing any whitespace stripping. Comments could even be added into the aggregated file like

/* Don't edit this system-generated file!  The source CSS for the section below is in modules/book/book.css */
m3avrck’s picture

Ok, everyone is failing to see the point of this patch, except for a few, so I'll try one last time, then I give up.

Let's step back a second. WHY did we split up drupal.css? Because drupal.css was *A HACK*. It was a special case for core, to lump each CSS file for each module into one file. Well that's dandy, but I bet on average each site only needed 1/10 of that file. So it was a waste. It was split up so only the CSS that your site needs is downloaded. This also made core modules work like contrib: each module had it's own CSS file. It then made it easy to unset specific CSS styles, as I outlined here.

If you revert drupal.css, you are introducing a *hack* that makes core Drupal modules special and they don't have to play by the similar rules like other modules. That's a hack. Period.

Is it easier to theme if it is split up? No, it's the same. It makes it easier to find styles to override in your theme, has nothing to do about editing. If you are editing core CSS files directly, then you are hacking core, and you're on your own. Split or combined, doesn't matter, since you should not be editing those files directly.

The only good argument for why drupal.css should exist is because it's faster-- well duh, that's the whole point of this patch, and to also include contributed modules and themes in this *single* file.

So whether you revert drupal.css or leave it, IS NOT THE POINT OF THIS PATCH. Whether it is there or not, you still have contributed modules with their own CSS files. You still have multiple CSS files per page load, which is too many. Same goes for JS files.

It's too many for the browser and too many the server. Don't believe me, have a thorough read here. If you're too lazy, just read this then:

Load fewer external objects. Due to request overhead, one bigger file just loads faster than two smaller ones half its size. Figure out how to globally reference the same one or two javascript files and one or two external stylesheets instead of many; if you have more, try preprocessing them when you publish them.

This is what this patch does -- IT ADDS A PREPROCESSOR to Drupal to process CSS files (and hopefully soon JS files), which aggregates them into one file and adds some compression, which speeds things up a nice percentage.

It's not a hack. Dries, if you think this is a hack, then you have not looked at color.module because there is some minor overlap in code, but color.module has more "hacks" in that sense.

And does this make it harder for newbies? Why would it. It works the same way as database caching -- on or off, and it's off by default. You can turn it on, but there is a caveat to make sure you clear cache if you make changes, same caveat as the database caching.

And no, these caches should not be the same, they are different. One cache, reduces the queries on the page from 100 to say a couple. This patch reduces the number of files from 100 to a 1. One is database, one is file. Both are clear and distinct.

The only thing to change in this patch (besides Dries remark) would be to change the wording from "caching" to "preprocessing". This is what this patch does at it's core and maybe that is the sense of confusion, I'd be ok changing the terminology there.

meba’s picture

Morbus, I am just so unhappy about "files/css/d40d2e46f62ab8c9e0a0a.css" in HTML source. I think you are right. Let's commit this. But turn it ON by default. We can always print "There was a problem with this Drupal installation: Directory files/ is not writable. Turned off CSS cache, please turn it on after correcting this problem at URL..." to administrator and turn the feature to Off programatically.

Owen Barton’s picture

Most of the points raised here have already been discussed at issue #81835. If you haven't already, please go and read that!

Some specific FAQs:

Where are the benchmarks showing the improvement?
See #81835. My benchmarks there tested the results of this patch (single css as opposed to multiple), and found a very large effect on page delivery time from multiple CSS files. The only thing not included in that benchmark was the creation of the initial cache file, which is certainly negligible.

How do I benchmark this?
We need to:
(a) Simulate a 'real' network connection (with lag and typical download speeds), but we obviously can't do these tests on a live site, as internet load times are too eratic
(b) Simulate an initial page load (i.e. ensure that the browser has nothing previously cached)
(c) Simulate a typical site, with 15 or so modules enabled

To do this I did as follows:
* Used Apache running (without any additional load) on my local machine
* Used the Charles proxy to throttle and lag downloads to the level of a 64kbps line, and also to time & measure download times
* Used a default Firefox browser profile, with caching disabled and no extensions
* Used a default Drupal install, with all core modules enabled. Of course, a normal site would probably have some core modules disabled, but they would also almost certainly be using several contrib modules. I didn't want to do this however, because contrib modules might potentially have performance problems of their own.
* Used an anonymous user (typical for initial page load)

Why not just have modules selectively include the CSS if required?
This is not a bad compromise - but there are still many situations where multiple content types and multiple blocks on a homepage could lead to 10 or more CSS includes. This is still a hefty hit.
In addition, it pretty much breaks any chance of aggregating/compressing the CSS, since each page can have a different payload.

Why not just go back to drupal.css?
I think this has been answered more than adequately if you go back and read the previous issues.

Why bother with a hook to allow gzip compression?
Because is a contrib module implemented this it would cut down css (and js, if we want) file size and associated bandwidth to 20-30% of the original size.

dvessel’s picture

@m3avrck, No one said anything about editing core styles directly. Understanding where a style comes from and where it applies does make it a lot easier for themers. I know there's a bigger issue behind it outside of theming but there's no point in me speaking up about that.

+1 sticking to separate .css!!
+1 to this patch.!

Owen Barton’s picture

I think we really only have 5 options here:

  1. We ignore it, and hope that not too many people are turned away by a slower initial load times
  2. We go back to the old system (drupal.css), with a loss in maintainability and flexibility
  3. Try and selectively include CSS files only when they are needed and minimize the number needed on the home page. Note that this option is fundamentally incompatible with (4) and (5).
  4. Current patch: we implement a simple caching system, where individual css files are appended to a single [media-type].css and placed in the files directory. The browser is then directed to that file. This cache file only needs to be updated when the individual css files are changed.
  5. We implement hooks in drupal_get_css() that allow a contrib module to provide option (3). This is equivalent to pushing the majority of the current patch off to contrib.

Here are the pros and cons as I see them:

  • (1) and (2) are obviously not good options.
  • (3) is a compromise on performance - it is still problematic is a site uses lots of modules that place content on the home page
  • (4) is the current patch, it adds some complexity, but is less of a hack than drupal.css was and offers the biggest performance gains.
    I am a little concerned about how robust the whitespace and URL regexps will be when standing up against hacked up (and potentially invalid) CSS code.

    However, it is a start, and something we can definitely work with and improve. It also offers lots of potential side benefits, such as the massive file size reduction offered by gzip and the potential to offer garland-style dynamic CSS with friendly interfaces (which is actually a big usability benefit for real newbies, compared to learning CSS).

    The "on/off by default" question raised by Dries, in relation to shared hosts is an interesting one. My personal feeing is that it would be more friendly to shared hosting to have it on by default. The issue then is that it would be confusing for newbies, since their CSS changes would no longer update. One way around this (with, I guess, a very minor performance hit, compared to multiple http requests) would be to check the file update times on each request, and refresh the cache when changes are noted.

  • (5) is, like (3) another compromise option, because techy users will always install the css caching contrib module, while the vast majority of (newbie) Drupal site admins will just put up with slow performance, not knowing what the problem is or how to fix it.
Caleb G2’s picture

Am not sure why everyone decided to focus only on the editability issues. There's are other issues:

Most people use Drupal for small sites on cheap hosting accounts. For these people, turning on the CSS cache is difficult because they need to configure the files directory. Because of the extra work, and the complexity of those steps, a lot of small Drupal sites won't enable CSS caching. Result: a _lot_ of Drupal 5 sites will be a _lot_ slower than Drupal 4.7 sites. This isn't going to bode well with mass hosting companies.

I think we need to optimize Drupal for cheap hosting companies, not the other way around. This patch is for Drupal experts, and doesn't degrade well if you're a total newbie. That's bad, and it is going to backfire. People will claim that Drupal is slow, and mass hosting companies are going to drop Drupal from their offerings.

I'll leave it to those who know the specific to work out the details, but +1 for some sort of compromise between newbie/hosting usability and advanced caching/compression features.

jacauc’s picture

Thanks m3avrck for your previous post.
It seems the issue got a bit sidetracked there.

With regard to the regexp whitespace removal, couldn't we consider the following options (seeing that whitespace removal was not really in the initial proposal: see http://drupal.org/node/81835#comment-157388)
1) CSS Caching Off
2) CSS Cacing On - No whitespace removal
3) CSS Caching On - With Whitespace removal

This way, people who want to "risk" option 3 are welcome to. ...And people who do not want to take the chance with regexp removal, can choose option 2.
This way, option 2 can gradually disappear in future releases once we have a solid baseline/knowing that the algorithm for removing whitespace is robust.

I am already comfortable myself with choosing option 3. But that's a matter of personal taste, trust & risk. Other might find it more difficult to take a chance like this... but within time those creases will sort themselves out.

...and finally, this patch has, in my understanding, no effect on a theme developer. As you say, it's a preprocessor, and to the theme developer, it should be transparent.

Caleb G2’s picture

Grugnog2's comments were posted as I was making mine above.

My personal feeing is that it would be more friendly to shared hosting to have it on by default. The issue then is that it would be confusing for newbies, since their CSS changes would no longer update. One way around this (with, I guess, a very minor performance hit, compared to multiple http requests) would be to check the file update times on each request, and refresh the cache when changes are noted.

It would be friendlier to hosts until, as you indicated in your next sentence, the user starts wondering what the heck was going on and/or how to deal with. One more (big) layer in trying to communicate site editing to a new Drupal user - which believe me - is like wrangling cats in the first place. I get enough of those kind of support tickets now.

For something to compare to - with our 4-7 installs, most of our users just give up with the current system and live with it, pay us to make changes for them, or give up on Drupal (and us) entirely. Was very excited to see the color.module, since easy editing like that is a brilliant way to shore up the happiness of users who don't have the budget/don't want to pay for custom work - and above all else - refuse to learn anything about editing even the smallest file. Adding more configuration options which further complicate the editing process is going the opposite way as the color.module.

If the update-for-changes idea could be made foolproof then i guess there wouldn't be a problem, but I'm fairly skeptical that's a fool proof proposition based on my experience with the blockcache.module which does the same type of thing you're referring to. (that's a good example of something that can be a nice performance addition, but also adds a layer of complication which can have serious consequences if not configured correctly - there's a definite learning curve to climb)

BioALIEN’s picture

FileSize
23.45 KB

Jacauc raised a very good point in his post (#91) which I missed in my concept attachment. So here's the updated concept image with the addition of Gzip and Whitespace removal.

Attached!

dvessel’s picture

Considering all that's been said above, I agree page caching & css preprocessing should be kept distinct. Even .js & .css as both types of files have their own browser quirks that must be dealt with separately.

Wanna correct myself also. The single drupal.css file isn't difficult to understand. Looking back on it now after experimenting with v5, it's a lot more clear to me for some reason. Still, depending on cascading selectors and verbose overriding properties makes it more difficult to manage. Unsetting then adding my own styles make it easier to work with.

Why not make it a requirement to have a writable '/sites/default' directory? The '/sites/default/settings.php' file has to be writable for the installer to work anyway. Create the files folder inside '/sites/default/' when installing and there's one less issue to deal with. I know I'm missing something here since this is pretty obvious.

eaton’s picture

Most people use Drupal for small sites on cheap hosting accounts. For these people, turning on the CSS cache is difficult because they need to configure the files directory. Because of the extra work, and the complexity of those steps, a lot of small Drupal sites won't enable CSS caching. Result: a _lot_ of Drupal 5 sites will be a _lot_ slower than Drupal 4.7 sites. This isn't going to bode well with mass hosting companies.

Where are these people who run high-traffic Drupal sites on dirt cheap hosting and don't know how to set up a files directory? I have a seven dollar a month hosting account and it's never given me problems in that regard, even when I was an utter drupal newbie who had never written line of PHP in my life. If they don't have a writable files directory, they can't use the upload module, they can't use the color module, they can't use audio module, they can't use image module, they can't... well, they can't do a lot of stuff.

Any site generating enough traffic with Drupal to anger an ISP with too many CSS file hits will be positively crushed running any number of other CMS's that use separate files and less efficient caching than Drupal already does. The CSS issue is important, and this patch is a good one, but suggesting that 5.0 will be some sort of plague on low-cost hosting companies is blowing things out of proportion. The hit they take from modules that don't use database caching effectively is far more serious from a hosting perspective.

We use the compressed version of jQuery. We store pages in the cache table to speed up busy sites. And with this patch, we'll aggregate module-added CSS files into a single efficient one to save speed, too. If there are technical problems or bugs with this code, then they can be fixed. But the latest round of objections are based on a fundamental misunderstanding of what function the patch serves.

ac’s picture

If this patch gets in please make sure module and core styles are added to the 'one big dynamic file' before the theme stylesheet is. Lots of people use drupal for its multisite functionality and don't want to edit anything in modules/. These people need to be able to overide module specific styles in their theme stylesheet.
Otherwise +1 for patch.

m3avrck’s picture

ac -- yes, I wouldn't have it any other way. The aggregated CSS file still properly supports cascading that's the beauty of it, it's not just a lump :-)

moshe weitzman’s picture

ok, lets give this issue a rest for 24 hours. there have 32 followups today ... it is time for the CVS reviewers to decide based on the excellent feedback given. Please only post here if you have something new to add (which is almost impossible by now). And please don't just vote for your favorite outcome. See the patch review guidelines at http://drupal.org/patch/review.

jacauc’s picture

One question: Before the final judgement is made on this patch, would a revised patch be released beforehand?
...As there are many other things discussed here, that is not in the last posted patch.

Dries’s picture

Decision #1: I won't revert the drupal.css patch.

Decision #2: I'm not ready to commit the proposed pre-processor patch.

The more I think about it, the more I lean towards conditionally including CSS files, as proposed by Steven and Moshe. I think we need to explore that path some more before we can make a final decision. Right now, we can't weight the pros and cons of each approach.

The screenshots of BioALIEN do a good job capturing the different configuration aspects. However, implementing that seems like a fair amount of work -- and is not something I'd advise this late in the release cycle. (That said, feel free to work on it -- it can go in early on in the Drupal 6 developement cycle.)

Morbus Iff’s picture

m3avrck: could you specifically address why we shouldn't be correcting the real issue, which is the fact that modules are including their CSS files too often and too early? Have we legitimately tried to fix that issue first? Including a CSS file in every page load, when it's not necessary, is a bug - instead of solving that bug, this patch attempts to introduce a new feature to /make that bug acceptable/. That's just not something I can +1, no matter how great the idea.

Grugnon2 says in regards to proper CSS inclusion: "In addition, it pretty much breaks any chance of aggregating/compressing the CSS, since each page can have a different payload." As above, this says "I don't care that modules are doing it wrong - in fact, this feature /requires/ that they do it wrong, and anyone doing it right /actively/ harms this patch's performance." That's horrible.

It just amazes me that people are supporting a patch the solves the wrong issue entirely. And then to suggest that modules who are doing it /right/ would actively cause this patch not to work as awesome as it should. Even if this feature DID make it into core (in combination with proper CSS inclusion), it should not "punish" a module developer who is doing /the right thing/ - by including their CSS only when they need it.

scroogie’s picture

Right now, we can't weight the pros and cons of each approach.

Why not having both the conditional includes and the css caching? This is only an additional feature and will improve performance no matter if the files are included conditionally or not. Correct me if im wrong.

eaton’s picture

Morbus, the problem isn't *just* that modules are including their CSS files willy nilly. That is relatively easy to fix on an ad-hoc basis. The problem is that storing individual CSS files makes development much easier, but page-loads slower. Selective CSS inclusion will definitely help that, but the problem will always be there to some extent.

From the page optimization link referenced earlier in the thread, the solution is simple:

Load fewer external objects. Due to request overhead, one bigger file just loads faster than two smaller ones half its size. Figure out how to globally reference the same one or two javascript files and one or two external stylesheets instead of many; if you have more, try preprocessing them when you publish them.

That's all this patch does. As people start selectively including their CSS, it will keep working. It will generate a larger 'mix' of pre-processed files, but the advantage lies in the preprocessing to reduce HTTP requests.

It just amazes me that people are supporting a patch the solves the wrong issue entirely. And then to suggest that modules who are doing it /right/ would actively cause this patch not to work as awesome as it should.

I wouldn't say that. I'd say instead that the patch speeds up drupal when module developers do the right thing, and it ALSO helps reduce the penalty when they do the wrong thing. We still want to encourage them to do the right thing.

dvessel’s picture

@Scroogie, read Morbus' reply to Grugnon2. It's pretty much one or the other. How can you aggregate something that is constantly changing and expect css caching to take place? Well, that won't work.

Hope to test out the alternative soon.

pwolanin’s picture

@Eaton - I'm looking through the patch code, and I don't quite understand your comments in relationship to the code. It seems as though if module CSS files are "properly" only included as needed, there needs to be a pre-processed CSS file per path and/or a table that's mapping each path to a CSS file. I don't see this anywhere in the code.

pwolanin’s picture

oops- nevermind. I see now that the md5 of the CSS is calculated every time in drupal_get_css().

Crell’s picture

I originally was on the "only include CSS when needed" side, and submitted a patch to start doing so. However, I now agree that caching is the better solution.

1) Conditionally including CSS still means multiple HTTP did-you-change requests per page. I hadn't thought of this as an issue before, but the benchmarks posted here and in the previous thread have convinced me that it is. Think of a views page, and potentially how many sheets it could have: system.css, views.css, nodea.css, nodeb.css, cck.css, style.css, and that's not counting any blocks. I have also written other systems (non-Drupal) where I did conditional includes, and there was a considerable pause on the start of every page. (I didn't realize why that was until this thread, actually.)

2) Conditional includes means you have inconsistent collissions. If two modules happen to use the same class name in their stylesheet, then when you include both they will clash and one will screw up the other. If they're both included only sometimes, that will be an intermittent problem and therefore harder to track down.

3) The "nasty" regex in the caching mechanism is run once, so its performance is not a bottleneck.

4) A writable files directory is already required for Drupal to function in the first place. Even an FTP-only host should allow you to chmod a directory. If you can't, then you can't use the new installer anyway (since you need to chmod settings.php). This is a red herring on this issue.

5) Yes, a cached CSS file will mean that the initial page load will include all the CSS you'll ever need, which increases bandwidth. However, it does so in one file instead of the absolute minimum of 2 of Drupal 4.7 (drupal.css and style.css), so it is at worst a reduction of one HTTP request over 4.7. Based on the module's I've seen (which I do not claim is exhaustive), most modules have either no CSS or very small CSS files. drupal.css and its descendants and the theme's style.css make up the vast majority of the actual CSS that needs to get sent. The extra 2-3 rules, especially if whitespace compressed, are really not going to make a major difference. I could maybe see an argument for the maintenance.css file or other things that are used on only 1-2 admin pages, but that's it.

6) On the assumption that the extra payload from module CSS is small per module, then yes, conditionally including CSS will increase page load time with this patch because different pages will have different compressed versions, and the entire theme CSS has to get sent a second time. I don't see that as punishing people who do it "right"; it's acknowleding that "right" will vary with the framework you're using. This patch would make the ideal "right" way of handling it be a hook_resources() of some sort where modules can declare what CSS/JS files they come with so that they can get cached once and be done with it. Let it get cached/compressed once and sent to the browser once, and then there's a cost of only one HTTP hit per page after that.

I didn't originally think it was worth the effort to try and build a caching mechanism, particularly post-code freeze. m3avrck has demonstrated otherwise. :-) +1 to this patch in concept. (I still need to spend more time testing it, but spent the weekend trying to get my mail server working again instead. Grrr...)

scroogie’s picture

How can you aggregate something that is constantly changing and expect css caching to take place? Well, that won't work.

Thats pretty much what every cache does. The css is not changing constantly for the _same_ page, it only changes for _different_ pages each of which will have an own cache file. When you have correct conditional includes, you will have one page which includes 10 css files, and another which includes only 5. after the aggregation, both of them includes only 1 css file. Do you really not see the advantage of that?

mfer’s picture

I was just taking a look at the 2nd beta over at http://drupal5-0.highervisibilitywebsites.com/

This just has a few/several of the core modules turned on. Hitting that homepage loads 17 css files.

While, for development having everything broken up into separate css files is great having them that way with a production site is a pain for visitors.

m3avrck’s picture

Conditional includes of when CSS should be left for another issue/patch. That is the not the point of this patch.

m3avrck’s picture

Conditionally loading files will *not* solve this issue. You will still run into the fact that you are loading a half dozen or more CSS files per page. Guaranteed. That might be better than say 12, but it's still too many.

This patch addresses this issue in its entirety. It solves *the whole* problem.

mfer’s picture

I am looking at this as someone who will launch at least a couple dozen sites with drupal 5. Having more than several css files is something that will cause a hit to the end user. A hit that more than makes up for all of the performance enhancements that have come recently.

Is something that might seem a little hackish worth it to implement if it gives that big of a performance boost? I would hope so unless there is a better solution.

If drupal 5 comes out without it there will be a significant speed drop and most won't know why or how to get around it. +1 for this patch. Even if it's only for drupal 5 and a better and more permanent solution comes out in drupal 6.

Owen Barton’s picture

Here is another compromise approach (lets call it #6) that would enable us to push this patch out to contrib for 5.0 and (at the same time) have Drupal include CSS only where required by default. This gets around the basic incompatibility of the two approaches I mentioned previously under approach (5).

  • Each module implements a hook_css_define() where they return a list of all css files they use (perhaps we also implement a filename convention which is added automatically - e.g. modulename.css)
  • This array is then padded out with a status variable (default false) for each file
  • Modules can then fire a load_css($file) function whenever they need their CSS included, which switches the status to true
  • With Drupal core, this list of enabled files is then passed on for output as css includes as normal
  • A contrib module can now implement a hook_css_adjust() (which is fired after all modules have finished) to collate and edit the CSS file list (replacing with a single aggregate file containing all possible CSS files - not just ones enabled for the current page)

This is basically the same as the 'conditionally include CSS' approach (#3) - with 2 exceptions:

  • We would collect a 'definitive' list of all CSS files, so a basically static cache file can be created. If we don't do this, then having the hooks is of no use - since we can't produce a cache of all available CSS. @scroogie suggested that the list of current CSS files could be used to create a new cache for every combination - but this is obviously flawed, since the same data will end up being downloaded many many times for a single user.
  • The hook functions (which could mostly be taken from the current patch) would be added in, allowing contrib modules to jump in and modify the file list.

Neither of these will require very many lines of code at all, or any additional regexps in core - but will allow both the main approaches to be possible, whilst (for now) keeping the simplest approach.

Despite @morbus' assertion that CSS was originally intended to be added conditionally (which is correct), the somewhat nonintuitive fact remains that this simply does not scale acceptably when you have more than 2-3 CSS files. Even if the number of conditionally included CSS files on the homepage could be got down to (e.g.) 6-7, it is still likely to be faster for the end user to download a single CSS file (even if it is bigger and includes all (e.g.) 15 CSS files for the whole site). You don't just have the network latency to consider, but also the cost of setting up http requests for each file, that likelyhood of these requests exceeding the browsers number of http pipelines, and even the bandwidth wastage of at the packet level when sending a 4 or 5 line CSS file (not to mention the bandwidth consumed by the multiple http headers). This point was extensively discussed in the previous issue (#81835).

Approaches #4 (m3avrck's patch, #5 (non-conditional includes + hooks) and #6 (this suggestion -> conditional includes + hooks) are still the only ones I can see that will not introduce unacceptable delays into the page load time.

Dries’s picture

m3avrck’s picture

Dries, no one ever disagreed with the fact, the followup to that comment:

@kasted: I agree. I submitted this request a few months ago, but so far has gotten no love.

http://drupal.org/node/82782

I find the multiple CSS files handy for development & tweaking purposes, but ultimately, you get about 10 CSS and JS files loading, and this is obviously not optimal...

Which is EXACTLY the point of this patch.

The multiple files are a nightmare for designers because it is hard to work with 17 files at once. BUT NO ONE IS WORKING WITH 17 FILES AT ONCE -- those are ALREADY GENERATED. Any new theme has 1 file at max. Themes are working on this file and reference the other 17 IF NEEDED. But that file needs to be merged with the other ones that it is interacting with so there is ONE file once development is over.

There is a huge and subtle difference here that most people are missing and I'm tired of arguing. Then again, I only wrote the TWiT CSS (http://www.twit.tv/themes/twit/style.css) and (http://www.twit.tv/themes/twit/style2.css) which had a combined total of 1200 lines of CSS (yes not compressed at all).

RobRoy’s picture

Just to agree with m3avrck and others here and be redundant :). Convenience for CSS designers is not the issue at hand, but regardless, this patch won't affect theme designers. Use the web developer toolbar for Firefox to look at the CSS and do a search on a selector if you don't know which file it's in. It should be obvious where you want to go if you need to look at an example in the drupal CSS files, but you'll be doing all the overriding in your own style.css so you shouldn't be bouncing around anyway! We could even include a /*** misc/whatever.css ***/ above each file in the compressed version if people are checking that out. (Someone already suggested this, not sure if it has been implemented.)

Performance is the issue here (yes, I know I'm not saying anything new) and I think we should improve our use of selectively including CSS files only when needed, but that is another issue and this patch still stands even if we do that as well (which I think we should).

So rock on m3avrck, et al. This is much needed.

scroogie’s picture

This gets around the basic incompatibility of the two approaches I mentioned previously under approach (5)

I really dont see this incompatability your talking about. What is it? The patch already works with changing css!

Owen Barton’s picture

@grugnog: This gets around the basic incompatibility of the two approaches I mentioned previously under approach (5)

@scroogle: I really dont see this incompatability your talking about. What is it? The patch already works with changing css!

OK, here is the problem:
Our site has 5 css files: a, b, c, d and e. These are added conditionally based on whether they are actually needed for that page, and a cache file is generated using the concatenated (md5-ed) css filenames as the cache css filename.
-> The user visits / - files a, b, and e are included in the abe-cache.css file, and the user downloads that file.
-> The user then visits /node/123 - files a and b are included in the ab-cache.css file, and the user downloads that file.
-> The user then visits /events - files a, b, d and e are included in the abde-cache.css file, and the user downloads that file.
-> The user then visits /search - files a, c and e are included in the ace-cache.css file, and the user downloads that file.

As you can see the user has now downloaded the css for a 4 times over, b 3 times over, e 3 times over and so on. Because the cache consists of all the files appended together the browser has no way of knowing that chunk has already been downloaded, and so much of the same css code is downloaded for each possible combination of css files on the site.
This is wasteful of bandwidth, defeats the purpose of caching, and over the course of several page loads will end up taking more of the end-users time than either the selective css approach or the single cache file approach. The cache file will of course change from time-to-time, and this is fine, but this is not the same as the user re-downloading a slightly different version of the file many times over.

Either we go the selective css route, or the single cache file route in core - mixing them both is going to break the caching.
The only possible compromise I can see is the one I suggested above in comment 113 - where we collect a 'full' list of site css, and contrib modules can use this to create a single, static cache file.

robertDouglass’s picture

To me the final solution is going to have to include a combination of caching and grouping. If you have a section on your site that is responsible for 5 style sheets being added that aren't added anywhere else (admin sections, for example), it would make sense to aggregate those 5 separately and cache a stylesheet just for the admin section. The only way we'd ever hope to build something that can intelligently group CSS added by modules is by requiring modules to register their CSS and provide some sort of interface for the advanced-advanced user to alter the grouping. Then we'd end up with 2, 3, maybe four aggregated stylesheet caches that address different areas of a site. This is hugely complicated and out of the question for 5.0.

For 5.0, I'd like to see this patch, as it is. The current state with dozens of CSS includes in not acceptable. The clients who pay the salaries of many of the people who are the most active core developers will balk at such wasteful misuse of HTTP. On the other hand, it is those same clients who have built advanced Drupal sites with cutting edge CSS and they would equally balk at a return to one monolithic drupal.css. We got rid of that for a good reason.

My suggestion at this point is to adopt this patch as it is because it is an improvement over what we had in Drupal 4-7 and it is an improvement over what we have in 5.0-beta-2.

I don't think we should delay 5.0 any longer while searching for a perfect solution to this problem. We have an effective if imperfect solution right now which is optional, can be turned on or off at will, and will inspire people to experiment further and study the behavior of Drupal's CSS on real live sites.

@Dries, Steven, Drumm: please indicate what can be done to get this patch into core for this release. Do we need to drop the whitespace stripping? Do we need a better admin interface? More benchmarking? Do we need to perfect selective includes for all core CSS, only to then conclude that we still end up with pages that import a dozen CSS files?

BioALIEN’s picture

@ Dries (#114)
Having created a theme for Drupal 5.x I spent 99% of my time on the theme's style.css
However, I did find myself changing menu.css and system.css

I can override those within the theme's style.css directly but I didnt want to over complicate my theme.
While 17+ CSS files will scare people initially, in time they'll realise that its really only one file: style.css

@ robertDouglass
Before this patch is added, I'd like to see some of the functionality added from my screenshot regarding the ability to turn this on/off for aggregation, whitespacing and GZip.

dvessel’s picture

@Dries, I respect your restraint. Regarding the digg post: The numerous CSS files might put off some people but having a single drupal.css isn't pretty either. The previous way of assigning core styles hides the problem better but that's all it's doing. The separate styles and this patch looks like a good way to keep everything manageable and consistent. core, contrib module styles.

That's a big plus. Learn how it functions in general and expect related items to work the same way. I say that's a +1 to usability even if there's the illusion of being more difficult with the separate files.

If there's any chance of you changing your mind then disregard my previous post. Thought your decision was final. :p

I'll say no more.

m3avrck’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

I have created a new thread to track THE ISSUE at hand.

This thread has gotten out of control with comments and a lot of them are getting off topic now.

New threads/issues/patches should be created for: conditionally loading CSS files (which DOES NOT solve this problem entirely, only contributes), and interface changes, new options, and so forth.

Thanks.

sami_k’s picture

This solution is more elegant:
http://rakaz.nl/item/make_your_pages_load_faster_by_combining_and_compre...

It also lets you load the files conditionally... You could additionally incorporate and cache the resulting files in the db depending on path. It both deals with CSS and JS.

jacauc’s picture

There was mention of having a JS preprocessor too for drupal 6.
Has someone worked on this yet? Is there a patch available?
If so, what is the link to that issue.

Thanks!
jacauc

ardelio’s picture

We didn't get any answer to this question posted in the forum.

What are the best practices for cache/CSS optimization management in Drupal 5.x?

We keep normal cache enabled, CSS optimization enabled, and JavaScript aggregation disabled (as we have many issues with JS behavior when enabled). The problem is that the sites are visually broken every time we change something in single CSS files (and remove aggregated .css files from /files/css). The client requests the old aggregated CSS file whose name is included in the code of the cached page (local/proxy/Drupal cache?). As that file doesn't exist anymore on the server, the client loads the page without CSS. The issue seems to be very important for a multi-site Drupal setup. It seems that the whole /files/css folder is purged every time you enable a new module (internally calling cache/css clear) or when you change Site Configuration/Performance/Optimize CSS files option on any site.

Any thoughts about best practices and this particular "site visually broken" problem?

Thanks