[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r36663 - trunk/subversion/libsvn_wc

From: Greg Stein <gstein_at_gmail.com>
Date: Thu, 19 Mar 2009 03:18:13 +0100

On Wed, Mar 18, 2009 at 22:23, Hyrum K. Wright <hyrum_at_hyrumwright.org> wrote:
>...
> +++ trunk/subversion/libsvn_wc/lock.c   Wed Mar 18 14:23:56 2009        (r36663)
> @@ -85,11 +85,9 @@ struct svn_wc_adm_access_t
>   /* SET_OWNER is TRUE if SET is allocated from this access baton */
>   svn_boolean_t set_owner;
>
> -  /* ENTRIES is the cached entries for PATH, without those in state
> -     deleted. ENTRIES_HIDDEN is the cached entries including those in
> -     state deleted or state absent. Either may be NULL. */
> -  apr_hash_t *entries;
> -  apr_hash_t *entries_hidden;
> +  /* ENTRIES_HIDDEN is all cached entries including those in
> +     state deleted or state absent. It may be NULL. */
> +  apr_hash_t *entries_all;

Comment is wrong. Should ref ENTRIES_ALL.

>...
> +static apr_hash_t *
>  prune_deleted(svn_wc_adm_access_t *adm_access,
> -              apr_pool_t *pool)
> +              apr_pool_t *scratch_pool)
>  {
>...
> +  /* Construct pruned hash without deleted entries */
> +  entries = apr_hash_make(adm_access->pool);

And... BOOM. Every single time some code calls
svn_wc_entries_read(show_hidden=FALSE) this will drop more memory into
adm_access->pool. And that fact is a documented part of the API.
Callers have no incentive to bother caching the value since it is
"known" that multiple calls will not grow that memory pool.

To maintain backwards compatibility, you have to continue using that
pool (not the passed-in pool which is a scratch_pool, and therefore
probably not of the correct lifetime).

I honestly do not see how you're going to get around this.

Now... to some extent if API users are making modifications, then the
old entries will get tossed and the new entries will take their place.
Memory in that pool will grow as a function of *writes* rather than
simply *reads*.

I sympathize, and agree that the old API/behavior/definition blows.
But I don't see much wiggly room here. Do you have some ideas that I'm
missing?

>...

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1352465
Received on 2009-03-19 03:18:30 CET

This is an archived mail posted to the Subversion Dev mailing list.