Re: svn commit: r1639352 - /subversion/trunk/subversion/libsvn_fs_fs/stats.c

Evgeny Kotkov wrote:
> Stefan Fuhrmann <stefan2_at_apache.org> writes:
>> +static svn_error_t *
>> +get_phys_change_count(query_t *query,
>> +                      revision_info_t *revision_info,
>> +                      apr_pool_t *scratch_pool)
>>  {
>> +  apr_pool_t *subpool = svn_pool_create(scratch_pool);
[...]
>> +  svn_pool_destroy(subpool);
>> +  return SVN_NO_ERROR;
>>  }
>
> What would be the reason to create a subpool here?  AFAIK, this is considered
> to be a bad practice, and there is a corresponding statement in the Subversion
> Community Guide [1]:
>
>   Functions should not create/destroy pools for their operation; they should
>   use a pool provided by the caller. Again, the caller knows more about how the
>   function will be used, how often, how many times, etc. thus, it should be in
>   charge of the function's memory usage.
>
>   For example, the caller might know that the app will exit upon the function's
>   return. Thus, the function would create extra work if it built/destroyed a
>   pool. Instead, it should use the passed-in pool, which the caller is going
>   to be tossing as part of app-exit anyway.

That "example" is spectacularly stupid -- the overhead would be insignificant.

Perhaps a better example is that of a caller calling the function many times in a tight loop, clearing the scratch pool on each iteration. Then the creation of an internal subpool is unnecessary and could be a significant overhead. Should we use that example instead in the community guide?

No comment on whether a sub-pool is useful in the particular cases you've noticed.

- Julian
Received on 2014-11-13 17:43:08 CET