Stefan Fuhrmann <stef...@apache.org> writes:
> URL: http://svn.apache.org/r1639352
> Log:
> Continue work on 'svnfsfs stats' for FSFS f6.
>
> Use the FSFS standard parser to read the changed paths list and get
> the number entries in it.
>
> * subversion/libsvn_fs_fs/stats.c
> (revision_info_t): Remove obsolete member.
> (get_change_count): Replace by ...
> (get_phys_change_count): ... this. Use FSFS standard functions to read
> the change list and count its entries.
> (read_phys_revision): Update caller.
>
> (get_log_change_count): The former get_change_count.
> (read_log_rev_or_packfile): Call it here.
>
[...]
> +static svn_error_t *
> +get_phys_change_count(query_t *query,
> + revision_info_t *revision_info,
> + apr_pool_t *scratch_pool)
> {
> - apr_uint64_t lines = 0;
> - const char *end = changes + len;
> + apr_pool_t *subpool = svn_pool_create(scratch_pool);
> + apr_array_header_t *changes;
>
> - /* line count */
> - for (; changes < end; ++changes)
> - if (*changes == '\n')
> - ++lines;
> + SVN_ERR(svn_fs_fs__get_changes(&changes, query->fs,
> + revision_info->revision, subpool));
> + revision_info->change_count = changes->nelts;
>
> - /* two lines per change */
> - return lines / 2;
> + 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.
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?
# 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);
[1] http://subversion.apache.org/docs/community-guide/conventions#apr-pools
Regards,
Evgeny Kotkov
