You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Eric Gillespie <ep...@pretzelnet.org> on 2005/02/27 03:55:37 UTC

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

Greg Hudson <gh...@MIT.EDU> writes:

> I have looked into this problem after getting some more information
> from Stephan over IRC, and here are my findings:

Thanks for the excellent findings, Greg.  Upon reading this
message, i realized this problem is probably the strange out of
memory problem our release engineers over here see when doing
weekly release branches.

This problem is what sent me on my last memory usage quest, and
the two resulting changes helped for a while, but the problem
eventually came back.  Unfortunately, we were still dying in
purge_txn, which made no sense to me.  I figured we must have
been coming *very* close to the resource limit somewhere else,
and then actually hitting the limit in purge_txn.  I could never
figure out where, though.

With your findings, i finally knew how to reproduce the problem:
run two cp operations for our tree at the same time.  Sure
enough, i can now reproduce it reliably.  So, i iterpool-ified
tree.c:merge and dag.c:svn_fs_fs__dag_walk_predecessors, and the
problem is solved.

I've tested fairly extensively, and the test suite passes.  If
there are no objections, i'll commit it.



Fix runaway memory usage when auto-merging a large tree.

* 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/dag.c
==================================================================
--- subversion/libsvn_fs_fs/dag.c  (revision 1910)
+++ subversion/libsvn_fs_fs/dag.c  (local)
@@ -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"
@@ -236,29 +237,34 @@
   svn_fs_t *fs = svn_fs_fs__dag_get_fs (node);
   dag_node_t *this_node;
   svn_boolean_t done = FALSE;
+  apr_pool_t *iterpool;
 
+  iterpool = svn_pool_create (pool);
   this_node = node;
   while ((! done) && this_node)
     {
       node_revision_t *noderev;
 
+      svn_pool_clear (iterpool);
+
       /* 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, 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, 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, iterpool));
     }
+  apr_pool_destroy (iterpool);
 
   return SVN_NO_ERROR;
 }
=== subversion/libsvn_fs_fs/tree.c
==================================================================
--- subversion/libsvn_fs_fs/tree.c  (revision 1910)
+++ subversion/libsvn_fs_fs/tree.c  (local)
@@ -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? */
         }
@@ -1724,6 +1728,7 @@
     }
       
   /* For each entry E in source but not in ancestor */
+  svn_pool_clear (iterpool);
   for (hi = apr_hash_first (pool, s_entries); 
        hi; 
        hi = apr_hash_next (hi))
@@ -1734,6 +1739,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 +1754,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 +1768,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 +1776,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 +1784,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


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

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

Posted by Philip Martin <ph...@codematters.co.uk>.
Eric Gillespie <ep...@pretzelnet.org> writes:

> @@ -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;
>  }

Assumming _clear is "better" than _create/_destroy, how about:

          last_iterpool = svn_pool_create (...);
          iterpool = svn_pool_create (...);

          while (...)
            {
               apr_pool_t *tmp_iterpool = last_iterpool;
               last_iterpool = iterpool;
               iterpool = tmp_iterpool;
               svn_pool_clear (iterpool);
               /* Allocations from iterpool will remain valid on the
                  next iteration. */

               ...
            }

          svn_pool_destroy (iterpool);
          svn_pool_destroy (last_iterpool);


-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

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

Posted by Eric Gillespie <ep...@pretzelnet.org>.
Julian Foad <ju...@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

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

Posted by Julian Foad <ju...@btopenworld.com>.
Eric Gillespie wrote:
> I've tested fairly extensively, and the test suite passes.

Excellent.

>  If there are no objections, i'll commit it.

I'm no expert, but I have a concern...

> === subversion/libsvn_fs_fs/dag.c
> ==================================================================
> --- subversion/libsvn_fs_fs/dag.c  (revision 1910)
> +++ subversion/libsvn_fs_fs/dag.c  (local)
> @@ -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"
> @@ -236,29 +237,34 @@
>    svn_fs_t *fs = svn_fs_fs__dag_get_fs (node);
>    dag_node_t *this_node;
>    svn_boolean_t done = FALSE;
> +  apr_pool_t *iterpool;
>  
> +  iterpool = svn_pool_create (pool);
>    this_node = node;
>    while ((! done) && this_node)
>      {
>        node_revision_t *noderev;
>  
> +      svn_pool_clear (iterpool);
> +
>        /* 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, 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, iterpool));

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.

>        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, iterpool));
>      }
> +  apr_pool_destroy (iterpool);
>  
>    return SVN_NO_ERROR;
>  }
> === subversion/libsvn_fs_fs/tree.c
> ==================================================================
> --- subversion/libsvn_fs_fs/tree.c  (revision 1910)
> +++ subversion/libsvn_fs_fs/tree.c  (local)
[...]
> @@ -1724,6 +1728,7 @@
>      }
>        
>    /* For each entry E in source but not in ancestor */
> +  svn_pool_clear (iterpool);

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, as an idiom to try to catch 
accidental use of iterpool outside the loop.  I'm not sure that that is 
worthwhile, but if you think it is, then that's OK with me.

>    for (hi = apr_hash_first (pool, s_entries); 
>         hi; 
>         hi = apr_hash_next (hi))
> @@ -1734,6 +1739,8 @@
>        apr_ssize_t klen;
>        svn_boolean_t s_ancestorof_t = FALSE;
>  
> +      svn_pool_clear (iterpool);
> +

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org