Re: svn commit: r17572 - in trunk/subversion: libsvn_ra_dav libsvn_repos libsvn_subr

[Greg Hudson <gh...@MIT.EDU>]

On Wed, 2005-11-30 at 17:48 -0600, dionisos@tigris.org wrote:
>   Create a subpool to allocate SVN_STREAM_CHUNK_SIZE buffers.  Just before
>   completing successfully, clean the subpool to free the 100kB buffer.

I don't understand the need for this.  If the function is being called
in a loop, the loop should be using an iteration pool.  If the function
is not being called from a loop, 100K is noise.

If the functions return allocated information, that might make it worth
doing large temporary allocations in a subpool.  You didn't include any
symbol names in your log message, so I can't easily tell whether you
modified functions of that nature.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r17572 - in trunk/subversion: libsvn_ra_dav libsvn_repos libsvn_subr

[Erik Huelsmann <eh...@gmail.com>]

On 12/1/05, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> On Thu, 1 Dec 2005, Erik Huelsmann wrote:
>
> > On 12/1/05, Greg Hudson <gh...@mit.edu> wrote:
> > On Wed, 2005-11-30 at 17:48 -0600, dionisos@tigris.org wrote:
> > >   Create a subpool to allocate SVN_STREAM_CHUNK_SIZE buffers.  Just before
> > >   completing successfully, clean the subpool to free the 100kB buffer.
> >
> > I don't understand the need for this.  If the function is being called
> > in a loop, the loop should be using an iteration pool.  If the function
> > is not being called from a loop, 100K is noise.
>
> >This commit is a consequence of lundblad 'complaining' about the fact
> >that I left a buffer allocated in libsvn_subr/subst.c. That buffer was
> >about the same size. So, I looked where else the buffer was allocated
> >and disposed of, moving the allocation to a subpool if it wasn't
> >already.
>
> I was complaining about a case where two such buffers were used by a
> stream object. That stretches the "noise" level a bit too far IMO. Say you
> use a few such streams (not in a loop). Then we start to waste megabytes
> just for too big buffers.
>
> As I've stated before, my preferred fix is to shrink the buffer size used
> from 100K to something more sensible (like 8K or 16K). There is no point
> in using such a large chunk and it has some problems. Then the above
> commit is unnecessary since allocating some kilobytes really *is* noise...

Correct.  Peter expressed the same to me yesterday on irc.  He also
referred to a discussion he had with Tobias Ringstrom about this,
trying to choose a "good" buffer size.  He has pinged Tobias about the
issue again.

Last time, no real action was taken and since I had the change in my
working copy anyway, we have an intermediate solution now.  As soon as
someone finds an efficient yet not-very-big buffer size to use, we can
back out this change and use a different buffer size.

bye,


Erik.

Re: svn commit: r17572 - in trunk/subversion: libsvn_ra_dav libsvn_repos libsvn_subr

["Peter N. Lundblad" <pe...@famlundblad.se>]

On Thu, 1 Dec 2005, Erik Huelsmann wrote:

> On 12/1/05, Greg Hudson <gh...@mit.edu> wrote:
> On Wed, 2005-11-30 at 17:48 -0600, dionisos@tigris.org wrote:
> >   Create a subpool to allocate SVN_STREAM_CHUNK_SIZE buffers.  Just before
> >   completing successfully, clean the subpool to free the 100kB buffer.
>
> I don't understand the need for this.  If the function is being called
> in a loop, the loop should be using an iteration pool.  If the function
> is not being called from a loop, 100K is noise.

>This commit is a consequence of lundblad 'complaining' about the fact
>that I left a buffer allocated in libsvn_subr/subst.c. That buffer was
>about the same size. So, I looked where else the buffer was allocated
>and disposed of, moving the allocation to a subpool if it wasn't
>already.

I was complaining about a case where two such buffers were used by a
stream object. That stretches the "noise" level a bit too far IMO. Say you
use a few such streams (not in a loop). Then we start to waste megabytes
just for too big buffers.

As I've stated before, my preferred fix is to shrink the buffer size used
from 100K to something more sensible (like 8K or 16K). There is no point
in using such a large chunk and it has some problems. Then the above
commit is unnecessary since allocating some kilobytes really *is* noise...

Thanks,
//Peter

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r17572 - in trunk/subversion: libsvn_ra_dav libsvn_repos libsvn_subr

[Erik Huelsmann <eh...@gmail.com>]

On 12/1/05, Greg Hudson <gh...@mit.edu> wrote:
> On Wed, 2005-11-30 at 17:48 -0600, dionisos@tigris.org wrote:
> >   Create a subpool to allocate SVN_STREAM_CHUNK_SIZE buffers.  Just before
> >   completing successfully, clean the subpool to free the 100kB buffer.
>
> I don't understand the need for this.  If the function is being called
> in a loop, the loop should be using an iteration pool.  If the function
> is not being called from a loop, 100K is noise.

This commit is a consequence of lundblad 'complaining' about the fact
that I left a buffer allocated in libsvn_subr/subst.c. That buffer was
about the same size. So, I looked where else the buffer was allocated
and disposed of, moving the allocation to a subpool if it wasn't
already.

> If the functions return allocated information, that might make it worth
> doing large temporary allocations in a subpool.  You didn't include any
> symbol names in your log message, so I can't easily tell whether you
> modified functions of that nature.

No, I didn't modify any of those.

bye,

Erik.