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

Re: [PATCH] Re: ugly problem found while trying to test KDE SVN

From: Eric Gillespie <epg_at_pretzelnet.org>
Date: 2005-02-28 00:02:14 CET

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

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