On 18/02/2014 7:16 p.m., Francesco wrote:
>
> On 18 Feb 2014, at 00:44, Alex Rousskov <rouss...@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
