On Thu, Nov 13, 2014 at 4:32 PM, Evgeny Kotkov <evgeny.kotkov_at_visualsvn.com>
wrote:
> 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?
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.
-- Stefan^2.
Received on 2014-11-14 00:41:57 CET