On 17 May 2015 at 17:41, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> wrote: > 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. 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
