Page MenuHomePhabricator

Implement shared_mutex re: N4508
ClosedPublic

Authored by mclow.lists on Jun 16 2015, 10:42 AM.

Details

Reviewers
EricWF
Summary

N45088 adds shared_mutex to the set of mutex classes. This is exactly the same as shared_timed_mutex, without the timed bits.

Create a new class __shared_mutex_base with all the non-timed bits in it, and make shared_timed_mutex and shared_mutex use it. Don't inherit from it, because ABI break. Also, leave some stubs in the dylib for the functions that were renamed, because ABI break.

Duplicate the shared_timed_mutex tests, strip out the timed bits, and rename them to use shared_mutex

Diff Detail

Event Timeline

mclow.lists retitled this revision from to Implement shared_mutex re: N4508.
mclow.lists updated this object.
mclow.lists edited the test plan for this revision. (Show Details)
mclow.lists added a reviewer: EricWF.
mclow.lists added a subscriber: Unknown Object (MLST).
EricWF edited edge metadata.Jun 29 2015, 10:53 AM

Why not just implement shared_mutex in terms of shared_timed_mutex instead of factoring out common functionality. I agree that this version is better code but it has a larger ABI impact.

include/shared_mutex
175

Why > 14 and not >= 17? I don't have a preference, just a question.

196

Why are these commented out/not getting committed?

Why not just implement shared_mutex in terms of shared_timed_mutex instead of factoring out common functionality?

I'd rather do it the other way, since shared_timed_mutex is a superset of the functionality of shared_mutex. But I can't, due to ABI concerns.

include/shared_mutex
175

Because we don't know that the next version of the standard will be "17". It could slip to 2018 (say). All you know is that it is post-c++14. FWIW, if you specify -std=c++1z in clang, you get a _LIBCPP_STD_VER that is "15".

196

In the standard, they are optional.

Can apple take this patch since it adds new symbols to the dylib?

include/shared_mutex
196

Why comment them out though? Either we should ship them or remove them.

Can apple take this patch since it adds new symbols to the dylib?

I believe that they're OK with that; I'll give them a heads-up.

include/shared_mutex
196

I'm fine with that.

EricWF accepted this revision.Jun 29 2015, 2:28 PM
EricWF edited edge metadata.

LGTM modulo the concerns with Apple. It seems to me that this could be implemented with no ABI changes if need be, but this change looks good.

This revision is now accepted and ready to land.Jun 29 2015, 2:28 PM

I talked to Doug last night; they haven't shipped a dylib with shared_mutex in it yet, so they have no ABI concerns here.

mclow.lists closed this revision.Jun 30 2015, 7:04 AM

Committed as r241067

I missed this when it went in and coming across the code now I'm quite surprised that it did. Why is shared_mutex not implemented as a wrapper around rwlocks (pthreads and Windows both provide this abstraction)? The current implementation looks a lot less efficient than the system version on any operating system that I'm familiar with and has the added effect that native_handle isn't possible to implement, meaning that these can't easily interoperate with C APIs either.

I don't see a way of fixing this without an ABI break, unfortunately. Perhaps we can provide a better implementation in the __v2 namespace and let users (and platforms that don't care about binary compatibility with older libc++) opt in?

I have recently benchmarked this (platform was RHEL 7 and Haswell), and the libc++ version has about twice the latency of a pthread_rwlock for both classes of locking. libc++ is also about twice the latency of the libstdc++ implementation, because it is just a lightweight wrapper around pthread_rwlocks.