httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: FLUSH, filtering, setaside, etc (was Re: Problems with EOS optimisation in ap_core_output_filter() and file buckets.)
Date Fri, 20 Feb 2009 09:15:02 GMT
On Thu, Feb 19, 2009 at 10:00:50PM +0100, Ruediger Pluem wrote:
> On 02/19/2009 12:32 PM, Joe Orton wrote:
...
> > @@ -497,13 +500,17 @@
> >                  next = APR_BUCKET_NEXT(bucket);
> >              }
> >              bytes_in_brigade += bucket->length;
> > -            if (!APR_BUCKET_IS_FILE(bucket)) {
> > +            if (APR_BUCKET_IS_FILE(bucket)) {
> > +                num_files_in_brigade++;
> > +            } 
> > +            else {                
> >                  non_file_bytes_in_brigade += bucket->length;
> >              }
> >          }
> >      }
> >  
> > -    if (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER) {
> > +    if (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER
> > +        || num_files_in_brigade >= THRESHOLD_MAX_FILES) {
> 
> If the 16 FD's were split over more then one brigade and the
> brigades before us were set aside the FD's belong already to the wrong pool
> (the connection pool). Deleting a file bucket doesn't close the FD it uses.

Not sure what the concern is there - this loop is iterating over the 
concatenation of the buffered brigade an the "new" brigade (right?), so 
it will count the total number of buckets which are potentially left 
buffered after this c_o_f invocation terminates.

w.r.t. MMAP buckets: the 64K bytes limit will apply here, since FILE 
only morphs into MMAP if the file size is > 8K.  (And no, theoretically, 
the fd from which an MMAP bucket was derived is not needed after the 
mmap() call, but I don't think the fds actually get closed earlier than 
pool closure, normally)

w.r.t. multiple FILE buckets for a single fd; failing to buffer as much 
as possible will not be the end of the world, but I suppose that case is 
common, so we could cope with it something like this.  (completely 
untested)

Index: server/core_filters.c
===================================================================
--- server/core_filters.c	(revision 734690)
+++ server/core_filters.c	(working copy)
@@ -367,6 +367,7 @@
 
 #define THRESHOLD_MIN_WRITE 4096
 #define THRESHOLD_MAX_BUFFER 65536
+#define THRESHOLD_MAX_FILES 16
 
 /* Optional function coming from mod_logio, used for logging of output
  * traffic
@@ -380,7 +381,8 @@
     core_output_filter_ctx_t *ctx = net->out_ctx;
     apr_bucket_brigade *bb;
     apr_bucket *bucket, *next;
-    apr_size_t bytes_in_brigade, non_file_bytes_in_brigade;
+    apr_size_t bytes_in_brigade, non_file_bytes_in_brigade, num_files_in_brigade;
+    apr_file_t *last_file_seen;
 
     /* Fail quickly if the connection has already been aborted. */
     if (c->aborted) {
@@ -466,6 +468,9 @@
 
     bytes_in_brigade = 0;
     non_file_bytes_in_brigade = 0;
+    num_files_in_brigade = 0;
+    last_file_seen = NULL;
+
     for (bucket = APR_BRIGADE_FIRST(bb); bucket != APR_BRIGADE_SENTINEL(bb);
          bucket = next) {
         next = APR_BUCKET_NEXT(bucket);
@@ -497,13 +502,20 @@
                 next = APR_BUCKET_NEXT(bucket);
             }
             bytes_in_brigade += bucket->length;
-            if (!APR_BUCKET_IS_FILE(bucket)) {
+            if (APR_BUCKET_IS_FILE(bucket) 
+                && (last_file_seen == NULL
+                    || last_file_seen != ((apr_bucket_file *)bucket->data)->fd)) {
+                num_files_in_brigade++;
+                last_file_seen = ((apr_bucket_file *)bucket->data)->fd;
+            } 
+            else {                
                 non_file_bytes_in_brigade += bucket->length;
             }
         }
     }
 
-    if (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER) {
+    if (non_file_bytes_in_brigade >= THRESHOLD_MAX_BUFFER
+        || num_files_in_brigade >= THRESHOLD_MAX_FILES) {
         /* ### Writing the entire brigade may be excessive; we really just
          * ### need to send enough data to be under THRESHOLD_MAX_BUFFER.
          */

Mime
View raw message