I happened to notice this open issue that is apparently a very easy task.
Issue #1310: Clear iteration pools at the beginning of each iteration rather
than the end, and destroy them afterwards, as per our policy.
Michael Price attached the latest revision of his patch to the issue last July.
I have updated it and reviewed it carefully. I noticed a few oddities:
+ subversion/libsvn_client/commit.c (svn_client_import):
The patch adds a 'destroy' at the end of the function. Does that invalidate
'commit_info' which seems to be allocated in the subpool and then returned to
the caller? An import seems to work OK, and passes 'make check', even if I
'clear' just before 'destroy', but is it right? I think I will remove it from
the patch if I don't find out for sure that it's safe.
+ subversion/libsvn_fs_fs/fs_fs.c (fetch_all_changes):
Currently, 'iterpool' is cleared part way through the loop body so that data
can be read into it before the loop and at the end of the loop body. I intend
to leave this as it is.
+ Two loops do not use their sub-pool:
subversion/libsvn_client/add.c (svn_client_mkdir): subpool
subversion/mod_dav_svn/repos.c (dav_svn_do_walk): params_subpool
I intend to fix these in a separate patch and not touch them in this one. (Use
the subpool in the first case, where it is currently using the main pool.
Remove the subpool in the second case, because a pool is not needed.)
Any comments on these oddities (especially the first one) or the patch itself
before I commit it?
- Julian
Log message:
[[[
Issue #1310: Clear iteration pools at the beginning of each iteration rather
than the end, and destroy them afterwards, as per our policy.
Patch from Michael Price.
]]]
Issue #1310: Clear iteration pools at the beginning of each iteration rather
than the end, and destroy them afterwards, as per our policy.
Patch from Michael Price.
Index: subversion/libsvn_fs_base/tree.c
===================================================================
--- subversion/libsvn_fs_base/tree.c (revision 12864)
+++ subversion/libsvn_fs_base/tree.c (working copy)
@@ -1591,11 +1591,11 @@
/* KEY will be the entry name, VAL the dirent (about
which we really don't care) */
const void *key;
+ svn_pool_clear (subpool);
apr_hash_this (hi, &key, NULL, NULL);
SVN_ERR (deltify_mutable (fs, root,
svn_path_join (path, key, subpool),
txn_id, subpool));
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/libsvn_fs_base/reps-strings.c
===================================================================
--- subversion/libsvn_fs_base/reps-strings.c (revision 12864)
+++ subversion/libsvn_fs_base/reps-strings.c (working copy)
@@ -1365,6 +1365,9 @@
new_target_baton.size = 0;
new_target_baton.key = NULL;
+ /* Free the window. */
+ svn_pool_clear (wpool);
+
/* Fetch the next window of txdelta data. */
SVN_ERR (svn_txdelta_next_window (&window, txdelta_stream, wpool));
@@ -1383,9 +1386,6 @@
/* Update our recordkeeping variables. */
tview_off += window->tview_len;
diffsize += ww->svndiff_len;
-
- /* Free the window. */
- svn_pool_clear (wpool);
}
} while (window);
Index: subversion/libsvn_fs_base/bdb/changes-table.c
===================================================================
--- subversion/libsvn_fs_base/bdb/changes-table.c (revision 12864)
+++ subversion/libsvn_fs_base/bdb/changes-table.c (working copy)
@@ -265,6 +265,9 @@
change_t *change;
skel_t *result_skel;
+ /* Clear the per-iteration subpool. */
+ svn_pool_clear (subpool);
+
/* RESULT now contains a change record associated with KEY. We
need to parse that skel into an change_t structure ... */
result_skel = svn_fs_base__parse_skel (result.data, result.size,
@@ -321,9 +324,6 @@
db_err = cursor->c_get (cursor, &query, &result, DB_NEXT_DUP);
if (! db_err)
svn_fs_base__track_dbt (&result, trail->pool);
-
- /* Clear the per-iteration subpool. */
- svn_pool_clear (subpool);
}
/* Destroy the per-iteration subpool. */
Index: subversion/svnadmin/main.c
===================================================================
--- subversion/svnadmin/main.c (revision 12864)
+++ subversion/svnadmin/main.c (working copy)
@@ -820,6 +820,8 @@
const char *txn_name_utf8;
svn_error_t *err;
+ svn_pool_clear (subpool);
+
SVN_ERR (svn_utf_cstring_to_utf8 (&txn_name_utf8, txn_name, subpool));
/* Try to open the txn. If that succeeds, try to abort it. */
@@ -851,10 +853,10 @@
(svn_cmdline_printf (subpool, _("Transaction '%s' removed.\n"),
txn_name));
}
-
- svn_pool_clear (subpool);
}
+ svn_pool_destroy (subpool);
+
return SVN_NO_ERROR;
}
Index: subversion/libsvn_wc/entries.c
===================================================================
--- subversion/libsvn_wc/entries.c (revision 12864)
+++ subversion/libsvn_wc/entries.c (working copy)
@@ -1147,6 +1147,8 @@
void *val;
svn_wc_entry_t *this_entry;
+ svn_pool_clear (subpool);
+
/* Get the entry and make sure its attributes are up-to-date. */
apr_hash_this (hi, &key, NULL, &val);
this_entry = val;
@@ -1157,7 +1159,6 @@
/* Append the entry to BIGSTR */
write_entry (&bigstr, this_entry, key, this_dir, subpool);
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/libsvn_wc/log.c
===================================================================
--- subversion/libsvn_wc/log.c (revision 12864)
+++ subversion/libsvn_wc/log.c (working copy)
@@ -1441,6 +1441,8 @@
}
}
+ svn_pool_destroy (iterpool);
+
return SVN_NO_ERROR;
}
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 12864)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -406,6 +406,8 @@
const char *this_path;
svn_wc_adm_access_t *child_access;
+ svn_pool_clear (subpool);
+
apr_hash_this (hi, &key, NULL, &val);
name = key;
current_entry = val;
@@ -431,8 +433,6 @@
(this_path, child_access,
(current_entry->kind == svn_node_dir) ? TRUE : FALSE,
new_revnum, rev_date, rev_author, NULL, subpool));
-
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
@@ -493,6 +493,9 @@
const char *base_name;
svn_wc_entry_t *dup_entry;
+ /* Clear our per-iteration pool. */
+ svn_pool_clear (subpool);
+
/* Get the next entry */
apr_hash_this (hi, &key, NULL, &val);
entry = val;
@@ -533,9 +536,6 @@
svn_wc_notify_state_unknown,
svn_wc_notify_state_unknown,
SVN_INVALID_REVNUM);
-
- /* Clear our per-iteration pool. */
- svn_pool_clear (subpool);
}
/* Handle "this dir" for states that need it done post-recursion. */
@@ -1628,6 +1628,8 @@
const char *keystring;
const char *full_entry_path;
+ svn_pool_clear (subpool);
+
/* Get the next entry */
apr_hash_this (hi, &key, NULL, NULL);
keystring = key;
@@ -1644,8 +1646,6 @@
use_commit_times,
cancel_func, cancel_baton,
notify_func, notify_baton, subpool));
-
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
@@ -1773,6 +1773,8 @@
const char *current_entry_name;
const svn_wc_entry_t *current_entry;
+ svn_pool_clear (subpool);
+
apr_hash_this (hi, &key, NULL, &val);
current_entry = val;
if (! strcmp (key, SVN_WC_ENTRY_THIS_DIR))
@@ -1841,7 +1843,6 @@
return err;
}
}
- svn_pool_clear (subpool);
}
/* At this point, every directory below this one has been
Index: subversion/libsvn_wc/update_editor.c
===================================================================
--- subversion/libsvn_wc/update_editor.c (revision 12864)
+++ subversion/libsvn_wc/update_editor.c (working copy)
@@ -445,6 +445,8 @@
}
}
+ svn_pool_destroy (subpool);
+
/* An atomic write of the whole entries file. */
SVN_ERR (svn_wc__entries_write (entries, adm_access, pool));
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 12864)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -2399,11 +2399,13 @@
/* iteration subpool */
subpool = svn_pool_create (pool);
- for ( ; ; svn_pool_clear (subpool))
+ while (1)
{
const char *name_utf8;
const char *full_path;
+ svn_pool_clear (subpool);
+
apr_err = apr_dir_read (&finfo, wanted, handle);
if (APR_STATUS_IS_ENOENT (apr_err))
break;
Index: subversion/libsvn_ra_local/ra_plugin.c
===================================================================
--- subversion/libsvn_ra_local/ra_plugin.c (revision 12864)
+++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
@@ -861,6 +861,8 @@
svn_fs_dirent_t *fs_entry;
svn_dirent_t *entry = apr_pcalloc (pool, sizeof(*entry));
+ svn_pool_clear (subpool);
+
apr_hash_this (hi, &key, NULL, &val);
entryname = (const char *) key;
fs_entry = (svn_fs_dirent_t *) val;
@@ -892,9 +894,8 @@
/* Store. */
apr_hash_set (*dirents, entryname, APR_HASH_KEY_STRING, entry);
-
- svn_pool_clear (subpool);
}
+ svn_pool_destroy (subpool);
}
/* Handle props if requested. */
Index: subversion/libsvn_client/revert.c
===================================================================
--- subversion/libsvn_client/revert.c (revision 12864)
+++ subversion/libsvn_client/revert.c (working copy)
@@ -96,6 +96,8 @@
{
const char *path = APR_ARRAY_IDX (paths, i, const char *);
+ svn_pool_clear (subpool);
+
/* See if we've been asked to cancel this operation. */
if ((ctx->cancel_func)
&& ((err = ctx->cancel_func (ctx->cancel_baton))))
@@ -122,14 +124,12 @@
else
goto errorful;
}
-
- svn_pool_clear (subpool);
}
- svn_pool_destroy (subpool);
-
errorful:
+ svn_pool_destroy (subpool);
+
/* Sleep to ensure timestamp integrity. */
svn_sleep_for_timestamps ();
Index: subversion/libsvn_client/commit_util.c
===================================================================
--- subversion/libsvn_client/commit_util.c (revision 12864)
+++ subversion/libsvn_client/commit_util.c (working copy)
@@ -460,6 +460,8 @@
const char *this_cf_url = cf_url ? cf_url : copyfrom_url;
svn_wc_adm_access_t *dir_access = adm_access;
+ svn_pool_clear (loop_pool);
+
/* Get the next entry. Name is an entry name; value is an
entry structure. */
apr_hash_this (hi, &key, NULL, &val);
@@ -509,7 +511,6 @@
NULL,
SVN_CLIENT_COMMIT_ITEM_DELETE);
svn_error_clear (lockerr);
- svn_pool_clear (loop_pool);
continue; /* don't recurse! */
}
else
@@ -536,8 +537,6 @@
FALSE,
ctx,
loop_pool));
-
- svn_pool_clear (loop_pool);
}
svn_pool_destroy (loop_pool);
@@ -592,6 +591,7 @@
const svn_wc_entry_t *entry;
const char *target;
+ svn_pool_clear (subpool);
/* Add the relative portion of our full path (if there are no
relative paths, TARGET will just be PARENT_DIR for a single
iteration. */
@@ -680,7 +680,6 @@
FALSE, nonrecursive, ctx, subpool));
i++;
- svn_pool_clear (subpool);
}
while (i < targets->nelts);
@@ -1210,6 +1209,7 @@
svn_boolean_t fulltext = FALSE;
svn_wc_adm_access_t *item_access;
+ svn_pool_clear (subpool);
/* Get the next entry. */
apr_hash_this (hi, &key, &klen, &val);
mod = val;
@@ -1244,8 +1244,6 @@
tempfile = apr_pstrdup (apr_hash_pool_get (*tempfiles), tempfile);
apr_hash_set (*tempfiles, tempfile, APR_HASH_KEY_STRING, (void *)1);
}
-
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/libsvn_client/commit.c
===================================================================
--- subversion/libsvn_client/commit.c (revision 12864)
+++ subversion/libsvn_client/commit.c (working copy)
@@ -656,6 +656,7 @@
&editor, &edit_baton, ctx, url, base_dir,
NULL, log_msg, NULL, commit_info,
FALSE, subpool)));
+ /* subpool is still in use at this point */
/* Reverse the order of the components we added to our NEW_ENTRIES array. */
if (new_entries->nelts)
@@ -704,6 +705,7 @@
return err;
}
+ svn_pool_destroy (subpool);
return SVN_NO_ERROR;
}
Index: subversion/svnlook/main.c
===================================================================
--- subversion/svnlook/main.c (revision 12864)
+++ subversion/svnlook/main.c (working copy)
@@ -1067,12 +1067,12 @@
void *val;
svn_fs_dirent_t *entry;
+ svn_pool_clear (subpool);
apr_hash_this (hi, &key, &keylen, &val);
entry = val;
SVN_ERR (print_tree (root, svn_path_join (path, entry->name, pool),
entry->id, (entry->kind == svn_node_dir),
indentation + 1, show_ids, subpool));
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/mod_dav_svn/liveprops.c
===================================================================
--- subversion/mod_dav_svn/liveprops.c (revision 12864)
+++ subversion/mod_dav_svn/liveprops.c (working copy)
@@ -692,8 +692,8 @@
for (spec = dav_svn_props; spec->name != NULL; ++spec)
{
- (void) dav_svn_insert_prop(resource, spec->propid, what, phdr);
svn_pool_clear(subpool);
+ (void) dav_svn_insert_prop(resource, spec->propid, what, phdr);
}
resource->info->pool = pool;
Index: subversion/mod_dav_svn/repos.c
===================================================================
--- subversion/mod_dav_svn/repos.c (revision 12864)
+++ subversion/mod_dav_svn/repos.c (working copy)
@@ -2088,6 +2088,8 @@
const char *href = name;
svn_boolean_t is_dir = (entry->kind == svn_node_dir);
+ svn_pool_clear(entry_pool);
+
/* append a trailing slash onto the name for directories. we NEED
this for the href portion so that the relative reference will
descend properly. for the visible portion, it is just nice. */
@@ -2121,7 +2123,6 @@
" <%s name=\"%s\" href=\"%s\"></%s>\n",
tag, name, href, tag);
}
- svn_pool_clear(entry_pool);
}
svn_pool_destroy(entry_pool);
Index: subversion/clients/cmdline/checkout-cmd.c
===================================================================
--- subversion/clients/cmdline/checkout-cmd.c (revision 12864)
+++ subversion/clients/cmdline/checkout-cmd.c (working copy)
@@ -112,6 +112,8 @@
const char *true_url;
svn_opt_revision_t peg_revision;
+ svn_pool_clear (subpool);
+
SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
/* Validate the REPOS_URL */
@@ -150,7 +152,6 @@
opt_state->nonrecursive ? FALSE : TRUE,
opt_state->ignore_externals,
ctx, subpool));
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/clients/cmdline/resolved-cmd.c
===================================================================
--- subversion/clients/cmdline/resolved-cmd.c (revision 12864)
+++ subversion/clients/cmdline/resolved-cmd.c (working copy)
@@ -59,6 +59,7 @@
for (i = 0; i < targets->nelts; i++)
{
const char *target = ((const char **) (targets->elts))[i];
+ svn_pool_clear (subpool);
SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
err = svn_client_resolved (target,
opt_state->recursive,
@@ -69,8 +70,6 @@
svn_handle_warning (stderr, err);
svn_error_clear (err);
}
-
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/clients/cmdline/cleanup-cmd.c
===================================================================
--- subversion/clients/cmdline/cleanup-cmd.c (revision 12864)
+++ subversion/clients/cmdline/cleanup-cmd.c (working copy)
@@ -59,9 +59,9 @@
{
const char *target = ((const char **) (targets->elts))[i];
+ svn_pool_clear (subpool);
SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
SVN_ERR (svn_client_cleanup (target, ctx, subpool));
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/clients/cmdline/add-cmd.c
===================================================================
--- subversion/clients/cmdline/add-cmd.c (revision 12864)
+++ subversion/clients/cmdline/add-cmd.c (working copy)
@@ -61,6 +61,7 @@
{
const char *target = ((const char **) (targets->elts))[i];
+ svn_pool_clear (subpool);
SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
err = svn_client_add2 (target, (! opt_state->nonrecursive),
opt_state->force, ctx, subpool);
@@ -74,7 +75,6 @@
else
return err;
}
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/clients/cmdline/switch-cmd.c
===================================================================
--- subversion/clients/cmdline/switch-cmd.c (revision 12864)
+++ subversion/clients/cmdline/switch-cmd.c (working copy)
@@ -68,9 +68,9 @@
for (i = 2; i < targets->nelts; i++)
{
const char *target = ((const char **) (targets->elts))[i];
+ svn_pool_clear (subpool);
SVN_ERR (svn_client_relocate (target, from, to, recurse,
ctx, subpool));
- svn_pool_clear (subpool);
}
}
Index: subversion/clients/cmdline/status-cmd.c
===================================================================
--- subversion/clients/cmdline/status-cmd.c (revision 12864)
+++ subversion/clients/cmdline/status-cmd.c (working copy)
@@ -106,6 +106,8 @@
{
const char *target = ((const char **) (targets->elts))[i];
+ svn_pool_clear (subpool);
+
SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
/* Retrieve a hash of status structures with the information
@@ -122,8 +124,6 @@
opt_state->no_ignore,
opt_state->ignore_externals,
ctx, subpool));
-
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/libsvn_repos/log.c
===================================================================
--- subversion/libsvn_repos/log.c (revision 12864)
+++ subversion/libsvn_repos/log.c (working copy)
@@ -342,6 +342,8 @@
svn_string_t *author, *date, *message;
apr_hash_t *changed_paths = NULL;
+ svn_pool_clear (subpool);
+
/* If we have a list of revs for use, check to make sure this is
one of them. */
if (revs)
@@ -419,8 +421,6 @@
if (++count == limit)
break;
-
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
Index: subversion/libsvn_repos/dump.c
===================================================================
--- subversion/libsvn_repos/dump.c (revision 12864)
+++ subversion/libsvn_repos/dump.c (working copy)
@@ -673,14 +673,14 @@
apr_hash_this (hi, &key, NULL, NULL);
path = key;
+ svn_pool_clear (subpool);
+
/* By sending 'svn_node_unknown', the Node-kind: header simply won't
be written out. No big deal at all, really. The loader
shouldn't care. */
SVN_ERR (dump_node (eb, path,
svn_node_unknown, svn_node_action_delete,
FALSE, NULL, SVN_INVALID_REVNUM, subpool));
-
- svn_pool_clear (subpool);
}
svn_pool_destroy (subpool);
@@ -968,6 +968,8 @@
svn_fs_root_t *to_root;
svn_boolean_t use_deltas_for_rev;
+ svn_pool_clear (subpool);
+
/* Check for cancellation. */
if (cancel_func)
SVN_ERR (cancel_func (cancel_baton));
@@ -1038,8 +1040,6 @@
}
loop_end:
- /* Reuse all memory consumed by the dump of this one revision. */
- svn_pool_clear (subpool);
SVN_ERR (svn_stream_printf (feedback_stream, pool,
_("* %s revision %ld.\n"),
dumping ? "Dumped" : "Verified", to_rev));
Index: subversion/libsvn_repos/delta.c
===================================================================
--- subversion/libsvn_repos/delta.c (revision 12864)
+++ subversion/libsvn_repos/delta.c (working copy)
@@ -959,6 +959,9 @@
const char *s_fullpath;
svn_node_kind_t tgt_kind;
+ /* Clear out our subpool for the next iteration... */
+ svn_pool_clear (subpool);
+
/* KEY is the entry name in target, VAL the dirent */
apr_hash_this (hi, &key, &klen, &val);
t_entry = val;
@@ -1017,9 +1020,6 @@
e_fullpath, tgt_kind, subpool));
}
}
-
- /* Clear out our subpool for the next iteration... */
- svn_pool_clear (subpool);
}
/* All that is left in the source entries hash are things that need
@@ -1035,6 +1035,9 @@
const char *e_fullpath;
svn_node_kind_t src_kind;
+ /* Clear out our subpool for the next iteration... */
+ svn_pool_clear (subpool);
+
/* KEY is the entry name in source, VAL the dirent */
apr_hash_this (hi, &key, &klen, &val);
s_entry = val;
@@ -1044,9 +1047,6 @@
/* Do we actually want to delete the dir if we're non-recursive? */
if (c->recurse || (src_kind != svn_node_dir))
SVN_ERR (delete (c, dir_baton, e_fullpath, subpool));
-
- /* Clear out our subpool for the next iteration... */
- svn_pool_clear (subpool);
}
}
Index: subversion/libsvn_delta/text_delta.c
===================================================================
--- subversion/libsvn_delta/text_delta.c (revision 12864)
+++ subversion/libsvn_delta/text_delta.c (working copy)
@@ -729,14 +729,14 @@
do
{
+ /* free the window (if any) */
+ svn_pool_clear (wpool);
+
/* read in a single delta window */
SVN_ERR( svn_txdelta_next_window (&window, txstream, wpool));
/* shove it at the handler */
SVN_ERR( (*handler)(window, handler_baton));
-
- /* free the window (if any) */
- svn_pool_clear (wpool);
}
while (window != NULL);
Index: subversion/libsvn_ra_dav/props.c
===================================================================
--- subversion/libsvn_ra_dav/props.c (revision 12864)
+++ subversion/libsvn_ra_dav/props.c (working copy)
@@ -1038,9 +1038,9 @@
{
const void *key;
void *val;
+ svn_pool_clear(subpool);
apr_hash_this(hi, &key, NULL, &val);
do_setprop(body, key, val, subpool);
- svn_pool_clear(subpool);
}
ne_buffer_zappend(body, "</D:prop></D:set>");
svn_pool_destroy(subpool);
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 12864)
+++ subversion/svnserve/serve.c (working copy)
@@ -703,6 +703,8 @@
name = key;
fsent = val;
+ svn_pool_clear(subpool);
+
file_path = svn_path_join(full_path, name, subpool);
entry = apr_pcalloc(pool, sizeof(*entry));
@@ -733,7 +735,6 @@
/* Store the entry. */
apr_hash_set(entries, name, APR_HASH_KEY_STRING, entry);
- svn_pool_clear(subpool);
}
svn_pool_destroy(subpool);
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 28 19:37:25 2005