Re: [PATCH] debugs-refactor, v1

On 18/02/2014 7:16 p.m., Francesco wrote:
>
> On 18 Feb 2014, at 00:44, Alex Rousskov <rousskov_at_measurement-factory.com> wrote:
>
>> On 02/17/2014 02:46 PM, Kinkie wrote:
>>
<snip>
>>
>>> +static Mutex printlock;
>>
>> Please avoid using phreads if Squid does not need them.
>
> This raises an interesting question: how do I detect if these are needed?
>

Not at all easily.

This being a universal mechanism in Squid I posit that it is either
always needed in here, or never.

>>
>>> +static Mutex printlock;
>> ...
>>> +Debug::Print(const std::string & logline)
>>> {
>> ...
>>> + printlock.lock();
>> ...
>>> + printlock.unlock();
>>> +}
>>
>> AFAICT, your implementation will freeze Squid forever when debugs() is
>> called recursively. If true, this would be a major regression not worth
>> any of the claimed improvements.
>
> It shouldn't, because the Print method (as only used by debugs() is serialized: input is accumulated before calling it, so any recursion should happen before it is invoked. This means that recursive debugs() are called depth-first but that's the way things are already.
>
>> We need debugs() not to crash when used reentrantly, but we do not
>> really want to pay much for "great" reentrant support because reentrant
>> calls are rare and one can usually figure out what happened anyway even
>> if the resulting debugging strings are messy. Strict locking (with
>> waiting) is the wrong solution here.
>
> The locking is only used when outputting, to avoid races mixing output lines. Is it worth it? I can't be sure: the current code has a platform-specific mutex in place on windows only and that's where I took the need from. Advice is welcome.
>
>> Perhaps I am missing some global dependency/context here, but if you
>> want to improve the common-case debugging efficiency and simplify
>> things, please do not force all debugging through mutexes, especially
>> through pthread mutexes. Something went wrong here. Perhaps you are
>> correctly solving the wrong problem, not sure:
>
> Well, the current code uses a very verbose mutex, but on windows only for some reason (see above)
>

Other OS seem not to have had issues. But iCelero were not able to get
Squid to run until it was added and working.
 - Although that said they later had issues with the windows AUFS code
that identified broken FD handling in identical ways to Linux had a year
or so back. Same fix as for Linux foxed that bug. So I'm not as certain
about the main lock anymore.

I will ACK on that "pthread mutexes" though. They are relatively slow
with several library jump calls in the assembly. Use the C++11
std::mutex when available or none.

Amos
Received on Tue Feb 18 2014 - 09:57:05 MST