subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
Subject Re: [VOTE] Merging 1.9-cache-improvements to /trunk
Date Sun, 17 May 2015 14:41:12 GMT
On Thu, May 14, 2015 at 2:37 PM, Branko ─îibej <brane@wandisco.com> wrote:

> On 13.05.2015 19:04, Ivan Zhakov wrote:
> > [adjusting subject to make it valid vote thread]
> >
> > On 13 May 2015 at 19:23, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com>
> wrote:
> >> Hi there,
> >>
> >> Ivan has reviewed my recent membuffer cache
> >> key handling changes, corrected and backported
> >> them on the 1.9-cache-improvements branch.
> >>
> >> I reviewed it and I'm +1 on merging it to /trunk -
> >> hoping we may even get it into 1.9. Since this
> >> touches a sensitive part of the server code, I'd
> >> like to see 2 more +1s for the branch->/trunk
> >> merge.
> >>
> > +1.
> >
> > PS: I think detailed log message will be useful for reviewers. I'll
> > make it tomorrow if you didn't outstrip me.
>
> -0 because:
>
> $ make
> .../subversion/libsvn_subr/cache-membuffer.c:2626:59: warning: implicit
> conversion loses integer
>       precision: 'unsigned long' to 'apr_uint32_t' (aka 'unsigned int')
> [-Wshorten-64-to-32]
>   cache->combined_key.entry_key.key_len = aligned_key_len + prefix_len;
>                                         ~ ~~~~~~~~~~~~~~~~^~~~~~~~~~~~
> .../subversion/libsvn_subr/cache-membuffer.c:2664:58: warning: implicit
> conversion loses integer
>       precision: 'unsigned long' to 'apr_uint32_t' (aka 'unsigned int')
> [-Wshorten-64-to-32]
>       cache->combined_key.entry_key.key_len = prefix_len + 16;
>                                             ~ ~~~~~~~~~~~^~~~
> .../subversion/libsvn_subr/cache-membuffer.c:3161:37: warning: implicit
> conversion loses integer
>       precision: 'apr_size_t' (aka 'unsigned long') to 'apr_uint32_t' (aka
> 'unsigned int') [-Wshorten-64-to-32]
>   cache->prefix.entry_key.key_len = prefix_len;
>                                   ~ ^~~~~~~~~~
> 3 warnings generated.
>

Ugh!

As of r1679863, all item and key sizes are represented
as size_t throughout cache-membuffer.c.

The reason why I first tried sticking with u32 was that
larger entry_t structs mean fewer of them fit into the
index hash. Per group, there are 10 entries on /trunk,
8 on the 1.9-cache-improvements branch and only 7
for the 1.10 equivalent. So, 1.10 will be able to keep
only 30% cache entries than 1.9rc1. Still ok-ish but it
is a tight fit.


> +1 if these warnings get fixed before or as part of the merge without
> adding casts.
>

Even done away with older casts :)

Going to merge to /trunk now with your and Bert's vote.

-- Stefan^2.

Mime
View raw message