Re: Sub-pools for recursion

Julian Foad wrote:
> In copy_versioned_files (in subversion/libsvn_client/export.c) a
> sub-pool is created on entry and destroyed at the end of the function.
> I understand that this is the idiom used for recursive functions.
>
> Within this function, "subpool" is used for most operations but "pool"
> is used for some. As far as I can tell, this is an accident and it is
> hard to spot because the accidental use of a parameter named "pool" is
> so familiar that it doesn't stand out.

Actually, I see now that the sub-pool in this function is serving double duty as a recursion sub-pool and as an iteration sub-pool. This complicates the situation.

As this sub-pool is cleared after each iteration, it cannot be used for data which must live throughout the function, and that is why the passed-in super-pool, "pool", is used for the adm. access. But it can be and is being used for temporary things before the loop begins, as well as for temporary stuff within the loop.

Should the sub-pool be reserved for either one purpose or the other? If so, should we introduce a second sub-pool for the other purpose, as I think is done elsewhere? In practice, there is probably not a great deal of memory being used here, but I would like to determine a "best practice".

Since the recursive call is made from within the iteration, there is really no need for a separate recursion sub-pool in this case, so I think this function should regard its sub-pool as an iteration sub-pool. Everything that is not local to the iteration should be allocated in the passed-in "pool". Does that sound right?

I believe there are definite mis-uses of both "pool" and "sub-pool" in the function. Here is an abbreviated and annotated version of the function; all comments are mine:

copy_versioned_files (..., apr_pool_t *pool)
{
  apr_pool_t *subpool = svn_pool_create (pool);

  SVN_ERR (svn_wc_adm_probe_open (..., pool)); /* OK: needed throughout */
  err = svn_wc_entry (..., subpool); /* ### Wrong: needed throughout */

  if (entry)
    {
      SVN_ERR (svn_io_stat (&finfo, ..., subpool)); /* Safe (used only once), but might as well go in "pool" */

      SVN_ERR (svn_io_get_dirents (&dirents, ..., pool)); /* OK: needed throughout */

      for (hi = apr_hash_first (pool, dirents); hi; hi = apr_hash_next (hi))
        {
           /* "subpool" is used mostly, as expected */
           ...
           SVN_ERR (svn_subst_build_keywords
                    (...,
                     apr_psprintf (pool, ...), /* Safe but should go in "subpool" */
                     ...,
                     subpool));
          ...
          svn_pool_clear (subpool);
        }
  svn_pool_destroy (subpool);
}

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Nov 28 15:32:43 2003