Stefan Fuhrmann <stefan2_at_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
Received on 2014-11-17 19:48:35 CET