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

Re: r1557736 - Reduce memory footprint of the svnrdump dump handling...

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Wed, 14 Jan 2015 11:00:21 +0000

Hi Bert.

The second part of this patch -- "Don't use editor pool as scratch pool" -- looks right.

The first part -- "Make file/dir pool within parent" -- looks unnecessary. The editor driver is already passing in suitably scoped pools: a per-directory pool to the 'open_directory' and 'add_directory' methods, and a per-file pool to the 'open_file' and 'add_file' methods. At least the driver *should* be doing so, and by inspection I see that ra_serf's implementation of svn_ra_replay[_range] actually is doing so.

Does it make sense to remove this creation and deletion of our own subpools here?

I noticed this because make_dir_baton's and make_file_baton's doc strings still say "Perform all allocations in POOL" but they don't.

- Julian

> Author: rhuijben
> Date: Mon Jan 13 14:47:16 2014
> New Revision: 1557736
>
> URL: http://svn.apache.org/r1557736
> Log:
> Reduce memory footprint of the svnrdump dump handling by creating subpools
> per file and directory instead of only per revision. Somehow everything was
> prepared, but not finished here.
>
> * subversion/svnrdump/dump_editor.c
>   (make_dir_baton,
>    make_file_baton): Make file/dir pool within parent.
>   (delete_entry): Use parent pool for data in parent.
>   (add_directory,
>    open_directory): Don't use editor pool as scratch pool.
>   (close_directory): Destroy directory pool.
>
>   (add_file,
>    open_file,
>    apply_textdelta): Don't use editor pool as scratch pool.
>   (close_file): Destroy file pool.
>
> Modified:
>     subversion/trunk/subversion/svnrdump/dump_editor.c
>
> Modified: subversion/trunk/subversion/svnrdump/dump_editor.c
> ==============================================================================
> --- subversion/trunk/subversion/svnrdump/dump_editor.c (original)
> +++ subversion/trunk/subversion/svnrdump/dump_editor.c Mon Jan 13 14:47:16 2014
> @@ -180,31 +180,38 @@ make_dir_baton(const char *path,
>    struct dump_edit_baton *eb = edit_baton;
>    struct dir_baton *new_db = apr_pcalloc(pool, sizeof(*new_db));
>    const char *repos_relpath;
> +  apr_pool_t *dir_pool;
>
>    /* Construct the full path of this node. */
>    if (pb)
> -    repos_relpath = svn_relpath_canonicalize(path, pool);
> +    {
> +      dir_pool = svn_pool_create(pb->pool);
> +      repos_relpath = svn_relpath_canonicalize(path, dir_pool);
> +    }
>    else
> -    repos_relpath = "";
> +    {
> +      dir_pool = svn_pool_create(eb->pool);
> +      repos_relpath = "";
> +    }
>
>    /* Strip leading slash from copyfrom_path so that the path is
>       canonical and svn_relpath_join can be used */
>    if (copyfrom_path)
> -    copyfrom_path = svn_relpath_canonicalize(copyfrom_path, pool);
> +    copyfrom_path = svn_relpath_canonicalize(copyfrom_path, dir_pool);
>
>    new_db->eb = eb;
>    new_db->parent_dir_baton = pb;
> -  new_db->pool = pool;
> +  new_db->pool = dir_pool;
>    new_db->repos_relpath = repos_relpath;
>    new_db->copyfrom_path = copyfrom_path
> -                            ? svn_relpath_canonicalize(copyfrom_path, pool)
> +                            ? svn_relpath_canonicalize(copyfrom_path, dir_pool)
>                              : NULL;
>    new_db->copyfrom_rev = copyfrom_rev;
>    new_db->added = added;
>    new_db->written_out = FALSE;
> -  new_db->props = apr_hash_make(pool);
> -  new_db->deleted_props = apr_hash_make(pool);
> -  new_db->deleted_entries = apr_hash_make(pool);
> +  new_db->props = apr_hash_make(dir_pool);
> +  new_db->deleted_props = apr_hash_make(dir_pool);
> +  new_db->deleted_entries = apr_hash_make(dir_pool);
>
>    return new_db;
> }
> @@ -218,14 +225,15 @@ make_file_baton(const char *path,
>                  struct dir_baton *pb,
>                  apr_pool_t *pool)
> {
> -  struct file_baton *new_fb = apr_pcalloc(pool, sizeof(*new_fb));
> +  apr_pool_t *file_pool = svn_pool_create(pb->pool);
> +  struct file_baton *new_fb = apr_pcalloc(file_pool, sizeof(*new_fb));
>
>    new_fb->eb = pb->eb;
>    new_fb->parent_dir_baton = pb;
> -  new_fb->pool = pool;
> -  new_fb->repos_relpath = svn_relpath_canonicalize(path, pool);
> -  new_fb->props = apr_hash_make(pool);
> -  new_fb->deleted_props = apr_hash_make(pool);
> +  new_fb->pool = file_pool;
> +  new_fb->repos_relpath = svn_relpath_canonicalize(path, file_pool);
> +  new_fb->props = apr_hash_make(file_pool);
> +  new_fb->deleted_props = apr_hash_make(file_pool);
>    new_fb->is_copy = FALSE;
>    new_fb->copyfrom_path = NULL;
>    new_fb->copyfrom_rev = SVN_INVALID_REVNUM;
> @@ -663,7 +671,7 @@ delete_entry(const char *path,
>       to the deleted_entries of the parent directory baton.  That way,
>       we can tell (later) an addition from a replacement.  All the real
>       deletions get handled in close_directory().  */
> -  svn_hash_sets(pb->deleted_entries, apr_pstrdup(pb->eb->pool, path),
> pb);
> +  svn_hash_sets(pb->deleted_entries, apr_pstrdup(pb->pool, path), pb);
>
>    return SVN_NO_ERROR;
> }
> @@ -686,7 +694,7 @@ add_directory(const char *path,
>    SVN_ERR(dump_pending(pb->eb, pool));
>
>    new_db = make_dir_baton(path, copyfrom_path, copyfrom_rev, pb->eb,
> -                          pb, TRUE, pb->eb->pool);
> +                          pb, TRUE, pb->pool);
>
>    /* This might be a replacement -- is the path already deleted? */
>    val = svn_hash_gets(pb->deleted_entries, path);
> @@ -738,12 +746,12 @@ open_directory(const char *path,
>      {
>        copyfrom_path = svn_relpath_join(pb->copyfrom_path,
>                                         svn_relpath_basename(path, NULL),
> -                                       pb->eb->pool);
> +                                       pb->pool);
>        copyfrom_rev = pb->copyfrom_rev;
>      }
>
>    new_db = make_dir_baton(path, copyfrom_path, copyfrom_rev, pb->eb, pb,
> -                          FALSE, pb->eb->pool);
> +                          FALSE, pb->pool);
>
>    *child_baton = new_db;
>    return SVN_NO_ERROR;
> @@ -794,7 +802,7 @@ close_directory(void *dir_baton,
>      }
>
>    /* ### should be unnecessary */
> -  apr_hash_clear(db->deleted_entries);
> +  svn_pool_destroy(db->pool);
>
>    return SVN_NO_ERROR;
> }
> @@ -861,7 +869,7 @@ open_file(const char *path,
>      {
>        fb->copyfrom_path = svn_relpath_join(pb->copyfrom_path,
>                                             svn_relpath_basename(path, NULL),
> -                                           pb->eb->pool);
> +                                           pb->pool);
>        fb->copyfrom_rev = pb->copyfrom_rev;
>      }
>
> @@ -954,7 +962,7 @@ apply_textdelta(void *file_baton, const
>
>    /* Record that there's text to be dumped, and its base checksum. */
>    fb->dump_text = TRUE;
> -  fb->base_checksum = apr_pstrdup(eb->pool, base_checksum);
> +  fb->base_checksum = apr_pstrdup(fb->pool, base_checksum);
>
>    return SVN_NO_ERROR;
> }
> @@ -1067,6 +1075,8 @@ close_file(void *file_baton,
>       dump` */
>    SVN_ERR(svn_stream_puts(eb->stream, "\n\n"));
>
> +  svn_pool_clear(fb->pool);
> +
>    return SVN_NO_ERROR;
> }
>
Received on 2015-01-14 12:02:17 CET

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