Re: [PATCH] Destroy the APR subpools

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.

-- 
C. Michael Pilato <cmpilato@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand