You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2008/04/21 20:55:58 UTC
Re: svn commit: r30743 - trunk/subversion/libsvn_client
On Mon, Apr 21, 2008 at 1:16 PM, <kf...@tigris.org> wrote:
> Author: kfogel
> Date: Mon Apr 21 13:16:30 2008
> New Revision: 30743
>
> Log:
> Fix a memory leak in remove recursive propget. See thread starting here:
>
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=137529
> From: "Peter Wemm" <pe...@wemm.org>
> To: dev@subversion.tigris.org
> Subject: svn propget -R on the root of a large repository == boom!
> Date: Mon, 21 Apr 2008 02:12:54 -0700
> Message-ID: <e7...@mail.gmail.com>
>
> * subversion/libsvn_client/prop_commands.c
> (remote_propget): Take perm_pool and work_pool arguments, instead of
> just one pool, and use them to manage memory more efficiently.
>
> Modified:
> trunk/subversion/libsvn_client/prop_commands.c
>
> Modified: trunk/subversion/libsvn_client/prop_commands.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/prop_commands.c?pathrev=30743&r1=30742&r2=30743
> ==============================================================================
> --- trunk/subversion/libsvn_client/prop_commands.c Mon Apr 21 12:12:51 2008 (r30742)
> +++ trunk/subversion/libsvn_client/prop_commands.c Mon Apr 21 13:16:30 2008 (r30743)
> @@ -628,7 +628,9 @@ propget_walk_cb(const char *path,
> * KIND is the kind of the node at "TARGET_PREFIX/TARGET_RELATIVE".
> * Yes, caller passes this; it makes the recursion more efficient :-).
> *
> - * Allocate the keys and values in POOL.
> + * Allocate the keys and values in PERM_POOL, but do all temporary
> + * work in WORK_POOL. The two pools can be the same; recursive
> + * calls may use a different WORK_POOL, however.
> */
> static svn_error_t *
> remote_propget(apr_hash_t *props,
> @@ -639,7 +641,8 @@ remote_propget(apr_hash_t *props,
> svn_revnum_t revnum,
> svn_ra_session_t *ra_session,
> svn_depth_t depth,
> - apr_pool_t *pool)
> + apr_pool_t *perm_pool,
> + apr_pool_t *work_pool)
> {
> apr_hash_t *dirents;
> apr_hash_t *prop_hash;
> @@ -649,41 +652,47 @@ remote_propget(apr_hash_t *props,
> SVN_ERR(svn_ra_get_dir2(ra_session,
> (depth >= svn_depth_files ? &dirents : NULL),
> NULL, &prop_hash, target_relative, revnum,
> - SVN_DIRENT_KIND, pool));
> + SVN_DIRENT_KIND, work_pool));
> }
> else if (kind == svn_node_file)
> {
> SVN_ERR(svn_ra_get_file(ra_session, target_relative, revnum,
> - NULL, NULL, &prop_hash, pool));
> + NULL, NULL, &prop_hash, work_pool));
> }
> else if (kind == svn_node_none)
> {
> return svn_error_createf
> (SVN_ERR_ENTRY_NOT_FOUND, NULL,
> _("'%s' does not exist in revision %ld"),
> - svn_path_join(target_prefix, target_relative, pool), revnum);
> + svn_path_join(target_prefix, target_relative, work_pool), revnum);
> }
> else
> {
> return svn_error_createf
> (SVN_ERR_NODE_UNKNOWN_KIND, NULL,
> _("Unknown node kind for '%s'"),
> - svn_path_join(target_prefix, target_relative, pool));
> + svn_path_join(target_prefix, target_relative, work_pool));
> }
>
> - apr_hash_set(props,
> - svn_path_join(target_prefix, target_relative, pool),
> - APR_HASH_KEY_STRING,
> - apr_hash_get(prop_hash, propname, APR_HASH_KEY_STRING));
> -
> + {
> + svn_string_t *val = apr_hash_get(prop_hash, propname,
> + APR_HASH_KEY_STRING);
> + if (val)
> + val = svn_string_dup(val, perm_pool);
> +
> + apr_hash_set(props,
> + svn_path_join(target_prefix, target_relative, perm_pool),
> + APR_HASH_KEY_STRING, val);
Would it make sense to conditionalize the apr_hash_set on val as well,
so that we don't end up allocating a path in perm_pool for every
single path (even those without the prop)?
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r30743 - trunk/subversion/libsvn_client
Posted by Karl Fogel <kf...@red-bean.com>.
"David Glasser" <gl...@davidglasser.net> writes:
>> + {
>> + svn_string_t *val = apr_hash_get(prop_hash, propname,
>> + APR_HASH_KEY_STRING);
>> + if (val)
>> + val = svn_string_dup(val, perm_pool);
>> +
>> + apr_hash_set(props,
>> + svn_path_join(target_prefix, target_relative, perm_pool),
>> + APR_HASH_KEY_STRING, val);
>
> Would it make sense to conditionalize the apr_hash_set on val as well,
> so that we don't end up allocating a path in perm_pool for every
> single path (even those without the prop)?
Yes -- indeed, I thought about that, and for reasons that I can no
longer even articulate to myself, decided not to do it.
Oh, I remember what it was: I thought that there might be some reason
why that key would *already* be in the hash, so if val were NULL (which
it could have been before this change too) that would cause that key to
be removed, and there might have been some reason why we were depending
on that.
But, on further consideration, the key can't already be there. So
conditionalizing the hash-set on val would only cause a behavior change
if there were a pre-existing bug in the code.
Thanks for the review, will fix.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org