On Tue, Jun 28, 2011 at 21:01, Greg Stein <gstein_at_gmail.com> wrote:
> 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.
>
Hi, Greg!
I was expecting your review and comments. Thanks! See my answers below.
> 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.
I understand the risk and that why I added ->scratch_pool instead of
using existing ->pool.
>
>> * 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 will fix only problem problem with
dav_svn__insert_all_liveprops(), but insert_prop() callback will have
unbounded memory usage. And this is serious problem. For example svn
ls -v for directory with 25000 files will consume 250 mb of memory
just to prepare response. My change reduce memory usage to 100mb,
which is also too high but much better.
>
> 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 :-(
>
I agree that code is really old and has crazy pool management and
usage, but I do not see other way to fix without change mod_dav API
and re-implementing PROPFIND handling in mod_dav_svn without using
mod_dav. Re-implementing PROPFIND handler is the best option for me,
but I think it's better to do after 1.7 branch if we decide to go this
way.
--
Ivan Zhakov
Received on 2011-06-28 20:31:11 CEST