Re: [PATCH] Destroy the APR subpools

On Tue, 2007-04-03 at 15:02 -0400, C. Michael Pilato wrote:
> Hyrum K. Wright wrote:
> > Destroy the APR subpool before every successful return statement.
> >
> > * subversion/svnadmin/main.c
> > (subcommand_rmlocks):
> > * subversion/libsvn_wc/update_editor.c
> > (make_editor):
> > * subversion/svnsync/main.c
> > (do_synchronize):
> > Destroy the APR subpool before the successful return statement.
> > * subversion/libsvn_repos/log.c
> > (get_history): If the history item predates the START revision, then
> > destroy the APR subpool and info->oldpool.
> > * subversion/libsvn_delta/path_driver.c
> > (open_dir): Destroy the APR subpool.
> > (svn_delta_path_driver): Destroy the APR subpool and iteration pool to
> > prevent iterative pool creation.
> >
> > Patch by: bhuvan
> >
> >
> > Index: subversion/svnadmin/main.c
> > ===================================================================
> > --- subversion/svnadmin/main.c (revision 23397)
> > +++ subversion/svnadmin/main.c (working copy)
> > @@ -1202,6 +1202,7 @@
> > svn_pool_clear(subpool);
> > }
> >
> > + svn_pool_destroy(subpool);
> > return SVN_NO_ERROR;
> > }
>
> This looks fine.
>
> >
> > Index: subversion/libsvn_wc/update_editor.c
> > ===================================================================
> > --- subversion/libsvn_wc/update_editor.c (revision 23397)
> > +++ subversion/libsvn_wc/update_editor.c (working copy)
> > @@ -2727,6 +2727,7 @@
> > edit_baton,
> > pool));
> >
> > + svn_pool_destroy(subpool);
> > return SVN_NO_ERROR;
> > }
>
> This, I'm not so sure about. This is an editor factory function. We create
> some hashes and stuff in subpool, but then we destroy the subpool before we
> use the editor? I'm pretty sure that's wrong.
>
>
> > Index: subversion/svnsync/main.c
> > ===================================================================
> > --- subversion/svnsync/main.c (revision 23397)
> > +++ subversion/svnsync/main.c (working copy)
> > @@ -1128,6 +1128,7 @@
> > NULL, subpool));
> > }
> >
> > + svn_pool_destroy(subpool);
> > return SVN_NO_ERROR;
> > }
>
> Good.
>
>
> > Index: subversion/libsvn_repos/log.c
> > ===================================================================
> > --- subversion/libsvn_repos/log.c (revision 23397)
> > +++ subversion/libsvn_repos/log.c (working copy)
> > @@ -289,6 +289,9 @@
> > don't fetch any more for this path. */
> > if (info->history_rev < start)
> > {
> > + svn_pool_destroy(subpool);
> > + if (info->oldpool)
> > + svn_pool_destroy(info->oldpool);
> > info->done = TRUE;
> > return SVN_NO_ERROR;
> > }
>
> This is complicated code, pool-wise, but I think this change is good.
>
> > Index: subversion/libsvn_delta/path_driver.c
> > ===================================================================
> > --- subversion/libsvn_delta/path_driver.c (revision 23397)
> > +++ subversion/libsvn_delta/path_driver.c (working copy)
> > @@ -71,6 +71,7 @@
> > item->pool = subpool;
> > APR_ARRAY_PUSH(db_stack, dir_stack_t *) = item;
> >
> > + svn_pool_destroy(subpool);
> > return SVN_NO_ERROR;
> > }
>
> Nope, not good. We just finished allocating 'item' from subpool, and now
> you're destroying the subpool. That's bad.
>
> > @@ -144,7 +145,11 @@
> >
> > /* Do nothing if there are no paths. */
> > if (! paths->nelts)
> > - return SVN_NO_ERROR;
> > + {
> > + svn_pool_destroy(subpool);
> > + svn_pool_destroy(iterpool);
> > + return SVN_NO_ERROR;
> > + }
> >
> > /* Sort the paths in a depth-first directory-ish order. */
> > qsort(paths->elts, paths->nelts, paths->elt_size, svn_sort_compare_paths);
>
> In this function, it would make more sense to simply not allocate subpool,
> iterpool, and item until after the paths->nelts check. Save us the
> unnecessary alloc/free cycle for two pool and another structure that won't
> even get used.

Thanks for the review comments Mike. Please find attached the revised
patch incorporating your comments. In addition, I've also bumped the
copyright year to 2007 in this patch.

[[
Efficiency the use of APR subpool where necessary, either by destroying
them before the successful return statement or allocating them when it
is used. In addition, bump the copyright year to 2007 where it is
appropriate.

* subversion/svnadmin/main.c
  (subcommand_rmlocks): Destroy the APR subpool before the successful
  return statement.
* subversion/svnsync/main.c
  (do_synchronize): Bump the copyright year to 2007 and destroy the APR
  subpool before the successful return statement.
* subversion/libsvn_repos/log.c
  (get_history): If the history item predates the START revision, then
  destroy the APR subpool and info->oldpool.
* subversion/libsvn_delta/path_driver.c
  (svn_delta_path_driver): Bump the copyright year to 2007 and allocate
  subpool, iterpool and item after the check for paths->nelts.
]]

-- 
Regards,
Bhuvaneswaran