| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | frontend performance, Performance |
Issue Summary
With all the talk of IEs 31 stylesheets, and CSS aggregation "on vs. off" I just had a bit of a brainwave.
The problem with having aggregation on is that you need to remember to manually clear the cache every time a file changes. While you can turn aggregation off for development (IE 31 limit issue notwithstanding) it is easy to forget to clear the cache when pushing theme changes (or module updates that don't require database changes) to a production site, and especially likely to flummox new users.
The filename for the aggregated file is generated from an md5 of the serialized array containing the filenames and inline/external code that was input to the aggregation.
It occurred to me that all we need to do is add the modification time for each file to it's array element and our problem will be solved, because the md5 will change any time a file mtime is updated. The attached patch does just this - apply the patch, enable aggregation and every single CSS or JS edit should be immediately functional. Why didn't we think of this sooner?! :)
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| css_js_mtime.patch | 1.27 KB | Idle | Passed on all environments. | View details | Re-test |
Comments
#1
forehead = sore from slapping it.
This is a great idea ... brilliant, simple AND brilliant.
+1
#2
Great, now all we need is aggregation on by default on this 31 stylesheet issue will be fixed!
#3
I like this idea, but I have to go -1. Here's the reasons why.
1. Performance. Just testing this on a vanilla D7 install, I compared the times from the current method to this method. As I suspected, doing a filemtime/stat on every css file for every request has a pretty decent performance hit. With 14 css files I saw the loop processing times jump from 0.06ms to 5.5ms. That will be felt on higher performance sites and sites with a lot of CSS files.
2. Page cache. The problem is if a new CSS file is generated automatically, with a new file name, the pages stored in page cache will still have the old CSS hash/file name linked in them. This will create more problems. If you are changing the CSS file, chances are you are logged in, so you won't see a cached version of the page. You quickly forget that your anonymous users will get the cached page, hence giving them the old CSS file.
My suggestion would be to change expand the "clear cache" options. Add checkboxes to it for exactly what should be cleared; each cache bucket, rebuild registries and menu router and then JS and CSS aggregated files (including a toggle all/none option - having it default to all). That way if someone is changing their CSS they could just clear the CSS cache out and the aggregated files will still have the same filename. Yes people will still have to manually clear the cache, but they won't be taking the performance hit of clearing the entire cache.
#4
For some reason I can't detect any measurable difference between these in ab benchmarks, doing -n2000 -c1 and taking the second pass results:
CSS/JS aggregation, no patch: 84.10 #/sec, 11.890ms mean
CSS/JS aggregation, with patch: 85.07 #/sec, 11.755ms mean
To double check, I did a benchmark of the specific functions (using separate php calls to avoid the stat cache and returning the microtime difference, and averaging this over 1000 calls). I also tested file_exists for reference (which we use all over the place).
mtime
0.00006409 seconds
file_exists
0.00006329 seconds
For 14 CSS files (as in the example above), this makes for a 0.9ms increase in the overall request time. If a site has 50 CSS and JS files (a not unreasonable number) this equates to 3.2ms, which is pushing it for something we can have enabled by default.
Unless anyone has any great ideas to make this perform I think the best bet is for someone to take this idea and do it in contrib, which should be quite possible as it is (you could just sum the mtimes, store a variable and if it changes clear the CSS/JS and page caches).
#5
@intoxication, can you expound on how you were testing? Also, I think some testing would need to be done to verify your second point in comment #3.
#6
I agree with #3, on both counts.
The performance impact is hard to measure, because it will vary significantly from system to system (how good is the OS's file system cache, is NFS being used, is the disk under heavy load). I think it's reasonable to assume that at least for some systems, stats will incur some performance hit that isn't desirable. And on a production site where CSS files aren't being changed, there's no benefit that makes the performance hit worthwhile. So at the very least, I'd want to see this have a setting so it can be turned off on production sites. This patch is most valuable during development, when CSS files are being changed, and while it nicely solves one of the main drawbacks of using aggregation during development, it doesn't solve all of them. You would still lose ability to know source file name and line number when debugging CSS with Firebug.
I think the page cache point is obvious. Just because you changed a source CSS file doesn't mean you've emptied pages from the page cache. So, when drupal_get_css() runs, it will generate a new md5 hash and therefore a new CSS file name for the aggregate file, because the timestamp of one of the source files have changed, but pages in the page cache will still be referring to the old aggregate file name. But this is true with all file changes. When you change a tpl.php file, the change doesn't auto-apply to pages in the page cache. So developers should already be used to the idea that changing any file needs to be followed with emptying the page cache. This is yet another reason why this patch is not so useful for sites in production (that have page cache turned on), but is useful for sites in development (where page cache is either not turned on, or developers know to empty it when updating any file).
So I think this patch does have a role: for sites in development where the developer doesn't need access to CSS source line number when debugging their CSS. Given that limited scope, I think it makes more sense as a contrib module (what an awesome showcase of why the new hook_css_alter() hook is so cool), though I'd be open to it being in core if it came with a setting to disable it.
#7
Sub. This could be a KILLER feature if we can get the gotchas worked out.
#8
+
#9
I see this is a solution for 7, but does anyone know if there is a working solution for version 6.15? There seems to be a lot of failed patches floating around for 6.
Thanks.
#10
There is no accepted solution yet ... once there is a solution it will be for D7 first. Only then might it be back-ported to D6.
#11
#12
Because of #3/#6 and because #228818: IE: Stylesheets ignored after 31 link/style tags is now RTBC, I don't see this as a bug, but a feature request. I'm wondering what folks would think of it being part of http://drupal.org/project/devel instead. My reasoning is based on #6: that the audience for this is developers, and that sites in production shouldn't normally use this (of course, nothing prevents a production site from having the devel module enabled and using this if that's where we put it), because if the site has page caching enabled, you need to empty the cache anyway when making a css change. And because of this, I think a setting on the admin/config/development/performance page would just confuse Drupal novices, but to me, a setting on the admin/config/development/devel page would make sense: just below the "Rebuild the theme registry on every page load" setting, we could have a "Rebuild CSS / JS aggregate files if any source files have changed" setting. Because this settings page is targeted to developers, it could even be more complex and feature rich (like also have an option to "Rebuild CSS and JS aggregate files on every page load").
If there's agreement to move this to the Devel project, I'll do what I can to help get it in there.
#13
Since no one objected to this being moved to Devel, let's give that a shot.
#14
Sorry, didn't see your suggestion to move to Devel until now.
Putting it into core gives the added benefit that we can enable aggregation by default, so I think it'd be good to see if we can get it into 7.x after all. If you still disagree, feel free to move it back.
#15
I agree with mcrittenden
#16
Yeah, that's a good argument.
We are striving to make Drupal 7 more usable "out of the box" than prior versions of Drupal have been, and part of what that means is matching default settings with best-practice knowledge, or at least, not worst-practice. And a site going live without aggregation just because a novice administrator didn't know about a setting is pretty bad: we should not have default settings be set to what we all know to be worst-practice.
But, we're also trying to not scare away new themers (or novice administrators who want to theme a little), and making aggregation that isn't sensitive to file changes the default was rejected by webchick as too likely to do that: #228818-260: IE: Stylesheets ignored after 31 link/style tags.
So, here's a patch with my recommendation: change the checkboxes to radios (disabled, enabled with file change sensitivity, enabled without file change sensitivity) and make the middle one the default. It's the one that's the most logical choice as a default. Developers wanting source file and line number information from Firebug can explicitly set to "disabled". People running high traffic sites where the extra file stats are a problem (#3.1) can set to "aggressive".
#17
The last submitted patch, aggregation-mtime-678292-16.patch, failed testing.
#18
Several tests assume aggregation is disabled. This patch makes those tests ensure that.
#19
Those file stat times look like a commonly known issue on Windows. Did you test on a Windows box by coincidence?
#20
In Drupal you generally cannot expect to be able to change module or theme (or core) files without clearing caches, rebuilding registries etc. When developing, this may be annoying, but it shouldn't be a problem on a production site. The problem is not specific to style sheets but to many different parts.
Making CSS/JS aggregation sensitive to files changes may be relevant during development (though it may be easier simply to turn off aggregation), but if we implement this, I don't think the UI for the setting belongs on the page where JS/CSS is enabled, but on a separate development page where all development-related caches may be disabled, including e.g. the "rebuild theme registry in every request" setting that I have seen implemented in some contrib themes.
#21
just linking to related discussion on turning CSS/JS aggregation on by default: #678160: Turn CSS aggregation on by default
#22
@#19: The numbers in #3 are probably not representative of a standard Linux production server, and with Linux file system caching, mtime is probably cheap enough to not worry about. But, we shouldn't assume a standard Linux file system. There's NFS, GlusterFS, and countless other server configurations. On some of them, file stats are noticeable. That's why APC provides a setting to turn them off, and so should we.
@#20: If you haven't already, please read #12-#16. The setting makes more sense to me as part of devel.module, but #14 makes a good point for it being in core, and core doesn't yet have a separate "development" settings page. Seems like overkill to create a new core settings page for this issue alone, but if there are other uses for it, then I like the idea.
#23
Issue Summary:
#24
Tagging.
#25
Thanks for that summary!
Please note that #769226: Optimize JS/CSS aggregation for front-end performance and DX will have a major impact on this discussion here. At least as far as JS is concerned, "enabled by default" will then mean that only always-loaded default JS files are aggregated, which normally shouldn't be altered in the first place.
#26
Env: D6.16 ,IE7, CSS and JS Optimize Enabled
otterbay.dksy.net
The home page does not display correctly in IE - OK in Firefox.
Is there an accepted solution yet ...
and is it back-ported to D6
or, since I would like to use Panels, is it time to migrate to D7?
Thanks
#27
@dekalaked: I'm not sure what the bug you're seeing is due to, but it's not related to this issue. Try to get some help in IRC or the support forums for pin-pointing the exact cause of the problem you're having, to find out whether it's a Drupal 6 core bug, and if so, whether it's been fixed in Drupal 7.
Please read the Drupal 7 Alpha 4 announcement. In short, it's still too soon to be using Drupal 7 for a "real" website. Also, there isn't even a semi-working alpha release of Panels for Drupal 7 yet.
Best of luck to you in troubleshooting and getting your website working!
#28
The issue was related to the combination of gallery and image modules that I had been experimenting with. When I started over with a minimal set of panel, view and image modules, the 2 column panel worked: http://otterbaycottage.dksy.net/
#29
Rerolling #18. There have been some changes to drupal_aggregate_css and drupal_get_js - we still need to test that the mtimes are still getting added and operating as they should.
#30
The last submitted patch, aggregation-mtime-678292-28.patch, failed testing.
#31
#29: aggregation-mtime-678292-28.patch queued for re-testing.
Note: I can install locally with this patch, wondering if this is a broken test bot...
#32
The last submitted patch, aggregation-mtime-678292-28.patch, failed testing.
#33
Try again testbot...
#34
The last submitted patch, aggregation-mtime-678292-33.patch, failed testing.
#35
Something odd is going on here - I can install Drupal just fine with this patch, both via the web interface and via drush site-install...trying a patch with just the tests.
#36
The last submitted patch, aggregation-mtime-678292-35.patch, failed testing.
#37
+++ includes/common.inc 28 Jul 2010 10:23:37 -0000@@ -152,6 +152,34 @@
/**
+ * Constants defining aggregation of JavaScript and CSS files.
+ */
It would be great to append these constants below the already existing JS/CSS constants, instead of just at the end.
+++ includes/common.inc 28 Jul 2010 10:23:37 -0000
@@ -2937,8 +2965,7 @@
- $preprocess_css = (variable_get('preprocess_css', FALSE) && (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update'));
-
+ $preprocess_css = (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') ? variable_get('preprocess_css', DRUPAL_AGGREGATION_ENABLED) : 0;
@@ -3770,7 +3810,7 @@
- $preprocess_js = (variable_get('preprocess_js', FALSE) && (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update'));
+ $preprocess_js = (!defined('MAINTENANCE_MODE') || MAINTENANCE_MODE != 'update') ? variable_get('preprocess_js', DRUPAL_AGGREGATION_ENABLED) : 0;
I'm pretty sure that you need to drop the MAINTENANCE_MODE exception, as this kind of exception actually states that it should try to look up a variable, and on top of that, default to enabled -- during installation.
The previous code worked, because it defaulted to FALSE.
+++ modules/profile/profile.test 28 Jul 2010 10:23:37 -0000@@ -339,7 +339,12 @@
+ // disable JavaScript aggregation for this request. ¶
Trailing white-space.
+++ modules/system/system.admin.inc 28 Jul 2010 10:23:38 -0000@@ -1680,15 +1680,25 @@
+ DRUPAL_AGGREGATION_DISABLED => t('Disabled. This can be useful during development, but slows down page download and display.'),
+ DRUPAL_AGGREGATION_ENABLED => t('Normal (recommended)'),
+ DRUPAL_AGGREGATION_ENABLED_WITHOUT_STAT => t('Aggressive. This setting skips the normal verification of changes to source JavaScript files, potentially reducing disk load on high traffic website.'),
When introducing new constants for this, we absolutely have to make sure that the #options can be extended by contrib. For example, with a DRUPAL_AGGREGATION_ENABLED_PLUS_MINIFICATION or similar.
For that sake, I wonder whether it would make sense to use bit flags (like the menu system constants).
Powered by Dreditor.
#38
contains everything but the bit flag suggestion
#39
The last submitted patch, drupal.aggregation-mtime.38.patch, failed testing.
#40
I moved that condition to a separate line as I think it deserves a comment, which also fixed the syntax. I agree that the new logic looks correct - previously it was relying on an implicit FALSE for install (because it defaults to false) which is no longer the case, hence we can now simply force it to FALSE anytime maintenance mode is set.
#41
The last submitted patch, aggregation-mtime-678292-40.patch, failed testing.
#42
Should fix color and theme tests - we just disable aggregation for these tests, since they just need to confirm that files are being added to the page.
#43
I have done some benchmarks to show the effect of enabling aggregation. I used a typical production Drupal 6 site since the benefits and method of aggregation are basically unchanged in Drupal 7 from a browser point of view, and the number of requests and request size is much more typical than core alone. I did do some tests on core also, and could reproduce some similar values when many modules/blocks are enabled.
In summary, aggregation saves anything from 0-0.5 seconds on initial page view, and 1.5+ seconds on subsequent page views (when mod_expires is disabled and 304 checks are needed). The zero gains on the first test with initial page view I think can be put down to the relatively slow download speed - the download time allowed the browser to compensate for the latency in a way that it couldn't on subsequent 304 requests. Either way, these numbers are plenty significant, given that a single second extra latency can lead to several % decrease in many site metrics.
#44
#42: aggregation-mtime-678292-42.patch queued for re-testing.
#45
The last submitted patch, aggregation-mtime-678292-42.patch, failed testing.
#46
FWIW, this specific problem raised by the OP (developing / testing on IE) can be solved by this module:
http://drupal.org/project/ie_css_optimizer
#47
#48
Completely untested patch reroll - almost certainly needs some more work for tests to pass, but at least it should now apply.
#49
@grendzy FWIW - this is completely unrelated to the IE problem (other than some people suggesting this as a possible solution), since that issue is already fixed in D7. Rather the point of this issue is to make core mush much faster by default (see the stats #43 above), especially for the mass of non-expert users who have little idea what "aggregation" means, or why they should care - whilst retaining "hack-ability" for newbies, easing deployment for low-traffic sites, and improved DX for developers and themers who want to keep their sandbox closer to a production environment.
#50
+++ b/modules/system/system.admin.inc@@ -1696,15 +1696,25 @@ function system_performance_settings() {
+ DRUPAL_AGGREGATION_ENABLED => t('Normal (recommended)'),
+ DRUPAL_AGGREGATION_ENABLED_WITHOUT_STAT => t('Aggressive. This setting skips the normal verification of changes to source CSS files, potentially reducing disk load on high traffic website.'),
Typo in the last sentence ("high traffic websiteS").
I am generally against adding something like this to the UI, but if that has been decided, can we please not name it "Aggressive"?
1. Aggressive generally has the connotation of having malicious side-effects, which this doesn't have at all.
2. Lots of people will think this is the re-introduction of Drupal 6's aggressive page cache, which actually did have some malicious side effects, depending on the modules you were running.
In general, I think the distinction between what is now "Normal" and "Aggressive" could be clarified IMO.
#51
Yes - plenty of work still to do here, not least fixing all the tests! In understand the resistance to adding UI here - we do need some way to enable the "non-stat" mode however - we could go with a settings.php only variable, but that seems like it has it's own problems. Alternatives welcome!