Re: svn commit: r1507044 -

On Fri, Jul 26, 2013 at 1:34 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
>> -----Original Message-----
>> From: Ben Reser [mailto:ben_at_reser.org]
>> Sent: vrijdag 26 juli 2013 04:53
>> To: Bert Huijben
>> Cc: Subversion Development; rschupp_at_apache.org;
>> commits_at_subversion.apache.org
>> Subject: Re: svn commit: r1507044 -
>>
>> On Thu, Jul 25, 2013 at 5:26 PM, Ben Reser <ben_at_reser.org> wrote:
>> > There are far fewer cases where svn_hash_gets() results in extra work.
>> > At first I thought it might not be worth fixing it since you have to
>> > turn it into a function. But then I ran into the cases in libsvn_wc
>> > and libsvn_delta and I think we should fix it as well.
>>
>> So I'm not sure what to do about the svn_hash_gets() case. If I
>> convert it to a function it's no longer optimal for literal strings
>> (unless the compiler is inlining it, which it probably won't be
>> because it's an external function). In which case I might as well
>> just put it back as:
>> #define svn_hash_gets apr_hash_get(ht, key, APR_HASH_KEY_STRING, val)
>>
>> If we don't change it then we need to change all the callers to be
>> careful about not putting expressions that would be an issue when
>> evaluated twice, which is almost certainly going to be done wrong at
>> some point in the future.
>>
>> If we do change it then we'll probably end up using apr_hash_get()
>> with literal strings because we end up with more optimal code in those
>> cases. The upside here is that even if we end up using svn_hash_get()
>> where it would be less optimal to do so, it probably won't be as bad
>> as using svn_hash_gets() as it exists now with a function call as the
>> key expression.
>>
>> Anyone else got some clever idea on how to fix it that I haven't thought
> of?
>
> I think you covered any case I can think of.
>
> I think going back to
> #define svn_hash_gets apr_hash_get(ht, key, APR_HASH_KEY_STRING, val)
> is probably the safest thing to do.
>
> In the original discussions around improving svn_hash_gets with other
> variants for improved performance, we already determined that most of this
> would only be relevant for certain performance critical cases.
>
> This all goes much further than the original idea of simply making it much
> easier to write than the extreme common apr_hash_get(ht, key,
> APR_HASH_KEY_STRING, val).
>
I really like idea go back to simple svn_hash_gets/svn_hash_sets()
implementation and remove these premature optimizations.

-- 
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com