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