[PATCH] Change base mutex implementations to use STL-provided mutexes

Aaron Ballman aaron.ballman at gmail.com
Fri Jun 6 11:48:23 PDT 2014


On Fri, Jun 6, 2014 at 12:52 PM, Zachary Turner <zturner at google.com> wrote:
>
>
>
> On Fri, Jun 6, 2014 at 8:52 AM, Aaron Ballman <aaron.ballman at gmail.com>
> wrote:
>>
>> I'm slightly concerned about the overhead from making these functions
>> virtual. Do you happen to have any indication as to how this affects
>> performance? What was wrong with the previous design where the base
>> did not hold concrete data, but instead a void *?
>
>
> I guess I took the inverse line of thought.  Unless there's data to show
> that it is a performance problem, then I preferred to use idiomatic C++.
> I'm not really a fan of type erasure in general, unless you really need it
> to solve a particular sign.

As Reid pointed out, idiomatic really depends on who you talk to. I'd
prefer to understand what all of the implications are, since this is
an existing design that's being changed. That being said, I'm not
opposed to your approach -- merely being cautious.

> Another thing is that this is a mutex.  All of the performance cost
> associated with these methods are going to be that of waiting on the actual
> mutex, and an extra pointer indirection should be negligible.  If there is a
> case where we are locking a mutex in a tight loop, then there are bigger
> issues to solve, such as moving the lock to a higher level.

Generally, this is true, except for all of the single-threaded cases
where there won't ever be contention, but we pay for the indirection
every time anyway. I suspect the indirection isn't incredibly painful,
but as you can see, this is a sweeping change across the codebase so
the implications could be surprising.

>
> I will work on addressing the other issues you pointed out, let me know your
> thoughts on the rationale above.
>
> By the way, there's another approach I could have taken, which is simply to
> delete llvm::sys::Mutex entirely, and embed std::mutex and
> std::recursive_mutex instances directly in the code.  We lose the Debug
> versions of the mutexes, but they seem to be not used heavily, so maybe this
> is ok.  I took the path of least resistance and made the semantics identical
> though, so if you think this other approach is better, I could change it.

I kind of wondered if a future patch may come which then replaces
sys::Mutex with std::mutex, but I think the current patch is the
correct approach for right now. This is sort of how we managed to get
OwningPtr -> std::unique_ptr as well.

Thanks for tackling this!

~Aaron



More information about the llvm-commits mailing list