Posted by chx on August 19, 2008 at 11:37am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | chx |
| Status: | needs work |
| Issue tags: | Performance |
Issue Summary
This is quite trivial and helps with performance -- various accelerator methods can have a better grip on what Drupal generates. page_set_header will need patching too but this is good as it is.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| headers.patch | 618 bytes | Idle | Passed: 7474 passes, 0 fails, 0 exceptions | View details | Re-test |
Comments
#1
It looks like drupal_page_cache_header() already does that for the Etag part. The Content-Length might help, thou.
#2
Could you please elaborate how the Content-Legth field can help with performance ? if you can give some URLs it would be nice !. Thanks.
#3
Sorry I mistakenly changed the title of the thread.
#4
I have found problems applying this patch on my site.
Specifically, I have found incomplete load of our home page when it is served from the cache on Internet Explorer 6, we use a lot of jquery and ajax calls on the web page, and the problem dissapears if I remove the Content-Length header and truncate cache_page.
#5
Revised patch that adds the content-length to drupal_page_cache_header, instead of adding the headers really late in page_set_cache.
#6
Revision to title because this patch no longer adds the etag header, since the drupal_page_cache_header function already does that. The patch in #5 also moves some header() calls around so that they can be done together.
#7
I am watching the patch and it seems not right to me. After adding the header Content-Lenght, there is a condition that uncompress cache->data if the browser does not support compression, that would make the value previously set invalid, wouldn't it?
#8
No, it sends that header only after $cache->data has been prepared.
#9
Ok, I didn't apply the patch and I was looking at my source code, which is D5:
function drupal_page_cache_header($cache) {
// Set default values:
$last_modified = gmdate('D, d M Y H:i:s', $cache->created) .' GMT';
$etag = '"'.md5($last_modified).'"';
// See if the client has provided the required HTTP headers:
// *** Added preg_replace; some browsers append a "; length=..." entity to the header
$if_modified_since = isset($_SERVER['HTTP_IF_MODIFIED_SINCE']) ? preg_replace('/;.*$/','',stripslashes($_SERVER['HTTP_IF_MODIFIED_SINCE'])) : FALSE;
$if_none_match = isset($_SERVER['HTTP_IF_NONE_MATCH']) ? stripslashes($_SERVER['HTTP_IF_NONE_MATCH']) : FALSE;
// *** Added:
// ***IE6 does not support Entity Tags, so depend on the if_modified_since to guide us.
if (preg_match('/MSIE\s6\.\d+/i',$_SERVER['HTTP_USER_AGENT']) && $if_modified_since!==false)
$if_none_match = '"'.md5($if_modified_since).'"';
if ($if_modified_since && $if_none_match
&& $if_none_match == $etag // etag must match
&& $if_modified_since == $last_modified) { // if-modified-since must match
header('HTTP/1.1 304 Not Modified');
// All 304 responses must send an etag if the 200 response for the same object contained an etag
header("Etag: $etag");
exit();
}
// Send appropriate response:
header("Last-Modified: $last_modified");
header("ETag: $etag");
// The following headers force validation of cache:
header("Expires: Sun, 19 Nov 1978 05:00:00 GMT");
header("Cache-Control: must-revalidate");
// Determine if the browser accepts gzipped data.
if (@strpos($_SERVER['HTTP_ACCEPT_ENCODING'], 'gzip') === FALSE && function_exists('gzencode')) {
// Strip the gzip header and run uncompress.
$cache->data = gzinflate(substr(substr($cache->data, 10), 0, -8));
}
elseif (function_exists('gzencode')) {
header('Content-Encoding: gzip');
}
Obvisouly to apply this patch on this code, the header('Content-Length') should be set AFTER the line:
$cache->data = gzinflate(substr(substr($cache->data, 10), 0, -8));because this line modifies the length.
That was what I was saying. Sorry for the confusion.
#10
Patch attached according to #9. Add also a comment (from http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html ) :
// Send size of the entity-body, in decimal number of octets:#11
Ok seriously, the patch in #5, sends that header after all the $cache->data manipulation has been done. You need to try the patch. According to the description, "The patch in #5 also moves some header() calls around so that they can be done together." (for clarity).
<?php
if (variable_get('page_compression', TRUE)) {
// Determine if the browser accepts gzipped data.
if (@strpos($_SERVER['HTTP_ACCEPT_ENCODING'], 'gzip') === FALSE && function_exists('gzencode')) {
// Strip the gzip header and run uncompress.
$cache->data = gzinflate(substr(substr($cache->data, 10), 0, -8));
}
elseif (function_exists('gzencode')) {
header('Content-Encoding: gzip');
}
}
+ // Send appropriate response:
+ header("Last-Modified: $last_modified");
+ header("ETag: $etag");
+ header('Content-Length: ' . strlen($cache->data));
+
+ // The following headers force validation of cache:
+ header("Expires: Sun, 19 Nov 1978 05:00:00 GMT");
+ header("Cache-Control: must-revalidate");
+
?>
I'm all for the additional comment in #10, but for readability sake, let's please base off the patch in #5.
#12
So ...
#13
#14
Dave Reid is absolutely right, his patch applies correctly.
Is it not possible to delete my previous post (#10) ?
I see that I can edit it, but I am a bit ashamed so I would like to remove it...
(you can consider me as a kind of drupal.org newbie :)
#15
Note that setting Content-Length explicitly causes problems with PHPs zlib.output_compression feature, so I suggest not adding the header if this feature is enabled:
http://www.php.net/manual/en/ref.zlib.php#43181
Could you give some examples of situations where this would improve performance?
#16
The last submitted patch failed testing.
#17
Failed due to #74645: modify file_scan_directory to include a regex for the nomask.. Setting back to code needs review.
#18
The last submitted patch failed testing.
#19
Add tag.
#20
What's the status of this? Is it safe to add the Content-Length header even for gzipped pages?