On Wed, October 25, 2006 11:48 pm, Davi Arnaut wrote: > No, we start with a file bucket on the brigade, I will try to explain > what happens to see if we are talking about the same thing. > > Suppose we have a brigade containing a a file bucket, and the file size > is 4.7GB. We want to read it fully. > > When we call apr_bucket_read() on this bucket, we end-up calling the > bucket read function (file_bucket_read). What does the bucket file read do > ? > > If mmap is supported, it mmaps APR_MMAP_LIMIT bytes (4MB) by creating a > new mmap bucket and splits the bucket. So, after calling read on a file > bucket you have two buckets on the brigade. The first one is the mmap > bucket and last is file bucket with a updated offset. > > The same thing happens if mmap is not supported, but the bucket type > will be a heap bucket. If we don't delete or flush those implicitly > created buckets they will keep the whole file in memory, but one single > read will not put the entire file on memory. > > What Joe's patch does is remove this first implicitly created bucket > from the brigade, placing it on the brigade on a temporary brigade for > sending it to the client.
Ok, this makes more sense, but in its current form the temporary brigade should not be necessary. While disk cache goes through the brigade, it replaces buckets on the incoming brigade with a bucket referring to the cached file. I think a number of the thorny memory problems in mod_*cache have come about because the brigade is read twice - once by store_body, and once by ap_core_output_filter. The replacing of the buckets with file buckets in the brigade by disk cache means the brigade will only be read once - by save_body(). > That's why splitting the brigade with magical values (16MB) is not such > a good idea, because the bucket type knows betters and will split the > bucket anyway. Right, the picture is getting clearer. Look closer at the cache code in mod_cache, right at the end of the CACHE_SAVE filter. Notice how store_body() is called, followed by ap_pass_brigade(). Notice how store_body() is expected to handle the complete brigade, all 4.7GB of it, before ap_pass_brigade() has a look see. Now - create a 4.7GB file on your drive. Copy this file to a second filename. Time this and tell me how long it takes. Do you see the further problem? The split-before-save_body patch wasn't described well enough by me - it is designed to prevent save_body() from having to handle bucket sizes that are impractically large to handle by a cache, both in terms of memory, and in terms of time. Regards, Graham --
