On Thu, May 14, 2015 at 2:37 PM, Branko Čibej <br...@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.fuhrm...@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.
