Graham Leggett wrote: > On Wed, October 25, 2006 5:58 pm, Joe Orton wrote: > >> Another couple of hundred lines of code and even a new config directive, >> and this still doesn't get close to actually fixing the problem! -1 >> already, this code is just not getting better. > > As has been explained a few times before, there isn't "a problem" or "the > problem", but rather many problems. > > These problems, are only practically solveable with many patches, each one > addressing a specific problem or behavior.
Well said. > The patch just committed removes a burden from the cache providers, in > that from a single source, there is control over the size of buckets that > cache providers are expected to handle. The alternative was one directive > per cache provider, which is not ideal. > > The patch just committed is part of an ongoing work to improve the > behaviour of the cache in the real world, to solve real problems at real > sites. > > Progress to date: > > - The cache can now cache a file, and serve from the cache at the same time. > > - While caching a file, data is sent both to the cache, and to the end > client, at the same time. The cache no longer caches the entire file > before sending it to the client. > > - It is possible to cache a file at full disk speed, even when the > downstream client is slower than the disk, without buffering data in RAM. > > Next step is to remove the copy_body() hack inside mod_disk_cache, as it > is unnecessary. I would (I know I can't :) vote for reverting that last patches (up to r450089) so that we can start all over. > To say "it's not getting better" without actually running the code and > seeing the progress involved is very hollow criticism. > > I appreciate the effort involved in pointing out areas where issues need > to be addressed, but having to contend with the constant barrage of > negativity, and the ridiculous notion that "the problem cannot be solved", > is a really tiring exercise. +1 >> mod_disk_cache is still >> liable to eat all your RAM in that apr_bucket_read() loop, > > Correct, and this will be fixed. > > We ideally want file writes to happen using a file write output filter, > which can then encapsulate any bucket specific weirdness exactly like > ap_core_output_filter does. > >> the >> apr_bucket_split() is not guaranteed to work for a morphing bucket type. > > Then morphing buckets are broken. > > The split method must either succeed, or return a non success APR value. > Both of these cases are handled for by the split loop. If morphing buckets > crash, then they must be fixed. > >> It is simple enough to fix this problem without adding all this code and >> without all the stuff in r450105 too, something like the below. > > I still see that this call is intact: > > apr_bucket_read(e, &str, &length, APR_BLOCK_READ) > > Given a 4.7GB bucket, it will attempt to read all 4.7GB of the bucket into > RAM, and abort. > > Is there something I am missing? Yes, first because size_t is 32 bits :). When you do a read like this on a file bucket, the whole bucket is not read into memory. The read function splits the bucket and changes the current bucket to refer to data that was read. The problem lies that those new buckets keep accumulating in the brigade! See my patch again. So Joe's patch removes this newly implicitly created bucket from the brigade, passes it to the client, and delete it. -- Davi Arnaut
