On Tue, Jun 28, 2011 at 21:01, Greg Stein <gst...@gmail.com> wrote: > On Tue, Jun 28, 2011 at 06:26, <i...@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
