You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bhuvaneswaran Arumugam <bh...@collab.net> on 2007/02/15 12:48:50 UTC
[PATCH] Destroy the APR subpools
Hello,
This is a follow-up patch for r23204 for destroying the APR subpools
before every successful return statement. Please review this patch.
Thank you.
[[
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
]]
--
Regards,
Bhuvaneswaran
Re: [PATCH] Destroy the APR subpools
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
On Wed, 2007-04-04 at 09:00 -0400, C. Michael Pilato wrote:
> Bhuvaneswaran Arumugam wrote:
> > 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.
>
> [...]
>
> > @@ -138,14 +138,15 @@
> > int i = 0;
> > void *parent_db = NULL, *db = NULL;
> > const char *path;
> > - apr_pool_t *subpool = svn_pool_create(pool);
> > - apr_pool_t *iterpool = svn_pool_create(pool);
> > - dir_stack_t *item = apr_pcalloc(subpool, sizeof(*item));
> >
> > /* Do nothing if there are no paths. */
> > if (! paths->nelts)
> > return SVN_NO_ERROR;
> >
> > + apr_pool_t *subpool = svn_pool_create(pool);
> > + apr_pool_t *iterpool = svn_pool_create(pool);
> > + dir_stack_t *item = apr_pcalloc(subpool, sizeof(*item));
> > +
> > /* Sort the paths in a depth-first directory-ish order. */
> > qsort(paths->elts, paths->nelts, paths->elt_size, svn_sort_compare_paths);
>
> This won't work in strict C compilers -- you can't have declarations
> following code in a function body. You'll want:
>
> apr_pool_t *subpool, *iterpool;
> dir_stack_t *item;
>
> if (! paths->nelts)
> return SVN_NO_ERROR;
>
> subpool = svn_pool_create(pool);
> iterpool = svn_pool_create(pool);
> item = apr_pcalloc(subpool, sizeof(*item));
>
> With those changes, +1 to commit.
I committed this patch with these changes in r24417. Thank you.
--
Regards,
Bhuvaneswaran
Re: [PATCH] Destroy the APR subpools
Posted by "C. Michael Pilato" <cm...@collab.net>.
Bhuvaneswaran Arumugam wrote:
> 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.
[...]
> @@ -138,14 +138,15 @@
> int i = 0;
> void *parent_db = NULL, *db = NULL;
> const char *path;
> - apr_pool_t *subpool = svn_pool_create(pool);
> - apr_pool_t *iterpool = svn_pool_create(pool);
> - dir_stack_t *item = apr_pcalloc(subpool, sizeof(*item));
>
> /* Do nothing if there are no paths. */
> if (! paths->nelts)
> return SVN_NO_ERROR;
>
> + apr_pool_t *subpool = svn_pool_create(pool);
> + apr_pool_t *iterpool = svn_pool_create(pool);
> + dir_stack_t *item = apr_pcalloc(subpool, sizeof(*item));
> +
> /* Sort the paths in a depth-first directory-ish order. */
> qsort(paths->elts, paths->nelts, paths->elt_size, svn_sort_compare_paths);
This won't work in strict C compilers -- you can't have declarations
following code in a function body. You'll want:
apr_pool_t *subpool, *iterpool;
dir_stack_t *item;
if (! paths->nelts)
return SVN_NO_ERROR;
subpool = svn_pool_create(pool);
iterpool = svn_pool_create(pool);
item = apr_pcalloc(subpool, sizeof(*item));
With those changes, +1 to commit.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Destroy the APR subpools
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
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
Re: [PATCH] Destroy the APR subpools
Posted by "C. Michael Pilato" <cm...@collab.net>.
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 <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Destroy the APR subpools
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Bhuvaneswaran Arumugam wrote:
> On Thu, 2007-02-15 at 18:18 +0530, Bhuvaneswaran Arumugam wrote:
>> Hello,
>>
>> This is a follow-up patch for r23204 for destroying the APR subpools
>> before every successful return statement. Please review this patch.
>> Thank you.
>
> Any review comment for this patch?
If there isn't anything in the next few days, I'll go ahead and file it
in the issue tracker.
-Hyrum
>> [[
>> 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
>> ]]
Re: [PATCH] Destroy the APR subpools
Posted by Bhuvaneswaran Arumugam <bh...@collab.net>.
On Thu, 2007-02-15 at 18:18 +0530, Bhuvaneswaran Arumugam wrote:
> Hello,
>
> This is a follow-up patch for r23204 for destroying the APR subpools
> before every successful return statement. Please review this patch.
> Thank you.
Any review comment for this patch?
>
> [[
> 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
> ]]
--
Regards,
Bhuvaneswaran