mmap_setaside?

[Brian Pane <bp...@pacbell.net>]

Is anybody working on modifying mmap_setaside so that it doesn't
have to memcpy the file contents?  (I recall this being an issue
a while ago.)  This could yield a small but measurable improvement
on requests that aren't handled using sendfile

--Brian

Re: [PATCH] Re: mmap_setaside?

[Cliff Woolley <cl...@yahoo.com>]

On Wed, 14 Nov 2001, Brian Pane wrote:

> apr_mmap_dup() with an optimization for the ancestor-pool case sounds
> good to me.  Actually, with a slight modification, I think this can
> handle the non-ancestor-pool case too:
>   * Create a reference count, in storage outside of any pool.  (I have
>     no major reservations about doing a malloc for this, since it's quick
>     compared to the mmap itself.  If it ever became a bottleneck, we could
>     add a custom free list.)
>   * Set the reference count to 1 when it's created, and increment in
>     apr_mmap_dup().
>   * In mmap_cleanup(), decrement the reference count.  If it's now zero,
>     munmap, and free the struct that holds the reference count.  If the
>     reference count is nonzero, do nothing.

+1!  I like this.  The hackishness of doing all this in mmap_setaside was
making me queasy.  :-)

--Cliff


--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA

Re: [PATCH] Re: mmap_setaside?

[Brian Pane <br...@cnet.com>]

Cliff Woolley wrote:
[...]

>I think the ideal situation would be if we had an apr_mmap_dup() similar
>to our apr_file_dup().  apr_file_dup() already solves this problem for
>files by ensuring that a duplicate copy of the file descriptor lives as
>long as the given pool.  apr_mmap_dup() in the case of duping into an
>ancestor pool could do exactly what we're trying to get mmap_setaside() to
>do: copy the apr_mmap_t() into the ancestor pool and tweak the cleanups.
>In the case of disjoint pools, it could do something else (what I haven't
>decided yet).  Then mmap_setaside() becomes as easy as file_setaside() is.
>
>Thoughts?
>

apr_mmap_dup() with an optimization for the ancestor-pool case sounds
good to me.  Actually, with a slight modification, I think this can
handle the non-ancestor-pool case too:
  * Create a reference count, in storage outside of any pool.  (I have
    no major reservations about doing a malloc for this, since it's quick
    compared to the mmap itself.  If it ever became a bottleneck, we could
    add a custom free list.)
  * Set the reference count to 1 when it's created, and increment in
    apr_mmap_dup().
  * In mmap_cleanup(), decrement the reference count.  If it's now zero,
    munmap, and free the struct that holds the reference count.  If the
    reference count is nonzero, do nothing.

What do you think?

Thanks,
--Brian

Re: [PATCH] Re: mmap_setaside?

[Cliff Woolley <cl...@yahoo.com>]

On Wed, 14 Nov 2001, Brian Pane wrote:

> Actually, the more I think about it, the less I like the idea of
> exposing mmap_cleanup in any form (including the function pointer
> in the struct).  The problem is that it introduces extra coupling
> between the mmap code and the bucket code, so that the latter can
> take advantage of an implementation detail of the former (the fact
> that there happens to be exactly one cleanup function registered
> per apr_mmap_t in its original pool).

I had sort of the same objection, which is why I didn't just go ahead and
do this.  <sigh>

> As an alternate implementation, how about this?
>   apr_status_t apr_pool_cleanup_transfer(apr_pool_t *src, apr_pool_t *dst,
>                                          void *object);

That has problems, too, because it doesn't work right for all types of
objects.  It only happens to work for MMAPs because MMAPs don't have other
stuff allocated from the pool, like internal locks and stuff.  (I believe
this would break horribly if you ran it on an apr_file_t, for example.)

I think the ideal situation would be if we had an apr_mmap_dup() similar
to our apr_file_dup().  apr_file_dup() already solves this problem for
files by ensuring that a duplicate copy of the file descriptor lives as
long as the given pool.  apr_mmap_dup() in the case of duping into an
ancestor pool could do exactly what we're trying to get mmap_setaside() to
do: copy the apr_mmap_t() into the ancestor pool and tweak the cleanups.
In the case of disjoint pools, it could do something else (what I haven't
decided yet).  Then mmap_setaside() becomes as easy as file_setaside() is.

Thoughts?

--Cliff

--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA

Re: [PATCH] Re: mmap_setaside?

[Brian Pane <bp...@pacbell.net>]

Cliff Woolley wrote:

>On Tue, 13 Nov 2001, Brian Pane wrote:
>
>>Thanks for the pointer.  Here's my attempt to work around the problem
>>that you noted in that message--the fact that the apr_mmap_t cleanup
>>function isn't accessible from within mmap_setaside.  My solution was
>>to add a pointer to the cleanup function as an extra field in the
>>apr_mmap_t.
>>
>
>Why a function pointer instead of just exposing mmap_cleanup as a public
>function?  You're essentially making it public anyway through the function
>pointer, and we don't really need polymorphism since there's only one
>mmap_cleanup function for a given APR platform...
>

Actually, the more I think about it, the less I like the idea of
exposing mmap_cleanup in any form (including the function pointer
in the struct).  The problem is that it introduces extra coupling
between the mmap code and the bucket code, so that the latter can
take advantage of an implementation detail of the former (the fact
that there happens to be exactly one cleanup function registered
per apr_mmap_t in its original pool).

As an alternate implementation, how about this?

  /**
   * Transfer all of the registered cleanup callbacks for the specified 
object
   * from one pool to another
   * @param src The pool that currently contains the cleanups for the object
   * @param src The pool to which the cleanups should be moved
   * @remark dst must be an ancestor of src
   */
  apr_status_t apr_pool_cleanup_transfer(apr_pool_t *src, apr_pool_t *dst,
                                         void *object);


If that doesn't bother anybody too much, I'll implement it sometime
later today.

[...]

>One thing, though, shouldn't we be copying to the heap instead of to the
>pool?  That way we can at least free() the thing when we're done with
>it rather than growing the pools to be really huge forever...
>

Using the heap instead makes sense to me--especially since a sufficiently
large pool allocation will often have to alloc from the system heap
anyway.  I'll change that bit of the code to use a heap bucket in the
next iteration of this patch.

--Brian

Re: [PATCH] Re: mmap_setaside?

[Cliff Woolley <cl...@yahoo.com>]

On Tue, 13 Nov 2001, Brian Pane wrote:

> Thanks for the pointer.  Here's my attempt to work around the problem
> that you noted in that message--the fact that the apr_mmap_t cleanup
> function isn't accessible from within mmap_setaside.  My solution was
> to add a pointer to the cleanup function as an extra field in the
> apr_mmap_t.

Why a function pointer instead of just exposing mmap_cleanup as a public
function?  You're essentially making it public anyway through the function
pointer, and we don't really need polymorphism since there's only one
mmap_cleanup function for a given APR platform...

> In this patch, I've only tried to optimize for the case where the
> destination pool is an ancestor of the current pool.  For disjoint
> pools, it will still do the big memcpy.

That's fine.  It's definitely better than what we have now.  I still
haven't found a case where you'd _want_ to setaside into a disjoint pool.
But somebody felt we should leave the option open.  <shrug>

One thing, though, shouldn't we be copying to the heap instead of to the
pool?  That way we can at least free() the thing when we're done with
it rather than growing the pools to be really huge forever...

--Cliff

--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA

[PATCH] Re: mmap_setaside?

[Brian Pane <bp...@pacbell.net>]

Cliff Woolley wrote:

>On Sat, 10 Nov 2001, Brian Pane wrote:
>
>>Is anybody working on modifying mmap_setaside so that it doesn't
>>have to memcpy the file contents?  (I recall this being an issue
>>a while ago.)  This could yield a small but measurable improvement
>>on requests that aren't handled using sendfile
>>
>
>I have worked on it in the past but haven't actively looked at in several
>weeks now.  It's a tricky issue... the last time I looked at it, here's
>what I came up with:
>
>http://marc.theaimsgroup.com/?l=apr-dev&m=100335839408391&w=2
>

Thanks for the pointer.  Here's my attempt to work around the problem that
you noted in that message--the fact that the apr_mmap_t cleanup function
isn't accessible from within mmap_setaside.  My solution was to add a 
pointer
to the cleanup function as an extra field in the apr_mmap_t.

In this patch, I've only tried to optimize for the case where the 
destination
pool is an ancestor of the current pool.  For disjoint pools, it will still
do the big memcpy.

--Brian

[PATCH] Re: mmap_setaside?

[Brian Pane <bp...@pacbell.net>]

Cliff Woolley wrote:

>On Sat, 10 Nov 2001, Brian Pane wrote:
>
>>Is anybody working on modifying mmap_setaside so that it doesn't
>>have to memcpy the file contents?  (I recall this being an issue
>>a while ago.)  This could yield a small but measurable improvement
>>on requests that aren't handled using sendfile
>>
>
>I have worked on it in the past but haven't actively looked at in several
>weeks now.  It's a tricky issue... the last time I looked at it, here's
>what I came up with:
>
>http://marc.theaimsgroup.com/?l=apr-dev&m=100335839408391&w=2
>

Thanks for the pointer.  Here's my attempt to work around the problem that
you noted in that message--the fact that the apr_mmap_t cleanup function
isn't accessible from within mmap_setaside.  My solution was to add a 
pointer
to the cleanup function as an extra field in the apr_mmap_t.

In this patch, I've only tried to optimize for the case where the 
destination
pool is an ancestor of the current pool.  For disjoint pools, it will still
do the big memcpy.

--Brian

Re: mmap_setaside?

[Cliff Woolley <cl...@yahoo.com>]

On Sat, 10 Nov 2001, Brian Pane wrote:

> Is anybody working on modifying mmap_setaside so that it doesn't
> have to memcpy the file contents?  (I recall this being an issue
> a while ago.)  This could yield a small but measurable improvement
> on requests that aren't handled using sendfile

I have worked on it in the past but haven't actively looked at in several
weeks now.  It's a tricky issue... the last time I looked at it, here's
what I came up with:

http://marc.theaimsgroup.com/?l=apr-dev&m=100335839408391&w=2

--Cliff


--------------------------------------------------------------
   Cliff Woolley
   cliffwoolley@yahoo.com
   Charlottesville, VA