On Mar 18, 2009, at 9:18 PM, Greg Stein wrote:
> 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?
First let me say that the API completely blows. The fact that we have
an *external* API which is so implementation dependent is mind-
boggling. The entries cache is documented as just that...a cache.
But the behavior we are discovering is anything but cache-like,
unfortunately. Why we publicly document the usage of opaque members
of a private struct is beyond me. </rant>
A few more questions:
* Does this have any known effects outside of libsvn_wc? Could it
be possible that this is a theoretical possibility, but of practical
nonimport? (A little part of me hopes so, but doubts it.)
* Does this qualify as errata worthy?
* Hmm, the thought just occurs to me: is there any chance we could
use a subpool here? Just allocate the output data in a subpool of the
access baton pool? ("Sure, but when would you clear it?" says the
other half of my brain.)
I think we both feel the Right Way to do this is an API which includes
a result pool in
which the output entries are allocated. Getting there from here could
be difficult.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1352865
Received on 2009-03-19 04:20:21 CET