Julian Foad <julianfoad@btopenworld.com> writes:
> Here, this_node is allocated in iterpool. On the next
> iteration of the loop, iterpool is cleared and then this_node
> is used (not just in the non-NULL test but in the call to
> get_node_revision). That looks wrong.
Good catch. I guess this survives my testing through luck; who
knows. I took inspiration from a construct i noted earlier in
fs_fs.c:get_combined_window, cycling through two pools for the
loop to fix this. Survives my testing, test suite is running
now. I'll commit when it's finished.
> That line appears to be redundant, as the pool is cleared a few
> lines further on, inside the next loop. Maybe you wanted to be
> sure it is cleared here as soon as the first loop has finished
> with it
Nope, it was just a mistake. Thanks.
Fix runaway memory usage when auto-merging a large tree. Before,
a simple cp of a tree of 70,000 files would die when running into
a 512 MB resource limit; now it uses only 8 MB.
* subversion/libsvn_fs_fs/dag.c
(svn_fs_fs__dag_walk_predecessors): Use an iterpool for the loop.
* subversion/libsvn_fs_fs/tree.c
(merge): Use an iterpool for the two loops.
=== subversion/libsvn_fs_fs/tree.c
==================================================================
--- subversion/libsvn_fs_fs/tree.c (/trunk) (revision 1924)
+++ subversion/libsvn_fs_fs/tree.c (/local/pool-cleanup) (revision 1924)
@@ -1300,6 +1300,7 @@
apr_hash_t *s_entries, *t_entries, *a_entries;
apr_hash_index_t *hi;
svn_fs_t *fs;
+ apr_pool_t *iterpool;
/* Make sure everyone comes from the same filesystem. */
fs = svn_fs_fs__dag_get_fs (ancestor);
@@ -1488,6 +1489,7 @@
a_entries = svn_fs_fs__copy_dir_entries (a_entries, pool);
/* for each entry E in a_entries... */
+ iterpool = svn_pool_create (pool);
for (hi = apr_hash_first (pool, a_entries);
hi;
hi = apr_hash_next (hi))
@@ -1498,6 +1500,8 @@
void *val;
apr_ssize_t klen;
+ svn_pool_clear (iterpool);
+
/* KEY will be the entry name in ancestor, VAL the dirent */
apr_hash_this (hi, &key, &klen, &val);
a_entry = val;
@@ -1532,13 +1536,13 @@
else
{
SVN_ERR (id_check_ancestor (&a_ancestorof_t, fs, a_entry->id,
- t_entry->id, pool));
+ t_entry->id, iterpool));
if (a_ancestorof_t)
{
/* this is an &&, so we need both ancestor checks. */
SVN_ERR (id_check_ancestor (&t_ancestorof_s, fs,
t_entry->id, s_entry->id,
- pool));
+ iterpool));
if (t_ancestorof_s)
{
/* This is Case 1. */
@@ -1552,7 +1556,7 @@
{
SVN_ERR (id_check_ancestor (&s_ancestorof_t, fs,
s_entry->id, t_entry->id,
- pool));
+ iterpool));
if (! s_ancestorof_t)
{
/* This is Case 2. */
@@ -1575,7 +1579,7 @@
SVN_ERR (svn_fs_fs__dag_set_entry
(target, t_entry->name, s_entry->id, s_entry->kind,
- txn_id, pool));
+ txn_id, iterpool));
}
/* or if target entry is different from both and
unrelated to source, and all three entries are
@@ -1588,11 +1592,11 @@
svn_node_kind_t s_kind, t_kind, a_kind;
SVN_ERR (svn_fs_fs__dag_get_node (&s_ent_node, fs,
- s_entry->id, pool));
+ s_entry->id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_node (&t_ent_node, fs,
- t_entry->id, pool));
+ t_entry->id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_node (&a_ent_node, fs,
- a_entry->id, pool));
+ a_entry->id, iterpool));
s_kind = svn_fs_fs__dag_node_kind (s_ent_node);
t_kind = svn_fs_fs__dag_node_kind (t_ent_node);
@@ -1605,19 +1609,19 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
a_entry->name,
- pool));
+ iterpool));
}
/* ... just recurse. */
new_tpath = svn_path_join (target_path, t_entry->name,
- pool);
+ iterpool);
SVN_ERR (merge (conflict_p, new_tpath,
t_ent_node, s_ent_node, a_ent_node,
- txn_id, pool));
+ txn_id, iterpool));
SVN_ERR (svn_fs_fs__dag_get_predecessor_count (&pred_count,
s_ent_node,
- pool));
+ iterpool));
/* If target is an immediate descendant of ancestor,
and source is also a descendant of ancestor, we
@@ -1625,7 +1629,7 @@
source. */
SVN_ERR (update_ancestry (fs, s_entry->id,
t_entry->id, txn_id,
- new_tpath, pred_count, pool));
+ new_tpath, pred_count, iterpool));
}
/* Else target entry has changed since ancestor entry,
but it changed either to source entry or to a
@@ -1643,7 +1647,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
a_entry->name,
- pool));
+ iterpool));
}
/* Else if E did not change between ancestor and source,
@@ -1666,7 +1670,7 @@
_("Unexpected immutable node at '%s'"), target_path);
SVN_ERR (svn_fs_fs__dag_delete (target, t_entry->name,
- txn_id, pool));
+ txn_id, iterpool));
/* Seems cleanest to remove it from the target entries
hash now, even though no code would break if we
@@ -1688,7 +1692,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
t_entry->name,
- pool));
+ iterpool));
}
else
{
@@ -1698,8 +1702,8 @@
this change. */
SVN_ERR (undelete_change (fs, svn_path_join (target_path,
t_entry->name,
- pool),
- txn_id, pool));
+ iterpool),
+ txn_id, iterpool));
}
}
/* E exists in neither target nor source */
@@ -1710,8 +1714,8 @@
for that change. */
SVN_ERR (undelete_change (fs, svn_path_join (target_path,
a_entry->name,
- pool),
- txn_id, pool));
+ iterpool),
+ txn_id, iterpool));
/* ### kff todo: what about the rename case? */
}
@@ -1734,6 +1738,8 @@
apr_ssize_t klen;
svn_boolean_t s_ancestorof_t = FALSE;
+ svn_pool_clear (iterpool);
+
apr_hash_this (hi, &key, &klen, &val);
s_entry = val;
t_entry = apr_hash_get (t_entries, key, klen);
@@ -1747,7 +1753,7 @@
if (t_entry)
{
SVN_ERR (id_check_ancestor (&s_ancestorof_t, fs, s_entry->id,
- t_entry->id, pool));
+ t_entry->id, iterpool));
}
/* E does not exist in target */
@@ -1761,7 +1767,7 @@
SVN_ERR (svn_fs_fs__dag_set_entry
(target, s_entry->name, s_entry->id, s_entry->kind,
- txn_id, pool));
+ txn_id, iterpool));
}
/* E exists in target but is different from E in source */
else if (! s_ancestorof_t)
@@ -1769,7 +1775,7 @@
return conflict_err (conflict_p,
svn_path_join (target_path,
t_entry->name,
- pool));
+ iterpool));
/* The remaining case would be: E exists in target and is
* same as in source. This implies a twin add, so target
@@ -1777,7 +1783,8 @@
*/
}
}
-
+ apr_pool_destroy (iterpool);
+
/* All entries in ancestor and source have been accounted for.
*
* Any entry E in target that does not exist in ancestor or source
=== subversion/libsvn_fs_fs/dag.c
==================================================================
--- subversion/libsvn_fs_fs/dag.c (/trunk) (revision 1924)
+++ subversion/libsvn_fs_fs/dag.c (/local/pool-cleanup) (revision 1924)
@@ -23,6 +23,7 @@
#include "svn_error.h"
#include "svn_fs.h"
#include "svn_props.h"
+#include "svn_pools.h"
#include "dag.h"
#include "err.h"
@@ -238,29 +239,39 @@
svn_fs_t *fs = svn_fs_fs__dag_get_fs (node);
dag_node_t *this_node;
svn_boolean_t done = FALSE;
+ apr_pool_t *last_iterpool, *new_iterpool;
+ last_iterpool = svn_pool_create (pool);
this_node = node;
while ((! done) && this_node)
{
node_revision_t *noderev;
+ new_iterpool = svn_pool_create (pool);
+
/* Get the node revision for THIS_NODE so we can examine its
predecessor id. */
- SVN_ERR (get_node_revision (&noderev, this_node, pool));
+ SVN_ERR (get_node_revision (&noderev, this_node, new_iterpool));
+ apr_pool_destroy (last_iterpool);
+
/* If THIS_NODE has a predecessor, replace THIS_NODE with the
precessor, else set it to NULL. */
if (noderev->predecessor_id)
SVN_ERR (svn_fs_fs__dag_get_node (&this_node, fs,
- noderev->predecessor_id, pool));
+ noderev->predecessor_id,
+ new_iterpool));
else
this_node = NULL;
/* Now call the user-supplied callback with our predecessor
node. */
if (callback)
- SVN_ERR (callback (baton, this_node, &done, pool));
+ SVN_ERR (callback (baton, this_node, &done, new_iterpool));
+
+ last_iterpool = new_iterpool;
}
+ apr_pool_destroy (last_iterpool);
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Feb 28 00:03:31 2005