[[
Well, you were wrong on your initial point ("missing zero crossing").
And you have been wrong on the "use the atomic functions" bit because
they don't cover 64 bit integers. The only available option is a critical
section (svn_mutex__t).
]]
The 64 bit integer is for statistics only.
If you just drop that one (that you only used for debugging your code. There
is no single read of that variable) the rest can be an atomic increment.
Bert
From: Stefan Fuhrmann [mailto:stefan.fuhrmann_at_wandisco.com]
Sent: donderdag 13 februari 2014 23:57
To: Ivan Zhakov
Cc: Subversion Development; Stefan Fuhrman
Subject: Re: svn commit: r1567996 -
/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
On Thu, Feb 13, 2014 at 9:02 PM, Ivan Zhakov <ivan_at_visualsvn.com
<mailto:ivan_at_visualsvn.com> > wrote:
On 13 February 2014 23:40, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com
<mailto:stefan.fuhrmann_at_wandisco.com> > wrote:
> On Thu, Feb 13, 2014 at 7:55 PM, Ivan Zhakov <ivan_at_visualsvn.com
<mailto:ivan_at_visualsvn.com> > wrote:
>>
>> On 13 February 2014 22:24, <stefan2_at_apache.org
<mailto:stefan2_at_apache.org> > wrote:
>> > Author: stefan2
>> > Date: Thu Feb 13 18:24:17 2014
>> > New Revision: 1567996
>> >
>> > URL: http://svn.apache.org/r1567996
>
>
>>
>> > +/* Count a hit in ENTRY within CACHE.
>>
>> > + */
>> > +static void
>> > +increment_hit_counters(svn_membuffer_t *cache, entry_t *entry)
>> > +{
>> > + /* To minimize the memory footprint of the cache index, we limit
>> > local
>> > + * hit counters to 32 bits. These may overflow and we must make
sure
>> > that
>> > + * the global sums are still (roughly due to races) the sum of all
>> > local
>> > + * counters. */
>> > + if (++entry->hit_count == 0)
>> > + cache->hit_count -= APR_UINT32_MAX;
>> > + else
>> > + cache->hit_count++;
>> > +
>> As far I understand this counters are updated from several threads
>> without mutex?
>
>
> That is correct.
>
>>
>> In that case we have to use atomic functions to
>> increment them, otherwise we may skip hit_count == 0 in some cases.
>
>
> No.
> Assuming that the system does not split 32 bit read nor write operation
I really, really don't like such assumptions in Subversion code. APR
and OS already provides functions for proper inter process
manipulations and we must use them, instead of saving several cycles.
Well, you were wrong on your initial point ("missing zero crossing").
And you have been wrong on the "use the atomic functions" bit because
they don't cover 64 bit integers. The only available option is a critical
section (svn_mutex__t).
So, I spent a few hours running actual load tests to see how lock
contention in a critical section would play out on a "real" server
machine with high multi-threaded load. And we got lucky (as opposed
to e.g. the APR thread pool code that we currently use in svnserve).
The new code is in r1568062.
-- Stefan^2.
Received on 2014-02-14 00:58:50 CET