You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Eric Gillespie <ep...@pretzelnet.org> on 2004/10/31 23:42:12 UTC
[PATCH] Fix svn_io_remove_dir pool usage
This passes the smoke test of allowing my massive import. If it
passes the test suite and no one objects, i'll commit this.
Memory usage for svn_io_remove_dir was going through the roof when
removing a large directory (e.g. bumping into a 128 MB resource limit
in svn_fs_fs__purge_txn when committing 89482 files). Fix its pool
usage to avoid this.
* subversion/libsvn_subr/io.c
(svn_io_remove_dir): Allocate path_apr and this_dir directly in the
input pool instead of the subpool. Use subpool instead of pool in
the loop, clearing it on each iteration.
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 11677)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -1445,9 +1445,9 @@
instead of just using svn_io_dir_open, because we're going to
need path_apr later anyway when we remove the dir itself. */
- SVN_ERR (svn_path_cstring_from_utf8 (&path_apr, path, subpool));
+ SVN_ERR (svn_path_cstring_from_utf8 (&path_apr, path, pool));
- status = apr_dir_open (&this_dir, path_apr, subpool);
+ status = apr_dir_open (&this_dir, path_apr, pool);
if (status)
return svn_error_wrap_apr (status, _("Can't open directory '%s'"), path);
@@ -1455,6 +1455,7 @@
status == APR_SUCCESS;
status = apr_dir_read (&this_entry, flags, this_dir))
{
+ svn_pool_clear (subpool);
if ((this_entry.filetype == APR_DIR)
&& ((this_entry.name[0] == '.')
&& ((this_entry.name[1] == '\0')
@@ -1470,7 +1471,7 @@
SVN_ERR (svn_path_cstring_to_utf8 (&entry_utf8, this_entry.name,
subpool));
- fullpath = svn_path_join (path, entry_utf8, pool);
+ fullpath = svn_path_join (path, entry_utf8, subpool);
if (this_entry.filetype == APR_DIR)
{
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix svn_io_remove_dir pool usage
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> It looks good to me. I'm kind of surprised we weren't doing it
> before, actually.
>
> You might want to move the subpool allocation to right before the
> for-loop, and its destruction to right after the for-loop, so it's
> clear that its scope is just the loop.
Agreed. Functionally it looks correct. Stylistically you are now using
'subpool' as an iteration pool, so it would be better to do as Karl says and
then rename it to 'iterpool' and don't use it outside the loop. (After the
loop, use the plain 'pool' instead of 'subpool', just as you did before the
loop). I don't believe there is any good reason for using a sub-pool that is
not an iteration pool in that function.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix svn_io_remove_dir pool usage
Posted by kf...@collab.net.
Eric Gillespie <ep...@pretzelnet.org> writes:
> This passes the smoke test of allowing my massive import. If it
> passes the test suite and no one objects, i'll commit this.
>
> Memory usage for svn_io_remove_dir was going through the roof when
> removing a large directory (e.g. bumping into a 128 MB resource limit
> in svn_fs_fs__purge_txn when committing 89482 files). Fix its pool
> usage to avoid this.
It looks good to me. I'm kind of surprised we weren't doing it
before, actually.
You might want to move the subpool allocation to right before the
for-loop, and its destruction to right after the for-loop, so it's
clear that its scope is just the loop.
-Karl
> * subversion/libsvn_subr/io.c
> (svn_io_remove_dir): Allocate path_apr and this_dir directly in the
> input pool instead of the subpool. Use subpool instead of pool in
> the loop, clearing it on each iteration.
>
> Index: subversion/libsvn_subr/io.c
> ===================================================================
> --- subversion/libsvn_subr/io.c (revision 11677)
> +++ subversion/libsvn_subr/io.c (working copy)
> @@ -1445,9 +1445,9 @@
> instead of just using svn_io_dir_open, because we're going to
> need path_apr later anyway when we remove the dir itself. */
>
> - SVN_ERR (svn_path_cstring_from_utf8 (&path_apr, path, subpool));
> + SVN_ERR (svn_path_cstring_from_utf8 (&path_apr, path, pool));
>
> - status = apr_dir_open (&this_dir, path_apr, subpool);
> + status = apr_dir_open (&this_dir, path_apr, pool);
> if (status)
> return svn_error_wrap_apr (status, _("Can't open directory '%s'"), path);
>
> @@ -1455,6 +1455,7 @@
> status == APR_SUCCESS;
> status = apr_dir_read (&this_entry, flags, this_dir))
> {
> + svn_pool_clear (subpool);
> if ((this_entry.filetype == APR_DIR)
> && ((this_entry.name[0] == '.')
> && ((this_entry.name[1] == '\0')
> @@ -1470,7 +1471,7 @@
> SVN_ERR (svn_path_cstring_to_utf8 (&entry_utf8, this_entry.name,
> subpool));
>
> - fullpath = svn_path_join (path, entry_utf8, pool);
> + fullpath = svn_path_join (path, entry_utf8, subpool);
>
> if (this_entry.filetype == APR_DIR)
> {
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org