Re: [PATCH] why not loop through apr_hash_t with pool as NULL.

On Fri, 06 Oct 2006, Philip Martin wrote:
...
> > Index: subversion/libsvn_fs_base/tree.c
> > ===================================================================
> > --- subversion/libsvn_fs_base/tree.c (revision 21795)
> > +++ subversion/libsvn_fs_base/tree.c (working copy)
> > @@ -1465,8 +1465,7 @@
> > if (table)
> > {
> > apr_hash_index_t *hi;
> > - apr_pool_t *subpool = svn_pool_create(pool);
> > - for (hi = apr_hash_first(subpool, table); hi; hi = apr_hash_next(hi))
> > + for (hi = apr_hash_first(NULL, table); hi; hi = apr_hash_next(hi))
> > {
> > svn_fs_dirent_t *entry;
> > struct node_kind_args nk_args;
>
> The way the subpool is used looks wrong. Why is the subpool used at
> all? Surely the overhead for a single iterator is small enough to
> come out of pool.
>
> If the subpool is required why is it not cleared and destroyed?

There does not appear to be a great need for a sub-pool here, but it
could be leveraged inside the loop:

Index: subversion/libsvn_fs_base/tree.c
===================================================================
--- subversion/libsvn_fs_base/tree.c (revision 21813)
+++ subversion/libsvn_fs_base/tree.c (working copy)
@@ -1472,15 +1472,18 @@
           struct node_kind_args nk_args;
           void *val;

+ svn_pool_clear(subpool);
+
           /* KEY will be the entry name in ancestor (about which we
              simple don't care), VAL the dirent. */
           apr_hash_this(hi, NULL, NULL, &val);
           entry = val;
           nk_args.id = entry->id;
           SVN_ERR(svn_fs_base__retry_txn(fs, txn_body_node_kind, &nk_args,
- pool));
+ subpool));
           entry->kind = nk_args.kind;
         }
+ svn_pool_destroy(subpool);
     }
   else
     {

This may not provide meaningful memory savings -- how expensive is
svn_fs_base__retry_txn()?

This code was injected as part of a FSFS-related branch merge:

------------------------------------------------------------------------
r9572 | ghudson | 2004-04-29 16:17:29 -0700 (Thu, 29 Apr 2004) | 4 lines

Merge the changes from the fs-abstraction branch. This allows the
libsvn_fs_fs filesystem to sit alongside the old BDB filesystem in the
same executable.

------------------------------------------------------------------------

It may've been cribbed from surrounding BDB code in a following
function which has a similar structure.