[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Tue, 28 Jun 2011 22:30:14 +0400

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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.