After reading this:

http://www.facebook.com/note.php?note_id=307069903919

It made me realize that the Drupal core behavior of aggregating libraries jQuery (and in D7 jQuery UI) together with all the other JS is potentially a significant performance suck for Drupal core. The reason - if I change one byte of JS that's in the aggergated JS (or add a new query string), all users have to re-download jQuery and jQuery UI again. Further if my home page and inner page have slightly different JS aggregated together, I end up downloading the libraries multiple times in aggregated form.

I propose that JS added as a library should (at least by default) never be aggregated and not get the query string added. We must, though, put the library version number in the file name.

Current status

The original issue has already been sorted, and the current patch merely changes variable names for consistency with core and sets the default stale file threshold to three days.

CommentFileSizeAuthor
#213 721400-variable-names-213.patch4.46 KBmcjim
#211 721400-variable-names-211.patch3.06 KBmcjim
#210 721400-variable-names-210.patch2.91 KBdstol
#191 721400-variable-names-158-retest.patch2.99 KBbleen
#189 721400-variable-names-158-retest.txt2.99 KBtom_o_t
#158 721400-variable-names-158.patch2.99 KBpwolanin
#156 varnames.patch2.69 KBbleen
#143 721400-qs-fu-143.patch1.14 KBpwolanin
#137 721400-delete-if-stale-137-D6.patch1.74 KBpwolanin
#130 721400-hash-contents-130.patch8.43 KBpwolanin
#126 721400-hash-contents-126.patch7.88 KBpwolanin
#123 721400-hash-contents-123.patch7.68 KBpwolanin
#121 721400-hash-contents-121.patch7.29 KBpwolanin
#119 721400-hash-contents-119.patch6.88 KBpwolanin
#114 721400-hash-contents-114.patch6.06 KBpwolanin
#97 721400-hash-contents-stacked-723802-96.patch59.67 KBpwolanin
#77 drupal-n721400-d6_skip_css_js_query_string.patch1.76 KBDamienMcKenna
#72 aggregate_reverse_proxy_0.patch2.74 KBOwen Barton
#67 721400-js-css-filenames-67-D6.patch2.77 KBpwolanin
#59 721400-css-js-aggregation-fixes-59.patch10.08 KBpwolanin
#52 721400-css-js-aggregation-fixes-52.patch9.55 KBpwolanin
#48 721400-css-js-aggregation-fixes-48.patch9.77 KBpwolanin
#34 721400-no-aggregate-jquery-34.patch10.06 KBpwolanin
#33 721400-no-aggregate-jquery-33.patch10.06 KBpwolanin
#23 721400-no-aggregate-jquery-24.patch10.4 KBpwolanin
#22 721400-no-aggregate-jquery-22.patch9.77 KBpwolanin
#20 721400-no-aggregate-jquery-20.patch9.61 KBpwolanin
#12 721400-no-aggregate-libraries-12.patch10.66 KBpwolanin
#7 721400-no-aggregate-libraries-7.patch8.22 KBpwolanin
#6 721400-no-aggregate-libraries-6.patch7.77 KBpwolanin
#5 721400-no-aggregate-libraries-5.patch5.92 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bleen’s picture

Category: bug » feature

Assuming you agree with the premise of this issue (which I'm not sure I do yet...maybe) wouldn't it be better to aggregate jQuery & jQueryUI (and other js added as a library) into one aggregated file and then all the contrib/theme js into a second aggregated file ...

catch’s picture

Also if I read that article right, any inline js which only depends on jquery as opposed to other files which are aggregated would be able to run instantly if jquery itself is cached by the browser. So it's not just the extra downloads, it'd also be rendering performance.

i don't think we add Jquery UI on every single page, at least I hope not, so that should be either aggregated with the code that uses it, or also not aggregated, but not lumped with jquery IMO.

pwolanin’s picture

Category: feature » bug

Bad performance is a bug.

pwolanin’s picture

I'll try to roll a patch tonight - I think the simplest approach is to make any library JS non-aggregated.

pwolanin’s picture

Status: Active » Needs review
FileSize
5.92 KB

Here's a fist pass - tries to use the information about version included in hook_library().

pwolanin’s picture

This patch is a little better - but it still kills the overlay.

Also - I notice the JS and CSS aggregation logic works differently for no readily apparent reason.

pwolanin’s picture

ok, we avoid killing the overlay by making drupal.js also type 'library'

Status: Needs review » Needs work

The last submitted patch, 721400-no-aggregate-libraries-7.patch, failed testing.

catch’s picture

Approach looks solid to me. We originally added the query string at the very end of the D6 cycle because that was the first time we'd upgraded a jQuery version in core and people were getting cached javascript on update.php. However that was a very aggressive approach and we knew that at the time. Appending the version string to the file name fixes the same root cause, without needlessly causing refreshes.

With #attached_js and #attached_css, there's going to be a lot less of modules adding their stuff in hook_init() like in D6 - i.e. you have to do that for blocks, for example and more loading as and when things are needed.

cbrookins’s picture

Subscribe

pwolanin’s picture

Thinking more about this - why do we alter the last letter of the aggregated file names? This makes no sense. The file name will change if the content changes. The current system just needlessly inhibits the desired caching.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.66 KB

Ok - I see now, we don't actually MD5 the contents, just the info array contents when constructing the file name. Still, I think we can prevent failed requests via cached pages by using a query string for the aggregated files too, rather than changing their names.

pwolanin’s picture

Title: Do not aggregate and do not append query string to library JS » Do not aggregate and do not append random query string to library JS

fix title

bleen’s picture

just a thought ... (and I recognize the extreme rareness of this) but without the query string, what happens if there is a patch to Drupal core that includes a new version of jQuery (lets say their was a security release for jq for the sake of argument)... in that case wont we need the query string to ensure proper cache busting?

(I also realize that JQ might be a bad example because there will likely be a new file name in that case, but I think my question is clear)

pwolanin’s picture

Did you try the patch? It does add a query string which is the jQuery version as returned from hook_library() - for exactly the reason you suggest. What I want to avoid is the aggregation and/or random query string that force re-downloading an unchanged jQuery library too often.

kkaefer’s picture

  • Why do you treat drupal.js and collapse.js as a library? Do we have a clear idea of what a library is? When we treat all core JS files as libraries, we’ll still end up with loads of requests.
  • Why do you now use 2 characters as query string?
  • Why are you appending the default query string to preprocessed JS files? I don’t think this is necessary since
pwolanin’s picture

drupal.js is loaded on every page where we have JS and is required for the other files to function. In that sense it's a library. It would possibly be preferable to add a version query string instead of the random query string, but that's less important since it's not a huge file.

I switched to the two character query string to increase the cycle time - in particular since it's being added to the preprocessed JS file instead of altering the filename of the preprocessed file. It's absolutely necessary to add the query string to the preprocessed file since the filename hash is only based on things like file name that do not change if the contents change.

RobLoach’s picture

I like the idea of using the version number of the library instead of a random character for the query string. Just want to be careful not to make a crazy amount of HTTP requests. jquery.js, drupal.js and jquery.once.js could be aggregated together just fine as they are added to every page with JavaScript.

pwolanin’s picture

Status: Needs review » Needs work

note - there is a conflict due to recent commits (no PIFR retesting?). I will try to reroll soon.

Also - note that Drupal 7 has instructions in .htaccess to set 2 week expire headers for all static content if possible:

# Requires mod_expires to be enabled.
<IfModule mod_expires.c>
  # Enable expirations.
  ExpiresActive On

  # Cache all files for 2 weeks after access (A).
  ExpiresDefault A1209600

  <FilesMatch \.php$>
    # Do not allow PHP scripts to be cached unless they explicitly send cache
    # headers themselves. Otherwise all scripts would have to overwrite the
    # headers set by mod_expires if they want another caching behavior. This may
    # fail if an error occurs early in the bootstrap process, and it may cause
    # problems if a non-Drupal PHP file is installed in a subdirectory.
    ExpiresActive Off
  </FilesMatch>
</IfModule>
pwolanin’s picture

Title: Do not aggregate and do not append random query string to library JS » Do not aggregate and do not append random query string to jquery.js and other always-loaded files
Status: Needs work » Needs review
FileSize
9.61 KB

Narrowing the scope of the patch here, since Rob pointed out that it's not really a clear win to not aggregate each and every library-type JS file.

pwolanin’s picture

note - WP already uses this method to control the refreshing of JS and CSS - e.g.:

 <link rel='stylesheet' id='wp-email-css'  href='http://www.theworld.org/wp-content/plugins/wp-email/email-css.css?ver=2.50' type='text/css' media='all' />
<script type='text/javascript' src='http://www.theworld.org/wp-includes/js/jquery/jquery.js?ver=1.3.2'></script>
pwolanin’s picture

We ought to add a version for drupal.js too - note that this number will need to be bumped whenever it's changed, so it will not be the same as the core version.

pwolanin’s picture

Oops - had an error of re-using a variable name in a loop that was also a function param.

mfer’s picture

The performance issue with the way we do things has been known about for some time. IMHO, with jquery ui added to the mix more often this issue will become more noticeable in D7. Trying to come up with better caching strategies is where modules like sf cache came from.

Dealing with this issue came up with earlier in the D7 dev cycle but a better default solution for Drupal could not be agreed on. I had hoped to make the preprocessing pluggable but that issue was pushed off to D8.

All that background context being said here are some specifics to the current patch...

+++ includes/common.inc
@@ -3615,8 +3614,10 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
+  // The index counter is used to keep aggregated and non-aggregated files in
+  // order by weight.

This is a change in the expected behavior. Drupal expects to have teh preprocessed files before the non-preprocessed files at all times. Is this change in expected behavior allowed at this point?

+++ includes/common.inc
@@ -3648,6 +3649,12 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
+    if (empty($item['version'])) {
+      $query_string = $default_query_string;
+    }
+    else {
+      $query_string = '?v=' . $item['version'];
+    }

This could be simplified with:

$query_string = empty($item['version']) ? $default_query_string : '?v=' . $item['version'];

Should the jQuery UI library elements be preprocessed? This is the area that could cause some bloat in preprocessed files.

A nice side effect of the changes here would be that jQuery could easily be swapped out for its version from Google or Microsofts CDN.

This is just a first pass at the patch. I didn't have time to install and test it. I wanted to get something up first.

139 critical left. Go review some!

pwolanin’s picture

We must change the aggregation/ordering behavior to make any progress on this since we need jquery.js and drupal.js to come before the aggregated JS.

sun’s picture

How do we handle JavaScript file weights in here? Does it really make sense to special-case jquery.js + drupal.js only? Any benchmarks?

Don't we have all necessary capabilities in place to allow this optimization from contrib?

IMHO, the proper solution is to build "bundles" of page requisites, like http://drupal.org/project/sf_cache does.

That said, I also like the idea of appending the version number - as long as the added files are derived from hook_library(). However, when doing that, there also needs a way to disable this functionality.

In the end, this rather sounds like D8 material for me.

pwolanin’s picture

@sun - look at the code. There is no special casing at all. They are just marked as non-aggregated. Instead the code adapts from what we use (or used) for CSS aggregation so that we can fully respect the specified weight.

I agree that sf_cache is a more "ideal" solution for people knowledgeable enough to tune those knobs - but this is frankly a pretty trivial change to improve Drupal performance out of the box.

mfer’s picture

I agree with @sun that grouping is the most ideal solution. Having a default grouping for core js would be a place to start. jquery.js, drupal.js, and jquery.once.js could be in one bundle.

@pwolanin do you have any benchmarks?

Typically, the slowest part of loading JavaScript is the blocking in some browsers. That is when one js file is being downloaded the other js files are not acted upon. Most modern browsers do not have this problem but some of the browsers drupal (or more directly, the people who build sites with Drupal) supports do.

So, by moving to 3 non-preprocessed files there is a performance loss when those are loaded the first time. On sites with many returning visitors that is ok. They take the hit on the first page request and then don't later. But, site with a high percentage of new visitors the performance hit for blocking would show up often.

So, we would have to make some assumptions about the default Drupal usage in deciding this issue.

pwolanin’s picture

I was trying to avoid the much more substantial changes needed to do grouping. An alternative variant of this patch would just leave jquery out of aggregation.

mfer’s picture

If we leave jQuery out of preprocessing and order it correctly site builders can use jquery from Google or Microsofts CDN. This is a bonus.

One question is, how do we handle it when someone weights a non-jquery.js external file in the middle of preprocessed files? Does this break up the preprocessed grouping into two of them? is jquery.js special cased? If so, what if someone wants to include prototype or dojo? Is there a way to special case them as well?

I don't mean to be a pain or to even take credit for these questions. They are the questions or type of question asked every time this issue comes up.

sun’s picture

Version: 7.x-dev » 8.x-dev

I can't help, but this change bears too many questions to qualify as performance quick-fix, sorry. Additionally, there are already contributed modules that easily allow you to do this and more. We need a discussion about what's a library and what's not, a discussion about proper grouping (the same applies to CSS files) that depends on the library discussion, a discussion about proper query strings, depending on the result of the library and grouping discussions. Ideally, we should try to prepare sf_cache or another module in a joint effort, to move its base functionality with sane defaults into core for D8, while the contrib module can stay for advanced users to tweak the defaults when needed.

pwolanin’s picture

Version: 8.x-dev » 7.x-dev

I disagree - this code is broken and needs to be fixed in core to at least behave reasonably. The proposed patch is pretty minor code change, and really affects no APIs.

pwolanin’s picture

Re-roll for: http://drupal.org/node/243251 and to respect weighting of external JS per mfer's suggestion that we need to be able to load something like jquery.js from a CDN. Only jquery.js is excluded by default from aggregation in this patch, not the other 2.

The changes I'm making actually make the behavior of the code more consistent with the doxygen for drupal_add_js()

pwolanin’s picture

Status: Needs review » Needs work

The last submitted patch, 721400-no-aggregate-jquery-34.patch, failed testing.

pwolanin’s picture

Looks like testbot or HEAD is broken?

pwolanin’s picture

Status: Needs work » Needs review
bgm’s picture

subscribing

This is, imho, an important performance issue which has often caused me problems in D6. Redownloading a big CSS or JS file, just because of a small change in it (between two pages who include different CSS/js files), can be a major annoyance for users.

pwolanin’s picture

Title: Do not aggregate and do not append random query string to jquery.js and other always-loaded files » Do not aggregate and do not append random query string to jquery.js, don't chnage filenames for aggregated

fixing title.

Note that changing the filenames of aggregated files is a real failure if you have a page cache or external (http) cache that may serve a page that references CSS and JS files that have magically disappeared in the background.

pwolanin’s picture

Title: Do not aggregate and do not append random query string to jquery.js, don't chnage filenames for aggregated » Do not aggregate or append random query string to jquery.js, don't change filenames for aggregated JS/CSS
catch’s picture

I've seen problems with filename changes before, serving a 404 is a lot worse than serving a stale file. Also just because sfcache offers a more finely tuned contrib solution doesn't mean we shouldn't try to improve what core does, and the CDN trick is a nice touch. Don't have time to do a review of the latest patch at the moment but previous ones seemed pretty sane.

pwolanin’s picture

David Strauss commented on this linked issue: http://drupal.org/node/243251#comment-2680098 that the query string should not interfere with mod_deflate if it's properly configured.

mfer’s picture

Status: Needs review » Needs work

How you aggregate js files depends on project context. 2 examples to illistrate.

A blog with over 80% of its traffic coming from new people is common. The typical js bundle would include the js for commenting and maybe a few interface things. One bundle that includes jQuery could be very good here because of the blocking issue that many browsers have.

By blocking I mean that on these sites, if they had 2 js files, the browser would request the first file, it would transfer it back, and then execute it. Then it would request the second file, transfer that, and execute it. The time spent talking on the network to manage two files would be longer than the time to download one larger file.

For caching to be of a benefit you need to assume characteristics about the usage of the site.

A site that people regularly return to that has different JS file combinations on different pages could benefit from some bundling/separation. I suspect gardens falls into this. There would be additional blocking but the caching / grouping strategies would work best if they were tailored to the site.

IMHO the optimal solution would be a pluggable preprocess system. Drupal would provide a good default. Then site builders could swap that out for something tailored to their context. This isn't going to happen before D8.

The problem of this being a performance burden in D7 is going to need a little justification and testing. What site context is it a performance burden to based on what style of site?

mbutcher’s picture

mfer is 100% correct.

The only right way of doing this right it to modify the drupal_*_js code to have a pluggable backend. The standard medium-sized site can certainly get away with standard aggregation. Meanwhile, those of us who have special requirements can build more sophisticated solutions (which don't clutter core with tuning for out-of-the-norm usage patterns).

Sun rightly pointed to a way that is much more appropriate for large sites that may deploy certain chunks on 90%+ of the site, while other scripts only get served on a small fraction of pages. Most of us want actual control over how those chunks get composed ('cuz we actually know which files are being requested at what frequency). A layer that purports to give us "what we want" while actually not giving much control at all is just frustrating.

Further, I'd suggest that if this is all done "in the name of optimization," somebody should supply some comparative numbers -- and base them on actual use cases, rather than assumptions about how JavaScript files *might* be distributed. Nothing speaks louder than numbers when talking about performance.

We did a huge amount of testing on our site, and clearly arrived at the conclusion that a single minified JS file was more efficient by a long shot. We went from four JS files down to one and effectively cut off 200+ msec of download time off of the average page load, and (of course) freed up web server resources. I strongly suspect that other use cases will be different, depending on how the JS is distributed over the site -- and that fact on its own implies that a single built-in solution is going to be sub-optimal.

Thus, if we are really talking about optimization, solution #1 should be to allow people to determine what they want to optimize and give them the freedom to write code to do that. Look at how that played out with caching, and you will see what appears to be a huge success: there are at least half-a-dozen successful cache modules out there, and probably countless custom ones.

EvanDonovan’s picture

mbutcher makes a good point on the performance optimization being different for different sites. On my D6 site, I hate how many different css & js aggregated files there are, but we have many (probably too many) modules enabled.

I think that the best thing to do, since the performance issue is debateable, would be to put that in a different patch, and possibly push that to D8 (and Pressflow?).

But I think that keeping the aggregated filenames the same and changing on the basis of a query string is necessary for reverse proxy support. So that part is a bug fix and should definitely go in D7. I didn't actually realize at first that both those things were happening in this patch, because I was confused by the discussion and the fact that the API for adding JS has changed a lot from what I was familiar with in D6.

pwolanin’s picture

Discussing this in IRC with mfer - the block on getting/processing JS files is an issue in older IE (IE6/7).

Much of the reasoning about aggregating jquery.js or not by default depends on the assumed visitor profile and behavior. If they visit 2 pages with different JS, the 2nd page will be faster if jquery is excluded. If they are using IE6, the 1st page may be slower. In FF, etc, possibly all pages will be faster.

I think this is still pretty much rtbc - but agreed with mfer to split the 1 line de-aggregating jquery by default from the rest of this patch which is pretty much a pure bug fix.

pwolanin’s picture

@mbutcher - a pluggable backend isn't' happening for D7, but maybe you can outline what behaviors/files you cannot change as desired using the existing D7 _alter hooks?

pwolanin’s picture

Title: Do not aggregate or append random query string to jquery.js, don't change filenames for aggregated JS/CSS » Order JS files according to weight, don't change filenames for aggregated JS/CSS
Status: Needs work » Needs review
FileSize
9.77 KB

Here's the patch with the one line changing jquery aggregation removed - it does 4 things:

  1. Fixes rendering of JS according to weight (the code is based on an how CSS was rendered in D7 before the 31 stylesheet really re-arranged that code).
  2. Allows JS files to have a 'version' element specified. That's used to make a query string instead of the random query string
  3. Assigns for all JS files originating in hook_library the 'version' element based on the library 'version' in the parent array.
  4. Appends the random query string to aggregated JS and CSS files instead of changing the filename each time they are re-aggregated.
pwolanin’s picture

Follow-up issue for de-aggregating jquery: #734080: By default, don't aggregate jquery.js

sun’s picture

Status: Needs review » Needs work
+++ includes/common.inc
@@ -2982,7 +2976,7 @@ function drupal_pre_render_styles($elements) {
-  $query_string = substr(variable_get('css_js_query_string', '0'), 0, 1);
+  $query_string = substr(variable_get('css_js_query_string', '0'), 0, 2);

@@ -3630,7 +3628,7 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
-  $query_string = substr(variable_get('css_js_query_string', '0'), 0, 1);
+  $default_query_string = substr(variable_get('css_js_query_string', '0'), 0, 2);

If there is a technical reason for why we change this from 1 to 2 query string characters, then we are missing important documentation here.

If there is none, this change needs to be reverted.

+++ includes/common.inc
@@ -3477,6 +3468,10 @@ function drupal_region_class($region) {
+ *       If not empty, the version is added as a query string insead of the

Typo in "insead"

+++ includes/common.inc
@@ -3677,11 +3677,15 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
           $query_string_separator = (strpos($item['data'], '?') !== FALSE) ? '&' : '?';
-          $js_element['#attributes']['src'] = file_create_url($item['data']) . $query_string_separator . ($item['cache'] ? $query_string : REQUEST_TIME);
...
+          $js_element['#attributes']['src'] = file_create_url($item['data']) . $query_string_separator . ($item['cache'] ? $query_string : '?' . REQUEST_TIME);

$query_string_separator already holds "?" or "&", so the additional "?" can only be wrong.

If this patch passed the tests, then we need tests for this functionality.

+++ includes/common.inc
@@ -3692,7 +3696,7 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
+        $processed[$index++] = theme('html_tag', array('element' => $js_element));

@@ -3701,16 +3705,18 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
+      $processed[$key] = theme('html_tag', array('element' => $js_element)) . "\n";

Why do we append a "\n" to one but not the other processed element?

+++ includes/common.inc
@@ -3937,21 +3943,27 @@ function drupal_add_library($module, $name) {
-  if (!array_key_exists($module, $libraries)) {
+  if (!isset($libraries[$module])) {

This is not the same. It means that any call to drupal_get_library() will pollute and bloat the statically cached $libraries, regardless of whether $module exists or not.

+++ modules/system/system.module
@@ -1074,7 +1074,7 @@ function system_library() {
-      'misc/jquery.ba-bbq.js',
+      'misc/jquery.ba-bbq.js' => array(),

API change?

148 critical left. Go review some!

pwolanin’s picture

Currently we can potentially cycle back to the same 1-char query string after ~20 times the cache is cleared and potentially some users may return and their browsers fail to refresh the files. Increasing to 2 characters greatly increases this to > 400.

re:

+  if (!isset($libraries[$module])) {

I know they are not the same - see the other changes in that function which always sets the value to at least an empty array instead of allowing it to sometimes be NULL.

+      'misc/jquery.ba-bbq.js' => array(),

The change simply fixes that line which was incorrect (see all the other entries in that function).

The other points suggest valid corrections that are neede.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
9.55 KB

I think this corrects the problems.

Status: Needs review » Needs work
Issue tags: -Perfomance, -frontend performance

The last submitted patch, 721400-css-js-aggregation-fixes-52.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: +Perfomance, +frontend performance
pwolanin’s picture

The failure is in the contact module test - unrelated to the patch.

mfer’s picture

Status: Needs review » Needs work

A summary of what I see the patch doing:

  1. Changing the names of the preprocessed files. They used to incorporate the "query string" as part of the md5 hash that generated the name. So, every time the cache was cleared a new file name was generated. The forced the browser to download the new file when there was a change. But, it was not good for reverse proxies. The name change appends the "query string" to the end of the url as an argument. This will cause the file to be downloaded on an update as well but is friendlier to reverse proxies.
  2. Changes the ordering for JS file. Currently, the ordering is preproccessed files, settings, then external / inline files. This change keeps the weighted ordering even with external and non-preprocessed files. If there is a non-preprocessed file or external file weighted between the preprocessed files there will be multiple preprocessed files.

With regard to #1, we need to make an assumption here. That the js file we loading does not pull an argument from the url with the same name as the argument we are using to handle force the file to reload. If we purse this we need to allow the variable name to be swappable in case a site has problem.

With regard to #2, why are we trying to preserve the weighted order between preprocessed and non-preprocessed files? The current expected functionality is that weight is preserved within the groupings. Having multiple preprocessed files will have a negative performance impact for many browsers because of blocking. Can you, provide, a justification for this change?

With the exception of the variable change to #1 I am fine with the code. Its the justification for #2 I am still unclear on.

pwolanin’s picture

Re: #2 - this change makes the behavior of the code match the documented behavior in the code comments - so I consider it a bug fix.

Also for #2 - Drupal 7 is supposed to support delivering files from a CDN or separate domain. Without this change, I can't ever deliver a file like jquery.js (or any other JS file that needs to come at the start) from an external URL.

mfer’s picture

For a little background on #2 from comment #56.

  • The current ordering that this patch changes has been in core since preprocessing went in for D5.
  • file_create_url is used so preprocessed and non-preprocessed files can be hosted off a CDN in the current functionality.

The proposed change allows for a non-preprocessed file or external file to be weighted differently. This would be useful for pulling jquery.js from a CDN and still keeping it positioned correctly.

The ordering @pwolanin proposes could be generated in contrib using a ModuleName_process_html callback. And this piece of functionality is something I could see as useful.

But, is it a feature change that is allowed at this late stage of the game?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.08 KB

Here's the change suggested by mfer - he clarified in IRC " a js file can grab the arguments (get style) from the url and act on them."

Thus we can't assume that every JS file/library in existence will gracefully accept ?v=X in its URL and we should allow a way for a site to change that behavior to some other query parameter that won't conflict.

mfer’s picture

Status: Needs review » Reviewed & tested by the community

The code is RTBC. Whether it should go into D7 is another matter. :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

Great! I think at least the use of query string versus changing filenames should be backported to 6.x too.

RobLoach’s picture

Status: Patch (to be ported) » Closed (works as designed)
sun’s picture

Version: 6.x-dev » 7.x-dev
Status: Closed (works as designed) » Fixed
pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

The small change I'm suggesting should be in 6.x core - it's a bug as it is now.

mfer’s picture

Version: 6.x-dev » 7.x-dev
Category: bug » feature
Status: Patch (to be ported) » Fixed

@pwolanin - making this change in D6 will affect sites and module that expect/depend the current behavior in D6. A behavior change like this should not happen in core. People will not just be able to update their sites. Some will have to change code. For people who are just applying updates to keep up with security this could be a problem.

This is a features change and not a bug since the bug could not be clearly articulated and shown (with data to support it).

pwolanin’s picture

Title: Order JS files according to weight, don't change filenames for aggregated JS/CSS » D6: Don't change filenames for aggregated JS/CSS
Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs review
FileSize
2.77 KB

@mfer - I don't think you understand what I'm suggesting for D6. Here's a patch. All I want is to not have filenames change for no good reason.

EvanDonovan’s picture

I like this - didn't realize it would be so easy to fix. Will consider testing this on a Pressflow Drupal 6.16 install in the next few days.

philbar’s picture

Issue tags: -Perfomance +Performance
RobLoach’s picture

I created a follow up to the code that was just committed: #746676: New grouping messes with JavaScript weight

c960657’s picture

Status: Needs review » Needs work

The logic WRT css_js_query_string was changed today in #751002: _drupal_flush_css_js() is too complex. The D6 patch should either incorporate this change or omit the substr+variable_get('css_js_query_string') change completely. I prefer the former.

Owen Barton’s picture

It is not clear anywhere in this ticket what the actual reasoning is for changing the querystring rather than the filename. If you change the querystring, the browser will redownload the entire file, just as it would if you change the filename. Modern reverse proxies (e.g. Varnish or Squid >3.0) will happily cache the response and invalidate the cache correctly in either case also. Is there some benefit I am missing here?

In addition many proxies (of the ISP/corporate kind, not the reverse kind) will not cache any requests with a querystring in the URL, hence this change appears to actually reduce performance for these users.

Attached is a patch that reverses this specific change (but leaves all the other good stuff, such as JS versioning and the CSS order fixes), but I would like to hear back a bit more about the original reasoning before putting this on the table, so to speak.

Owen Barton’s picture

Title: D6: Don't change filenames for aggregated JS/CSS » Don't change filenames for aggregated JS/CSS
Version: 6.x-dev » 7.x-dev
pwolanin’s picture

Version: 7.x-dev » 6.x-dev

@Owen - see #56. By changing the file name you can often get a 404 for CSS or JS (leaving a very ugly site) when your site is behind a reverse proxy.

yusufg’s picture

@pwolanin. I am a Drupal newbie but isn't it possible to canoncalize (ie, rewrite) all incoming Drupal URL's to remove the versioning from the URL and thus eliminate the 404

I agree with @Owen that query strings should be avoided, Steve Souders of Google and of the main drivers for performance on the front end makes a good case against them for it

http://www.stevesouders.com/blog/2008/08/23/revving-filenames-dont-use-q...

Some other blog posts on doing static asset versioning

http://muffinresearch.co.uk/archives/2008/04/08/automatic-asset-versioni...
http://www.ejeliot.com/blog/125

pwolanin’s picture

@yusufg - not really, These files are served by the webserver. It would be a serious performance issue to serve them via PHP.

DamienMcKenna’s picture

How's about this option - a new variable called "skip_css_js_query_string" which lets you skip the query string addition entirely (defaults to FALSE meaning the query string is added)? A corresponding patch has been supplied for the Zen theme (#772234: Optionally disable using css_js_query_string).

DamienMcKenna’s picture

Status: Needs work » Needs review
pwolanin’s picture

Status: Needs review » Needs work

@DamienMcKenna - sounds like a feature request for Drupal 7

DamienMcKenna’s picture

@pwolanin: I'm going to grok D7 to take a look, at an initial glance the code works differently.. I'll work on it.

pwolanin’s picture

@yusufg - those posts are a bit old. Varnish does not exhibit that behavior, when tested against Drupal 7:

$ curl -i http://example.com/sites/example.com/files/css/css_8b9b5e778d8fc108d57aece98a78e42b.css?l0xab7

Server: nginx/0.7.62
Content-Type: text/css
Last-Modified: Thu, 15 Apr 2010 15:32:01 GMT
Cache-Control: max-age=1209600
Expires: Thu, 29 Apr 2010 15:32:42 GMT
Vary: Accept-Encoding
Content-Length: 63225
Date: Thu, 15 Apr 2010 15:32:42 GMT
X-Varnish: 46147310
Age: 0
Via: 1.1 varnish
Connection: keep-alive
X-Cache: MISS

$ curl -i http://example.com/sites/example.com/files/css/css_8b9b5e778d8fc108d57aece98a78e42b.css?l0xab7

Server: nginx/0.7.62
Content-Type: text/css
Last-Modified: Thu, 15 Apr 2010 15:32:01 GMT
Cache-Control: max-age=1209600
Expires: Thu, 29 Apr 2010 15:32:42 GMT
Vary: Accept-Encoding
Content-Length: 63225
Date: Thu, 15 Apr 2010 15:32:48 GMT
X-Varnish: 46147314 46147310
Age: 6
Via: 1.1 varnish
Connection: keep-alive
X-Cache: HIT
pwolanin’s picture

@DamienMcKenna - can you open a different issue perhaps? Unless you really think this change should not be backported to Drupal 6 (which was the state of this issue)

EvanDonovan’s picture

I think that opening a new issue for #77 would be good, so that this change could be backported - especially for the sake of those of us using reverse proxy caches.

DamienMcKenna’s picture

Will do; I thought this patch might at least *help* towards the goal of this task.

DamienMcKenna’s picture

FYI I've added #772328: Optionally disable using css_js_query_string to handle my patch - d6 for now, d7 later.

EvanDonovan’s picture

Thanks, DamienMcKenna!

Owen Barton’s picture

Version: 6.x-dev » 7.x-dev

@pwolanin - the concern with using query strings to rev files is not that they break caching in reverse proxies (used for scaling/balancing/ha etc), because as I noted in my original comment, that is not the case. Instead, the concern is that they break the normal "forward" caching proxies that are frequently used by large organisations and ISPs. This means that these users have to make a long distance request to the original site, rather than a fast and (relatively) local one.

I am not sure I understand the issue about getting 404s on CSS/JS. There was a bug in old versions of D6 that it would not clear the page cache when aggregate CSS/JS was cleaned up that could cause this behaviour - but Jacob Singh fixed that issue. Caching reverse proxies (at least when properly configured!) shouldn't have this issue either AFAICS, because they should be caching CSS/JS at least as long as regular pages, if not longer. If this is an issue, then it seems that perhaps an approach could be to use filename caching, but be less aggressive about cleaning it up - e.g. only delete files with a ctime of > 1 week.

mfer’s picture

Version: 7.x-dev » 6.x-dev

@Owen Barton Squid and other proxies should be able to cache with the query string. At least according to the details at http://drupal.org/node/772328#comment-2851494.

Owen Barton’s picture

Version: 6.x-dev » 7.x-dev

@mfer - yes, I have already acknowledged (twice) that they can cache content with a query string - my point however, is that until very recently Squid didn't default to do this, and hence the vast majority of ISP/institutional caching squid proxies (and web gateways/filter appliances based on Squid) in the wild have this option disabled. Drupal sysadmins have no control over these (remember, we are not talking about hosted reverse proxies), so unless you are offering to contact all the caching proxy squid users in the whole wide internets and fix their Squid configuration we still have a problem here.

Kiphaas7’s picture

How about MD5'ing both the names and the filemtime() of files as the aggregated filename, without a querystring?

pwolanin’s picture

@Kiphass7 - this is the sort of thing we just removed from Drupal 7. The debate is whether it was the correct change to use a query string that changes instead of changing the file name.

After discussing some of the other cases with Owen today about various use cases, I think I conclude that either:

we made the right decision and should proceed to apply it to Drupal 6, or when we clear the cache we need to retain (not delete) the prior aggregated versions for a long period (e.g. 1 mo to 1 year).

Wim Leers’s picture

Read through the entire issue.

Good change.

But why is this issue marked as 7.x while it's been committed (#61) and now a basic/essential D6 backport is being discussed? Or did I miss something?

pwolanin’s picture

@Wim - there was some concern about using the query string with forward proxies, so it got moved back to Drupal 7.

It would be nice if we could make the filenames correcspond to the hashed content of the aggregated files, but that would need more substantial re-architecting.

pwolanin’s picture

Discussing this with Narayan and others, we shoudl change what code is doing here due to the possible forward proxy problems highlighted by Owen.

pwolanin’s picture

going to roll a stacked patch that depends on http://drupal.org/node/723802

pwolanin’s picture

Category: feature » bug
Status: Needs work » Needs review
Issue tags: -Performance, -frontend performance

New approach - generates the file name based on the hash of the actual file contents. As above - this is a stacked-on patch, so likely will fail to apply to a clean core.

pwolanin’s picture

New approach - generates the file name based on the hash of the actual file contents. As above - this is a stacked-on patch, so likely will fail to apply to a clean core.

AlexisWilke’s picture

Hi Pwolanin,

I like the idea of the version... and if it causes problems to some intermediate proxies, maybe consider putting it in the filename instead of the query string as in:

  sites/default/files/js/drupal-1.2.3.js

and that file would be a copy of the original...

Then, when the original changes, you create a new file such as:

  sites/default/files/js/drupal-1.3.2.js

That way, old pages that are still in some caches (but not the corresponding .js) can reference the old file and still work as expected. New pages will start to make use of the new .js. Some CRON tool could then delete "very" old files (3 or even 6 months old... but here you need to know that drupal-1.2.3.js is the old file...)

Hope this helps a bit 8-}

Thank you.
Alexis Wilke

Owen Barton’s picture

I think the hashing based on file contents might be better off at #678292: Add a development mode to allow changes to be picked up even when CSS/JS aggregation is enabled (or not), where there is an existing patch that makes the aggregate filename sensitive to file changes by including the mtime for each file in the file array. With that patch aggregation can be enabled by default without infuriating new themers. There are some performance issues with the mtime, so this patch allows busy sites can disable the mtime stat check and clear caches manually. Users can also disable aggregation for when using firebug etc. I am not clear if aggregating based on file contents would be faster or not - if so we might be able to drop the new option. Either way, that part seems pretty unrelated to cache expiry - although if I am missing something please explain!

The drupal_delete_file_if_stale() stuff looks great though, and I think we should extract that out into a separate patch so that it is easier for people to review (since that wouldn't need all the sha stuff mixed in).

pwolanin’s picture

@Owen - that issue is trying to solve a different problem. Perhaps using the hash of the content is the right approach there too, but honestly that issue looks like Drupal 8 to me.

sun’s picture

Heavy consideration given #769226: Optimize JS/CSS aggregation for front-end performance and DX --

No matter what we do, the main issue will remain with the current approaches.

So what do others do?

Just recently, I stumbled over www.wepay.com and happened to look at its source. They are doing more or less exactly what crossed my mind with regard to this issue all over again:

<link href="/min/css/v1272414697.reset.css-default.css-button.css-frame.css-welcome.index.css" media="screen" type="text/css" rel="stylesheet" />

The main issue here is that aggregated files may get lost, but external caches and whatnot may still reference them. But URLs are allowed to be long - very long actually. And given aforementioned conceptual aggregation bug #769226, which will heavily decrease the amount of aggregated files, and, also taking into account that file counts may be decreased further to ensure proper file weights (multiple aggregates)... this leads to the open question:

Is there room for an ImageCache-alike approach?

I.e.:

<link href="/sites/default/files/css/jquery.js--jquery.once.js--drupal.js--contextual.js--toolbar.js?v12345" type="text/css" rel="stylesheet" />

Regardless of query string, regardless of whether the aggregate does not exist yet, the referenced aggregate file would load.

pwolanin’s picture

@sun - interesting idea. But requires much more change than this, plus means that in some cases an external cache will still be invalid if Drupal is down.

philbar’s picture

@sun interesting suggestion.

Would it be possible to use an encryption to rename the aggregated css to something smaller:

So this...
/sites/default/files/css/jquery.js--jquery.once.js--drupal.js--contextual.js--toolbar.js?v12345

Becomes this using the CRC Hex Representation...
/sites/default/files/css/f5b1da79.js?v12345

AlexisWilke’s picture

Guys,

That's what the md5() call is for. An md5() is an advanced CRC computation.

Again, putting the version in the file name is probably wiser if you want to keep older versions of it. The fact is that the query string is totally ignored by Drupal. It is there only to make the caches along the way wake up to a new version of the file...

My only worry about running an md5() on the content is that it will take longer as the content can be rather large. On the other hand, unless you are a developer, the content does not change but when you upgrade to a new version. So it should not be a major problem. And doing the md5() on the whole content is much safer than expecting the silly programmers to remember bumping the versions up on each release.

Thank you.
Alexis

philbar’s picture

I'm sorry. I don't think I was clear enough about my suggestion.

I think there should be some concern about filesize optimization.

So instead of having the dozens of characters....
jquery.js--jquery.once.js--drupal.js--contextual.js--toolbar.js

Have a truncated representation. For example, I just ran the CRC encryption on the filename, not the contents.
f5b1da79.js

I think md5() will be too long unless it can be specified as less than 10 characters.

Maybe I'm splitting hairs, but I think for sites with thousands of visitors, a few kb here and there can add up.

/js/f5b1da79.js?v1234
is much better, in my opinion, than...
/sites/default/files/js/jquery.js--jquery.once.js--drupal.js--contextual.js--toolbar.js?v12345

Especially considering it's call on every page...

philbar’s picture

Yep, I just double checked and md5() uses 32 characters, which isn't ideal for bandwidth optimization.

http://php.net/manual/en/function.md5.php

pwolanin’s picture

@all - the patch above is trying to address a defined problem with minimal refactoring. See the linked issue from Owen if you want to delve into deeper architectural changes for Drupal 8.

Owen Barton’s picture

Status: Needs review » Needs work

Having looked at the patch more carefully I now see that it is hashing the file contents only when generating the aggregate, and then storing a "file array hash -> file contents hash" lookup variable that is used to determine the filename on later runs. In my initial (mis)read I thought it was loading and hashing all file contents on every run, which would have been a major performance red flag.

The other thing that was not very clear was how the change in aggregate name generation is related to the issue at hand here. Theoretically it is not - the aged expiry code would work fine without this. However, given the rate at which the "css_js_query_string" currently increments on active sites this is going to lead to a really, really large number of files in this directory. Changing to content based naming, together with #769226: Optimize JS/CSS aggregation for front-end performance and DX will reduce this to a much more sane number. Hence I think it is important that both these changes go together.

So overall, I think this is a great approach. Not only will versioning by filename, rather than querystring improve cacheability but also leads to a dramatic reduction in the length of time a JS/CSS file can be cached by end-users, as the aggregate files no longer need to be redownloaded every time "css_js_query_string" is incremented, even if the actual contents haven't changed at all.

In terms of shortening the aggregate file name - I think that is also a good idea, but please open another issue for this. My sense is probably that we can simply truncate the sha hash pretty dramatically and still avoid collisions. I don't think CRC will add anything over truncating the sha has (I think it's probably worse, for the same length hash). Does anyone care to do the math to determine the value of N characters of sha hash needed to ensure the possibility of a collision on all the aggregate files likely to be generated in the next 100 years is very tiny?

In terms of this patch:

- If submitting a stacked patch, please add an interdiff also, so we can see the actual changes in question. Otherwise it is really tricky to review!

- I think this really needs some comments - it took me a quite a while to figure out what was going on.

- I think a month is certainly a sensible default for expiry - I can't imagine even the most aggressive caches getting more than a month out of sync. I wonder if we should wrap this in a variable_get however, I can imagine some occasions where sites might want to shorten this (e.g. for intranet sites, where there are no intermediate caches), or even lengthen it (e.g. for very large but static sites where pages are mostly served from CDN). The code is only called on cache clear, so I don't think there is any performance consideration.

kkaefer’s picture

I found

substr(base_convert(md5($string), 16, 36), 0, 10);

to be a good compromise. This results in filenames like "6tfzsat59d.css".

pwolanin’s picture

@Owen - I think this would be an improvement regardless of whether we default aggregation to false. The goal of my patch is that the file name only changes when the contents of the aggregated files change. That's exactly the behavior we want.

Owen Barton’s picture

@pwolanin - I meant that the aggregate file name based on file contents and the dated expiry (in other words, this patch) need to go in together. As I said, the dated expiry technically solves the issue in question on it's own, but it is not practical (too many files).

AlexisWilke’s picture

@Owen,

One detail in regard to when files are being aggregated/changed. It is not only when the cache is being cleared. It can happen any time a file is added and/or removed. Especially, some modules will generate more or less .js/.css add depending on the page you're on. Each on of these pages will generate a different aggregated file.

The existing code (without the patch) is 100% the same as what @sun explains in #101 without the versioning. What I mentioned earlier was that by adding the version in the filename, we'd get a perfect naming convention. However, since we need to load all the data to aggregate it, there is no reason to attempt using a version that a programmer may forget to update each time he changes his JavaScript code.

Finally, truncating the sha1() or md5() is fairly useless unless you really have million of users. In that case, you may also want to truncate all the tabs in your themes and all the new lines as you're at it. And any space at the end of the line before the new-line, or at least replace all the \r\n by \n only... You'd probably save several MB with million of hits every day that way... We should have Drupal do that work for us! Add an HTML optimizer!

Thank you.
Alexis

Owen Barton’s picture

One detail in regard to when files are being aggregated/changed. It is not only when the cache is being cleared. It can happen any time a file is added and/or removed. Especially, some modules will generate more or less .js/.css add depending on the page you're on. Each on of these pages will generate a different aggregated file.

Yes - this is easy to fix though: #769226: Optimize JS/CSS aggregation for front-end performance and DX

The existing code (without the patch) is 100% the same as what @sun explains in #101 without the versioning. What I mentioned earlier was that by adding the version in the filename, we'd get a perfect naming convention. However, since we need to load all the data to aggregate it, there is no reason to attempt using a version that a programmer may forget to update each time he changes his JavaScript code.

That proposal is great for non-aggregated files (you can use rewrites to strip version numbers and serve unversioned filenames), however it is off-topic as this issue is about aggregated files. It is not easy to generate a simple version numbering scheme for aggregate files, which is why we use a hash.

Finally, truncating the sha1() or md5() is fairly useless unless you really have million of users. In that case, you may also want to truncate all the tabs in your themes and all the new lines as you're at it. And any space at the end of the line before the new-line, or at least replace all the \r\n by \n only... You'd probably save several MB with million of hits every day that way... We should have Drupal do that work for us! Add an HTML optimizer!

Well, Drupal does have many millions of (end) users, so I think that proves the point really. Still, I think that is a different issue. Optimizing HTML is actually not unreasonable, although cached pages are gzipped, so it is hard to make an impact without actually removing divs.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
6.06 KB

re-roll for sha-256 patch plus adding an opaque variable as Owen suggests.

Jerome F’s picture

Subscribe.

off topic comment - removed -

AlexisWilke’s picture

Jerome,

There are 3 sites on which I had problems and at some point I finally noticed that the problem was that Drupal could not create the css folder. And instead of not trying to aggregate, it failed the write and put the invalid link in the page header... and it does not report the error either. (At least in D6)

Thank you.
Alexis Wilke

Jerome F’s picture

off topic comment - removed -

performance issue caused by panels + wsod module
http://drupal.org/node/789534

Owen Barton’s picture

@All - please open a separate support issue if you are having problems with aggregation in Drupal 6. This patch won't fix your problem, and your comments add noise to the issue and prevent it moving forwards.

@pwolanin - I think the newest patch still needs some kind of comment explaining the high level approach to managing aggregate file names, but other than that it looks good to me.

pwolanin’s picture

now with added comments

Owen Barton’s picture

+ * The file name for the CSS cache file is generated from the hash of all
+ * the aggregated contents. This forces proxies and browsers to download new
+ * CSS when it's really required.  The prior aggregated files also kept until
+ * deleted by drupal_delete_file_if_stale(), so that the CSS referenced by a
+ * cached page will still be valid.

This reads like the full file contents is hashed on every pageload, which would obviously be a bad idea - I think it would also be worth mentioning something along the lines of:
"Once generated, the aggregate filename is retrieved on subsequent page loads via a lookup table that uses the hash of the file array as the key."

Other than that I think this looks great.

pwolanin’s picture

revised code comments.

jhodgdon’s picture

Function header docblocks should start with verbs like "Aggregates" rather than "Aggregate".

I also think the writing could be slightly better if long sentences were broken up, maybe just adding commas. For instance:

+ * The cache file name is retrieved on a page load via a lookup variable using
+ * a hash of the file names as the key and the cache file is generated if
+ * missing. Prior aggregated files are also kept until deleted by
+ * drupal_delete_file_if_stale() when caches are cleared so that the JS
+ * referenced by a cached page will still be valid.
pwolanin’s picture

revised doxygen with further input from jhodgdon and Owen Barton in IRC.

Owen Barton’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready to go

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

This doesn't quite conform to doc standards:
a) There should be a blank line between @param and @return sections

b)

 /**
- * Delete all cached CSS files.
+ * Delete old cached CSS files.
  */

Delete -> Deletes
Same lower down in the JS section.

c)

/**
- * Aggregate JS files, putting them in the files directory.
+ * Aggregates JS, and writes a cache file to the files directory.

I think this should say "Aggregates JavaScript files into a cache file in the files directory." maybe?

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.88 KB

Ok, I think this fixes all the doxygen issues.

jhodgdon’s picture

OK by me!

Dries’s picture

+++ includes/common.inc
@@ -3105,23 +3102,35 @@ function drupal_pre_render_styles($elements) {
+ * lookup key is a hash of the file names in $css. The cache file is generated
+ * if missing, including after the variable is emptied to force a rebuild of
+ * cache. Old cache files are not deleted immediately when the variable is
+ * emptied but are expired after a set period by drupal_delete_file_if_stale().

I had to read that sentence 3 times. It is rather dense/brief -- could we make it a bit easier to read?

Otherwise this patch is RTBC. The documentation improvements are not a requirement, but it would be nice to have.

pwolanin’s picture

@Dries - that version is the third revision attempting to make it more comprehensible with input from Jennifer and Owen ;-)

However, I'll if I can take another pass at it, or maybe find someone less familiar with the code to make suggestions.

pwolanin’s picture

Made those code comments a little longer and more explicit to ideally enable comprehension on the first read.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks Peter et al. Committed the patch in #130 to CVS HEAD.

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

should we try to backport this to Drupal 6?

RobLoach’s picture

Maybe it could go in JavaScript Aggregator for 6?

pwolanin’s picture

To summarize:

the current D6 code changes the file names of the CSS and JS cache file on every cache clear and also deletes the old files.

  1. this degrades performance by forcing proxies/browsers to download new CSS and JS even if the content is the same
  2. this sometimes causes breakage with reverse (or forward) proxies that have the page cached but not all the assets (JS and CSS) cached.
  3. some forward proxies will not cache files with a query string, so the original solution here was deemed incorrect

A full backport would potentially add a new function and change a couple function signatures in Drupal 6.

A partial backport of just the "delete file if stale" function would probably solve #2, but not #1.

AlexisWilke’s picture

pwolanin,

I have had problems with #2 where someone will go to my site and not see any CSS at all which makes the page rather... ugly. So fixing that already would be a plus.

#1 is a nice to have but oh well...

I do not know whether I had problems with #3, I would assume that what looks like #2 could also be related to #3.

On my end, I'll continue to use D6 for a while since I have many customers on it and D7 is not ready yet. (and most of the 200+ modules I use are not D7 compatible.)

Thank you.
Alexis Wilke

EvanDonovan’s picture

I believe a 6.x backport would be very useful, as I had stated above (in the 70's), back when we had an earlier version of the patch.

pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.74 KB

here's a first pass at the partial backport.

I scanned DRUPAL-6--1 contrib and don't see any modules or themes using the functions that would be affected by a full backport, but that's not a 100% guarantee

AlexisWilke’s picture

pwolanin,

I applied this patch, I'll let you know if I see any side effect. As is, it looks good to me.

One thing though, the variable drupal_stale_file_threshold will not get some UI this way. 8-)

Thank you.
Alexis

pwolanin’s picture

@AlexisWilke - it's not intended to have an UI every variable - some tweaks are for experts.

pcambra’s picture

suscribe, does anybody know if this is implemented/going to be implemented in pressflow?

pwolanin’s picture

@pcambra - well, if we get it fixed here in core, it will get merged into pressflow I assume.

pwolanin’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work

oops - the 7.x patch missed removing the query string. rats :-(

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
Owen Barton’s picture

Status: Needs review » Reviewed & tested by the community

Given that was the whole point of revisiting this issue, not sure how we missed this! Looks good though.

AlexisWilke’s picture

Note that today I checked my logs closely (to check on a spammer) and noticed that each page hit from that user generated a reload of the .js and .css files (the .css was less obvious because it is compressed but it was the same thing.) The query string was changing between each page cache and thus the user browser had to reload each distinct "version." I thought that was an interesting side effect. I'm using boost for long term caching and thus I'm 99% sure the problem also comes from there. Ah! And I've been working on simplemenu that resets the js/css cache every time you hit Save in the settings (because I have a newer version not yet in the CVS that has a dynamic CSS file that needs to work next time you visit a page...)

I would assume that after a few weeks, that problem would go away.

Anyway, all that to say that even when not aggregating it would still be a lot better to send a file with an md5 of the content as the filename, instead of filename?<letter/digit>. In such situation, it would save a lot of traffic.

Thank you.
Alexis Wilke

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review

@Alexis Wilke - are you referring to Drupal 6 or Drupal 7? The change already made for Drupal 7 is what you suggest.

AlexisWilke’s picture

This is my current D6. 8-)

Yet, I thought that the D7 was doing that for aggregated files, not for the others... But maybe I did not read the patch properly?

Will the non-aggregated files look like this now:

md5(content of blah.js).js

or like before, more like that:

blah.js?A

This is a problem with non-aggregated files and I guess this thread is not exactly in link with those... but the idea pointed out here is a powerful optimization!

Now imagine using this name:

$filename = $js_path . '/' . md5(filegetcontents($path . 'blah.js')) . '.js'

It would not need to change until the content of blah.js (the source) changes. But if we just send out the files as is when not aggregated, we'll still get:

blah.js?A

and later

blah.js?B

etc.

And again, all of those are non-aggregated files.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

@AlexisWilke - please check the D7 patches in detail. D7 does now base the file name on the hash of the contents

hashing the contents on every page load is a significant performance hit, so not something we will do.

(also, resetting status which I did not mean to change)

webchick’s picture

That extra query string is very useful as a means to do server-side clearing of clients' caches in the case of updated CSS.

Can someone point me to a good summary in this issue of why removing this feature is desirable?

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs review

Ok, spoke with pwolanin in #drupal-contribute.

He summarized this issue as it replacing the existing code, which appends a querystring in the case of non-aggregated CSS files, and a hashed name in the case of aggregated ones, and re-generates both in case of a cache clear... with a smarter algorithm that instead only re-generates in case of actual changes. So the behaviour I want to preserve is there, just smarter.

So there's no point in keeping the old behaviour around, therefore this patch is just a small clean-up.

Committed to HEAD. Thanks!

Back to 6.x and needs review for #137.

AlexisWilke’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community

@pwolanin - nevermind 8-) Actually you have a version string that comes from the JavaScript being sent to you. In D6, you are using the same query string addition which changes all the time for ALL the scripts.

catch’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Needs review

Looks like a cross post.

sun’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs review » Needs work
+++ includes/common.inc
@@ -3105,23 +3102,39 @@ function drupal_pre_render_styles($elements) {
+function drupal_build_css_cache($css) {
...
+  $map = variable_get('drupal_css_cache_files', array());
+  $key = hash('sha256', serialize($css));

@@ -3142,11 +3155,20 @@ function drupal_build_css_cache($css, $filename) {
+    variable_set('drupal_css_cache_files', $map);

@@ -3275,10 +3297,21 @@ function _drupal_load_stylesheet($matches) {
 function drupal_clear_css_cache() {
...
+  variable_del('drupal_css_cache_files');
...
+function drupal_delete_file_if_stale($uri) {
+  // Default stale file threshold is 30 days.
+  if (REQUEST_TIME - filemtime($uri) > variable_get('drupal_stale_file_threshold', 2592000)) {

@@ -4257,23 +4287,39 @@ function drupal_add_tabledrag($table_id, $action, $relationship, $group, $subgro
+function drupal_build_js_cache($files) {
...
+  $map = variable_get('drupal_js_cache_files', array());

@@ -4281,22 +4327,30 @@ function drupal_build_js_cache($files, $filename) {
+    variable_set('drupal_js_cache_files', $map);
...
 function drupal_clear_js_cache() {
...
-  variable_set('javascript_parsed', array());
+  variable_del('javascript_parsed');
+  variable_del('drupal_js_cache_files');

AFAIK, we do not have a single system variable that is prefixed with "drupal_".

Since these aggregates are globally dependent on the corresponding "preprocess_*" variables, they should be in the same namespace, i.e.:

preprocess_js_files
preprocess_css_files
preprocess_stale_treshold

79 critical left. Go review some!

pwolanin’s picture

hmm, didn't even notice this comment until now - seems like a valid critique.

locomo’s picture

subscribe

bleen’s picture

Status: Needs work » Needs review
FileSize
2.69 KB

I dont disagree with Sun's point in #153, but its worth pointing out that these variables exist too:

drupal_http_request_fails
drupal_test_email_collector
drupal_private_key
drupal_stale_file_threshold

This patch makes these changes:
drupal_js_cache_files -> preprocess_js_cache_files
drupal_js_version_query_string -> preprocess_js_version_query_string
drupal_css_cache_files -> preprocess_css_cache_files

Owen Barton’s picture

Status: Needs review » Needs work

drupal_js_version_query_string -> preprocess_js_version_query_string

I don't think this is an accurate name - the query string used only for non-preprocessed (i.e. individually served) JavaScript files.

I think "js_version_query_string" is both accurate and sufficiently detailed.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
2.99 KB

Here's the variable name as Owen suggests plus reducing the default saved time to 3 days from 30, since that turns out to be silly in practice.

Owen Barton’s picture

Now that #769226: Optimize JS/CSS aggregation for front-end performance and DX is committed (hooray!) the number of aggregated files generated should be much more sane and minimal (really just a handful), so I think a higher number of days might still be reasonable. Even if a site has weekly releases that include CSS/JS changes I think you shouldn't end up with an overwhelming number of files.

Other than that I think this is RTBC.

pwolanin’s picture

Well, the # of days should correspond to the maximum time any forward or reverse proxy might reasonably hold the page. for that reason 3 seems reasonble (I could be persuaded to 5 or whatever, just not multiple weeks).

In development where one clears the cache often, the number of files can get quite large.

bleen’s picture

my 2¢'s = I think 3 days is plenty

mikeytown2’s picture

I would opt for 2 weeks, it's what we use in other places; like htaccess.
http://drupalcode.org/viewvc/drupal/drupal/.htaccess?view=markup

bryancasler’s picture

Patch in #67 does not work for me.

bryancasler’s picture

#137 I am trying this patch but I am confused if it is intended to solve the problem I came here for.

On my site I aggregate JS and CSS files. I'm also using boost.

The problem I am trying to fix is that updates to my sites CSS or JS breaks all the pages cached by boost because the JS and CSS file names change. Is the patch in #137 intended to fix this? and if so how long until the CSS and JS are re-aggregated after changes are made?

Example output
href="/sites/default/files/css/css_d9fe3bfc3489744012374479bc8e4146.tidy.css"
src="/sites/default/files/js/js_e9acbc38a52b8c288bb69ecd05a148f8.jsmin.js"

AlexisWilke’s picture

animelion,

No, that's another problem I ran into. Boost does not take the default Performance flags in account.

Under "Boost cacheability settings" make sure that the .css and .js files are being cached.

I reported that problem to the Boost people and the answer was that they did not have to do anything about it...

Thank you.
Alexis

mikeytown2’s picture

Take a look at the "Ignore cache flushing: " setting under the "Boost cache expiration/flush settings" section. In short if you have css/js caching enabled you can ignore hook_flush_caches and not have to wipe out the html cache every time cron runs. Blog post explaining the problem http://www.metaltoad.com/blog/how-drupals-cron-killing-you-your-sleep-si...

bryancasler’s picture

Thanks for everyones help, I've made site adjustments accordingly. I still have one question though. If I did make a change to the CSS is there a way I can view those changes on the homepage without flushing the site's css and damning all the cached pages?

AlexisWilke’s picture

animelion,

Yes, I do that all the time. Load your home page and look at the current .css filename, then delete the corresponding files (in boost perm cache and files/css/...) Finally, reload your home page.

It's a bit of manual work, but it has been working for me and better than deleting everything or turning off the caching while testing...

Thank you.
Alexis Wilke

bryancasler’s picture

Brilliant! Thanks for the work around Alex. I will give it a shot later tonight.

EvanDonovan’s picture

Is this already in Pressflow 6.19? Also, what's necessary on 7.x before this can be backported to standard 6.x? This will help many users greatly, both with regard to Boost and Varnish caching.

The variable name and time changes seem pretty simple from looking at the code. I didn't test though.

EvanDonovan’s picture

#158: 721400-variable-names-158.patch queued for re-testing.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

In #159 Owen says this is RTBC except for number of days, but note that this number of day is only relevant to how long a browser or proxy holds a cached page without refreshing it from Drupal. A typical max-age header is for 5 or 10 minutes, so I think 3 days is more than sufficient.

pwolanin’s picture

#158: 721400-variable-names-158.patch queued for re-testing.

hass’s picture

Now core does not delete my JS caches anymore. Is this really intentionally? drupal_clear_js_cache() calls drupal_delete_file_if_stale and this function only deletes files older than 3 days (currently 30 days). I think I understand your idea about keeping the files for some days, but if I do not press the "Cache clear" button in performance settings - the files are never ever deleted...

At least DX is broken as I still expect a drupal_clear_js_cache() will purge my outdated JS files from disk...

Owen Barton’s picture

The marker to the "current" set of files is outdated, so the old files are not referenced in generated pages any more. The old files are still available in case proxies/browsers upstream manage to cache old HTML requests, but not the CSS/JS files that they reference (which currently leads to broken pages).

hass’s picture

Yes, but now with the latest code (and this patch, too) my disk "fills up" with stale files and I may have 1 year old JS files on my disk if I do not press the clear cache button for one year...

AlexisWilke’s picture

@hass,

I have that patch installed and my /js/ folder has 30-day old files, no more. The clearing function is being called in other circumstances (when a new file is created.) What could happen under MS-Windows is that the file creation date is not correctly returned... I thought I read something about that somewhere.

Now what does happen is a large number of files because of some conditional CSS load from different modules. So I get tons of CSS files. My main site has 1215 CSS files, but again, the oldest is 31 days, no more. With 3 days, it will reduce that count quite a bit which is great (probably ~40 files.)

Thank you.
Alexis

hass’s picture

I was not able to find a place where the clearing is called on normal production machines. It's called id you run an update hook , but there is no cron or so on... It looks like luck if your disk get's cleared...

AlexisWilke’s picture

I guess you are correct. I suppose I don't see those lists grow any more than that because I work so much on all my websites. I could be called from the function that creates new JSes and we'd want to add a variable so the check only happens once a day to avoid wasting to much time.

function drupal_build_js_cache($files, $filename) {
  $contents = '';

  // Create the js/ within the files folder.
  $jspath = file_create_path('js');
  file_check_directory($jspath, FILE_CREATE_DIRECTORY);

  if (!file_exists($jspath .'/'. $filename)) {
    // Build aggregate JS file.
    foreach ($files as $path => $info) {
      if ($info['preprocess']) {
        // Append a ';' and a newline after each JS file to prevent them from running together.
        $contents .= file_get_contents($path) .";\n";
      }
    }

    // Create the JS file.
    file_save_data($contents, $jspath .'/'. $filename, FILE_EXISTS_REPLACE);

    $last = variable_get('last_delete_call_time', 0);
    if ($last + 86400 < time()) {
      variable_set('last_delete_call_time', time());
      drupal_clear_js_cache();
    }
  }

  return $jspath .'/'. $filename;
}

We'd need something similar for CSS files.

hass’s picture

Maybe... I cannot remember exactly where and when drupal_build_js_cache() is called, but I guess only if a JS file changes... so - no cleanup :-)

EvanDonovan’s picture

@hass: So you're saying the old files will never get deleted if the site runs in production (i.e., no module changes, etc., so no JS changes), since they stop being referenced, but aren't actually deleted?

Unfortunately, and I say this as a user of the Varnish reverse proxy, this doesn't sound good. Possibly would we need another setting added, something like "file lifetime", which could be set on the performance page also, and then people could set it to be longer than things would persist in the cache of their reverse proxy? The default for this could be something like 30 days, which should be plenty of time for reverse proxies to no longer be referencing the old files.

(Alternatively, if that was bad UX from the perspective of having too many settings, it could be a variable that was set, but which wasn't visible in the UI. Then users of reverse proxies would have to change it in settings.php if they were unhappy with the default.)

hass’s picture

So you're saying the old files will never get deleted if the site runs in production (i.e., no module changes, etc., so no JS changes), since they stop being referenced, but aren't actually deleted?

Code wise - currently - yes. Only search core code for drupal_clear_js_cache() calls and you will not find any calls to this function - except you interactivly press the "clear cache button" or you "submit" the modules page form. This are all things that never happens in production...

EvanDonovan’s picture

@hass: Currently on my site, I actually press this in production once in a while, but that's because we make changes more often than changes are probably made to the average production website. So do you think something like my proposal in #181 is necessary?

I am hesitant to set this issue back to "needs work" though, because I really would like to see a backport to D6, and that can't happen until this followup is committed to D7.

catch’s picture

Even if they never get deleted without a cache clear, and most sites change the contents of these files at some point, then you're still only going to have a files a maximum of 30 days old at the time the last cache clear ran in that directory.

AlexisWilke’s picture

@catch, no, that's the point. Like EvanDonovan, I'm changing things on my website all the time and thus it's kept in check, but if you are to keep going without updating anything (i.e. modules or run a performance + clear cache) then the number of files will build up.

Doing like in #179 would clear old files whenever you create a new one. That would maintain the number of files, whether it's 3 or 30 days (number of days can be changed in the settings.php file.) It won't matter if the function isn't called for 60 days if no new file is created.

I propose to add this, to be clear:

    $last = variable_get('last_delete_call_time', 0);
    if ($last + 86400 < time()) {
      variable_set('last_delete_call_time', time());
      drupal_clear_js_cache();
    }
EvanDonovan’s picture

@AlexisWilke: Thanks for the clarification. Your proposed code makes sense to me. We'll have to see what the original patch authors (pwolanin, et al.) think...

EvanDonovan’s picture

Bump - I need this on a D6 site. Is there still a chance of this getting in for D7?

pwolanin’s picture

#158: 721400-variable-names-158.patch queued for re-testing.

tom_o_t’s picture

No changes made, bot is stuck, re-uploading patch for retesting.

bleen’s picture

Status: Reviewed & tested by the community » Needs review

here testbot

bleen’s picture

i think it needs a .patch extension ... same patch from #189

hass’s picture

Patch in #605318: Add some garbage collection to the update manager may be of interrest here, too.

momper’s picture

subscribe

drupalninja99’s picture

do we know if this patch works (#67 for drupal 6)? i am so fed up with aggregated files, eventually something happens that causes the files to get renamed and varnish or some other cache is left referring to aggregated files that dont exist bc the names change.

I would use this patch on high performance sites if i knew it worked.

drupalninja99’s picture

Index: includes/common.inc
===================================================================
--- includes/common.inc	(revision 274)
+++ includes/common.inc	(working copy)
@@ -1852,7 +1852,7 @@
   // browser-caching. The string changes on every update or full cache
   // flush, forcing browsers to load a new copy of the files, as the
   // URL changed.
-  $query_string = '?'. substr(variable_get('css_js_query_string', '0'), 0, 1);
+  $query_string = '?'. substr(variable_get('css_js_query_string', '0'), 0, 2);
 
   foreach ($css as $media => $types) {
     // If CSS preprocessing is off, we still need to output the styles.
@@ -1897,7 +1897,7 @@
     if ($is_writable && $preprocess_css) {
       // Prefix filename to prevent blocking by firewalls which reject files
       // starting with "ad*".
-      $filename = 'css_'. md5(serialize($types) . $query_string) .'.css';
+      $filename = 'css_'. md5(serialize($types)) .'.css';
       $preprocess_file = drupal_build_css_cache($types, $filename);
       $output .= '<link type="text/css" rel="stylesheet" media="'. $media .'" href="'. file_create_url($preprocess_file) .'" />'."\n";
     }
@@ -2261,7 +2261,7 @@
   // URL changed. Files that should not be cached (see drupal_add_js())
   // get time() as query-string instead, to enforce reload on every
   // page request.
-  $query_string = '?'. substr(variable_get('css_js_query_string', '0'), 0, 1);
+  $query_string = '?'. substr(variable_get('css_js_query_string', '0'), 0, 2);
 
   // For inline Javascript to validate as XHTML, all Javascript containing
   // XHTML needs to be wrapped in CDATA. To make that backwards compatible
@@ -2300,7 +2300,7 @@
   if ($is_writable && $preprocess_js && count($files) > 0) {
     // Prefix filename to prevent blocking by firewalls which reject files
     // starting with "ad*".
-    $filename = 'js_'. md5(serialize($files) . $query_string) .'.js';
+    $filename = 'js_'. md5(serialize($files)) .'.js';
     $preprocess_file = drupal_build_js_cache($files, $filename);
     $preprocessed .= '<script type="text/javascript" src="'. file_create_url($preprocess_file) .'"></script>'."\n";
   }
@@ -2689,7 +2689,7 @@
         'created' => $_SERVER['REQUEST_TIME'],
         'headers' => array(),
       );
-    
+
       // Restore preferred header names based on the lower-case names returned
       // by drupal_get_header().
       $header_names = _drupal_set_preferred_header_name();

I created a patch for pressflow but I dont know what the paths need to be. It always stores my local file paths in the patch so I tried to remove those. I think for my acquia pressflow sites, if this works I'm going to use it every time just bc sooner or later aggregated files are going to be out of sync with some cache, be it varnish or page cache and this gives it a fighting chance.

jcisio’s picture

subscribe

mikeytown2’s picture

Came up with a module. It's my take on how aggregation should work. It is a D6 module.
http://drupal.org/project/advagg
Still needs some polishing but it is fully functional and feedback is greatly appreciated.

EvanDonovan’s picture

@mikeytown2: That is great news. Possibly this will allow us to fix a long-standing cache issue on our (Varnish-using) 6.x site.

ChrisLaFrancis’s picture

Subscribing.

drupalninja99’s picture

ya i like mikeytowns idea once that is stable. in the meant time one solution for your theme is to take your theme files out of the .info file and embed them in the page.tpl or have some module do that automatically (the how is the trick there havent figured that out). If you only say half a dozen stylesheets its not that big of a deal, its more of a drawback if you have a bunch in your theme.

i did this for a site and i think i will keep doing it bc the alternative is just unacceptable for a high performance site. there are so many actions that inadvertently delete those aggregated files so if you move your theme css out you mitigate the risk and you dont have to completely abandon aggregation altogether.

mikeytown2’s picture

@shomam
Working on something much better in D6 at the moment. It's at RC3. Give it a shot and let me know how it works.
http://drupal.org/project/advagg

EvanDonovan’s picture

Version: 7.x-dev » 8.x-dev

If there is still work to be done in 7.x, shouldn't this patch be bumped up to 8.x and then backported to 7.x once it gets in there?

dstol’s picture

Sub

Agileware’s picture

Subscribing

pwolanin’s picture

Issue tags: +Needs backport to D6

needs a backport to D6 so we retain the aggregated CSS/JS files for some period rather than immediately deleting them. My patch at #67 is really no good since it's an approach we later discarded.

hejazee’s picture

JohnAlbin’s picture

#191: 721400-variable-names-158-retest.patch queued for re-testing.

I can already tell 191 needs a re-roll so it applies to the files in the core/ directory.

Status: Needs review » Needs work
Issue tags: +Performance, +Needs backport to D6, +frontend performance

The last submitted patch, 721400-variable-names-158-retest.patch, failed testing.

dstol’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

Here's a quick re-roll based on #191 for 8.x.

mcjim’s picture

Re-roll to match HEAD.

Status: Needs review » Needs work

The last submitted patch, 721400-variable-names-211.patch, failed testing.

mcjim’s picture

Status: Needs work » Needs review
FileSize
4.46 KB

Updated core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.php with new variable names.

nod_’s picture

Not sure how that related to the first post. Haven't read everything but it probably needs a new summary.

mcjim’s picture

Updated summary.
The original issue has already been sorted, and the current patch merely changes variable names for consistency with core and sets the default stale file threshold to three days.

bleen’s picture

Title: Don't change filenames for aggregated JS/CSS » Rename aggregated JS/CSS variables to be consistent with core
Status: Needs review » Reviewed & tested by the community

The patch in #213 looks good

aspilicious’s picture

Why not using the config system now?

webchick’s picture

Status: Reviewed & tested by the community » Needs work

If we're renaming variables, then we need an update path to do this, or at the very least to drop the old variables so they don't sit around crufting up the database. I don't understand why we're renaming variables, though, nor why variable renaming is holding up a 6.x bug fix. I would recommend moving that out into its own issue as a follow-up.

sun’s picture

Thanks @webchick for holding this off :)

These variables will be "renamed" for D8 anyway. In fact, the concrete variables we're talking about here are not even variables, but "state data" instead, for which we'll introduce a completely separate state storage; see #1175054: Add a storage (API) for persistent non-configuration state

I'm not sure whether it is relevant for this issue in any way, but the only functional change in this patch seems to be this:

+++ b/core/includes/common.inc
@@ -3461,8 +3461,8 @@ function drupal_clear_css_cache() {
 function drupal_delete_file_if_stale($uri) {
-  // Default stale file threshold is 30 days.
-  if (REQUEST_TIME - filemtime($uri) > variable_get('drupal_stale_file_threshold', 2592000)) {
+  // Default stale file threshold is 3 days.
+  if (REQUEST_TIME - filemtime($uri) > variable_get('drupal_stale_file_threshold', 259200)) {
     file_unmanaged_delete($uri);
   }

Somehow the latest patch in here heavily diverged from the original patch that was RTBC in #143...

The above quoted change only affects the GC routine, so it's not really clear what kind of Performance bug is being fixed here.

Wim Leers’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Closed (fixed)
Issue tags: -Needs backport to D6, -Needs issue summary update

Read through this entire issue.


#43:

IMHO the optimal solution would be a pluggable preprocess system. Drupal would provide a good default. Then site builders could swap that out for something tailored to their context. This isn't going to happen before D8.

Done now: #352951: Make JS & CSS Preprocessing Pluggable.


Backport to D6: that hardly seems relevant anymore, after years of zero activity on the backport, and D8 around the corner. Removing the tag.


Current patch in #210/#213: has been silent for many months, is a tiny change, and it's repurposing this issue for the fifth or so time. It's time this issue got closed.

webchick’s picture

Title: Rename aggregated JS/CSS variables to be consistent with core » Order JS files according to weight, don't change filenames for aggregated JS/CSS

Restoring title.

webchick’s picture

Issue summary: View changes

Updated issue summary.

candelas’s picture

Issue summary: View changes

Any news? I keep with the problem. Thanks for your knowledge and time :)