[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
Date: Thu, 13 Nov 2014 18:32:55 +0300

Stefan Fuhrmann <stefan2_at_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
Received on 2014-11-13 16:34:29 CET

This is an archived mail posted to the Subversion Dev mailing list.