Re: [PATCH] profiler updates

On 11/06/2012 4:47 a.m., Alex Rousskov wrote:
> On 06/08/2012 07:02 PM, Amos Jeffries wrote:
>> On 8/06/2012 2:46 a.m., Alex Rousskov wrote:
>>> On 06/06/2012 07:25 PM, Amos Jeffries wrote:
>>>>>> + if (delta< head->best)
>>>>>> + head->best = delta;
>>>>>> + if (delta> head->worst)
>>>>>> + head->worst = delta;
>>>>>> + head->summ += delta;
>>>>>> + head->count++;
>>>>>> + head->overheads += get_tick() - stopped_;
>>>>> The above XProfilerNode::stop() code is not thread-safe because
>>>>> multiple
>>>>> threads may update the same "head" and you are not using atomics for
>>>>> *head members.

Here is the patch implementing the above.

>>>> Ok, for now this is the second major TODO.
>>>> I'm torn between GCC __sync_* and C++11 atomic_* types.
>>>>
>>>> There is maybe a wrapper compat layer needed temporarily I think.
>>> Since SMP Squid already uses GCC __sync_* primitives and has the
>>> corresponding wrappers, consider reusing them for your project. See
>>> ipc/AtomicWord.h for a starting point.
>>>
>>> The existing wrappers can be adjusted to use C++11 atomic_* types when
>>> those are provided by the build environment, but that sounds like a very
>>> different project to me.
>> The __sync seem to needs a lot of individual data entry locks and
>> un-locks for this.
> Based on the code you have posted and my recollection of the old
> profiling code, no explicit locks are needed. Perhaps my recollection is
> wrong or your code is different now.
>
> Alex.

As you wrote earlier:
"
The above XProfilerNode::stop() code is not thread-safe because multiple
threads may update the same "head" and you are not using atomics for
*head members.
"

The __sync_* atomics seem to be okay for simple boolean equality test,
add or subtract. But not so much for protecting against
less/greater-than tests or arbitrary alterations.

Here is the patch implementing the spinlocks on *head before multiple
read/compare/change operations.

Amos