apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@covalent.net>
Subject Re: Bucket API cleanup issues
Date Tue, 27 Feb 2001 05:24:18 GMT

> 1) The apr_bucket_shared and apr_bucket_simple structs are completely
> useless, and a needless time/resource drain.  If we just add an apr_off_t
> start into the apr_bucket struct, apr_bucket_shared and apr_bucket_simple
> can go away completely, saving massive amounts of work in all of the
> bucket code and making the code vastly simpler and easier to
> read/understand.  That simplicity can easily be seen in that
> apr_bucket_shared_split is reduced to simple_split plus a refcount++ (same
> for shared_copy).  I've renamed simple_split and simple_copy giving them
> the apr_bucket_ prefix and making them public functions called by their
> shared counterparts.  I have this patched, tested, and ready to go.  If
> anyone wants to see the patch, tell me and I'll post it.

+1

> 2) The parameter 'w' (returned length) to apr_bucket_heap_make() and
> apr_bucket_heap_create() is useless.  The returned length is invariant
> from the passed-in length, assuming malloc didn't fail, which we can
> easily determine from the function's return value.  There's no way to get
> *part* of the data made into a heap bucket... malloc just doesn't do
> "partial" allocations.  You've either got enough memory or you dont.
> Those callers that are currently using the parameter generally discard its
> function anyway, seeming to think that they have to use it just because
> it's there.  This patch is done for apr-util but not for Apache.

+1

> 3) pool_bucket_cleanup() is completely bogus AFAICT.  I've added this
> comment to the code, which describes the problems pretty well:
>     /*
>      * XXX: this is fubar... only the *first* apr_bucket to reference
>      * the bucket is changed to a heap bucket... ALL references must
>      * be changed.  So instead of h->b, we need an array of b's in h.
>      * pool_destroy() should remove that bucket from the array.  But
>      * pool_destroy doesn't know where the apr_bucket itself is.
>      * ARRGGGH.  Only solution: apr_bucket_destroy() should let the
>      * e->type->destroy() function destroy the apr_bucket itself, too.
>      * pool_destroy() can remove destroyed apr_bucket referrers from
>      * the array.  Any referrers that remain when this function is
>      * called get converted to heap buckets by adding a loop over the
>      * array here (only the first call to apr_bucket_heap_make() gets
>      * the copy flag set, of course).  Another gotcha, though this one
>      * is easily worked around: apr_bucket_heap_make() calls
>      * apr_bucket_shared_make() which invalidates the reference count
>      * on the refcounted bucket. --jcw
>      */
>
> 4) The same problem applies to file buckets that have been split/copied
> when APR_HAS_MMAP: when one of them gets read it gets made into an MMAP.
> ALL of the file buckets should be converted at the same time to reference
> the same MMAP.

I disagree about how to fix this.  The simple solution is to just abstract
on level, such as:

  bucket   ->     bucket   ->   bucket
    |               |             |
  shared          shared	shared
    |               |             |
     -----------------------------
                    |
                pool_bucket

Same works for file_buckets, and it is why simple_buckets were invented
IIRC.  Of course if this is done, then heap buckets need to have the same
general setup.  This keeps us from having to traverse a list of buckets
whenever we convert from one type to another.

> 5) apr_bucket_destroy() should allow the type-specific destructor to free
> the bucket itself, too (needed to solve problem #3).

No.  This was done on purpose so that we can cache the buckets themselves.
If the type-specific destructor frees the bucket itself, then we can't
cache the freed buckets.  It also makes it very difficult to convert
buckets from one type to the next.

> 6) Should mmap_destroy() call apr_mmap_delete(m->mmap) after when the last
> reference to the mmap bucket is destroyed?

No.  If you do that, then caching MMAP's becomes impossible.  The
apr_mmap_delete should be left up to the pool cleanup.

> 7) socket_read() and pipe_read() should return APR_SUCCESS even in an EOF
> condition, as discussed on new-httpd a week or so ago.  I haven't made
> this change yet, though I have put an XXX comment in the code about it,
> because it requres fixing the input filters in Apache to figure out EOS on
> their own.  Looking for suggestions on how to do that.

Didn't we decide that buckets should return EOF?  I could be wrong, but I
was under the impression that buckets needed to return EOF in order for
things to work.

> 8) Can I make apr_bucket_shared_destroy() just return a boolean value
> indicating whether or not this bucket is the last reference to the shared
> resource?  It's silly to return a pointer to that resource, since the
> caller must already know where the resource is anyway.

+1

> 9) file_read in the non-mmap case was allocating HUGE_STRING_LEN even if
> the file was smaller than that. Fixed and thrown in with patch #1.

+1

> 10) socket_read and pipe_read should modify the heap bucket they create to
> have alloc_len == HUGE_STRING_LEN to reflect the actual size of buf. Fixed
> and thrown in with patch #1.

+1

> PHEW!  I think that's it for now.  Comments, please?

Each of these should be a separate patch, and preferably with a few hours
in between each patch.

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------


Mime
View raw message