Re: svn commit: r1140512 - in /subversion/trunk/subversion/mod_dav_svn: dav_svn.h liveprops.c

On Tue, Jun 28, 2011 at 06:26, <ivan_at_apache.org> wrote:
> Author: ivan
> Date: Tue Jun 28 10:26:47 2011
> New Revision: 1140512
>
> URL: http://svn.apache.org/viewvc?rev=1140512&view=rev
> Log:
> Reduce memory used by processing PROPFIND requests for specfici
> properties (not allprop).
>
> * subversion/mod_dav_svn/dav_svn.h
>  (dav_resource_private): Add SCRATCH_POOL member. I decided do not use
>   existing POOL, because it's unclear where it should be cleared or
>   destroyed. May be someone with better knowledge fix it in the future.

No way, man. This is one of the most dangerous constructs possible. If
other functions start to use ->scratch_pool, and clear it in the same
fashion, then nobody can rely on using it for their own work. You
simply cannot share a scratch pool through a structure where it might
get cleared by *any* call to another mod_dav function.

> * subversion/mod_dav_svn/liveprops.c
>  (insert_prop): Create or clear SCRATCH_POOL. Use it for temporary
>   allocations.

The correct answer is to pass SCRATCH_POOL to this function. Don't use
the resource pool, but take an extra parameter.

Since that doesn't match the prototype in dav_hooks_liveprop, then you
can have an "internal" version taking the pool parameter, for use by
insert_all, and one for the hooks structure that just passes
resource->info->pool for SCRATCH_POOL.

>  (dav_svn__insert_all_liveprops): Do not create subpool, because
>   insert_prop() already handles this.

Create an iterpool and pass it to the function, rather than injecting
it into the structure.

This code is incredibly old, and doesn't follow very good pool usage.
But we can start improving it using the two-pool parameter mechanism.

A scratch pool in a structure will only lead to future breakage. That
needs to be removed :-(

Cheers,
-g
Received on 2011-06-28 19:01:33 CEST