On Fri, Jul 26, 2013 at 1:34 PM, Bert Huijben <b...@qqmail.nl> wrote: >> -----Original Message----- >> From: Ben Reser [mailto:b...@reser.org] >> Sent: vrijdag 26 juli 2013 04:53 >> To: Bert Huijben >> Cc: Subversion Development; rsch...@apache.org; >> comm...@subversion.apache.org >> Subject: Re: svn commit: r1507044 - >> >> On Thu, Jul 25, 2013 at 5:26 PM, Ben Reser <b...@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
