On Thu, Nov 13, 2014 at 4:32 PM, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote:
> 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? 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.
