Re: [VOTE] Merging 1.9-cache-improvements to /trunk

On 17 May 2015 at 17:41, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com> wrote:
> On Thu, May 14, 2015 at 2:37 PM, Branko Čibej <brane_at_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_at_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.
AFAIK structures usually aligned to sizeof(size_t), so using
apr_uint32_t doesn't reduce memory usage.

PS: Thanks for docstring update.

-- 
Ivan Zhakov