subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Evgeny Kotkov <evgeny.kot...@visualsvn.com>
Subject Re: svn commit: r1639352 - /subversion/trunk/subversion/libsvn_fs_fs/stats.c
Date Mon, 17 Nov 2014 18:47:39 GMT
Stefan Fuhrmann <stefan2@apache.org> writes:

>> What would be the reason to create a subpool here?
>
>
> Thanks for the review, Evgeny!
>
> In this particular case, there is actually a reason: Neither this function
> now its caller owns the SCRATCH_POOL, hence can't clean it. This function
> potentially allocates a large amount of memory before the caller continues
> to recourse into the node-tree. The SUBPOOL shaves off quite a bit of the
> peak memory usage.
>
> Documented in r1639426.
>
>>
>> It looks like this "subpool pattern" is quite common in the code around,
>> but is there any reason for this?  What would happen if we dropped all the
>> subpools / local_pools / file_pools?
>
>
> The code is older than it looks (first committed as fsfs-reorg.c but existed
> before that) and was simply not written with the two-pool paradigm in mind.
> OTOH, memory usage had been critical for fsfs-reorg, so every function tried
> to keep its dynamic usage as low as feasible.
>
>>
>>   # grep svn_pool_create subversion/libsvn_fs_fs/stats.c @ r1632945
>>
>>   apr_pool_t * file_pool = svn_pool_create(pool);
>>   apr_pool_t *iterpool = svn_pool_create(scratch_pool);
>>   apr_pool_t *subpool = svn_pool_create(scratch_pool);
>>   apr_pool_t *subpool = svn_pool_create(scratch_pool);
>>   apr_pool_t *local_pool = svn_pool_create(pool);
>>   apr_pool_t *iterpool = svn_pool_create(local_pool);
>>   apr_pool_t *local_pool = svn_pool_create(pool);
>>   apr_pool_t *iterpool = svn_pool_create(pool);
>>   apr_pool_t *localpool = svn_pool_create(pool);
>
>
> As of r1639549, the whole stats code uses the two-pool paradigm and none of
> the local pools exists anymore, except for the one in get_phys_change_count.

Thanks!

The new code (without the sub- and local-pools) looks much clearer to me.


Regards,
Evgeny Kotkov

Mime
View raw message