Boost C++ Libraries: Ticket #11798: Implementation of boost::shared_mutex on POSIX is suboptimal

owner, status, type, severity changed

viboes — Mon, 16 Nov 2015 22:22:34 GMT

owner changed from Anthony Williams to viboes
status new → assigned
type Bugs → Tasks
severity Problem → Optimization

Hi,

any patch that makes the code more efficient is welcome. Boost.Thread is an old library that doesn't make use of spinlocks.

Ideally, I will prefer a patch that allows to switch to the new optimized version usedg a compiler flag. The reason is that we have a lot of platforms and we need to verify the new code before removing completely the old one.

What do you mean by an initial patch?

Ah, BTW, I've moved this to a task/optimization ticket, as it is not a bug/problem.

alex@… — Mon, 16 Nov 2015 23:46:18 GMT

Sounds good, I'll push a patch. I meant "initial" as I've not committed patches against boost before, so I'll be learning the process as I go :)

The code in the patch will be guarded by compile-time flags.

attachment set

alex@… — Tue, 17 Nov 2015 08:09:37 GMT

attachment → patch.diff

initial patch for pthread_rwlock* implementation

alex@… — Tue, 17 Nov 2015 08:10:21 GMT

A very initial patch is below. Notably, it's lacking a couple things that will prevent this to be production-ready:

Doesn't implement the timeout-based locking public methods. This should be relatively straightforward using pthread_rwlock_timedrdlock(3) and pthread_rwlock_timedwrlock(3).
Doesn't implement any of the upgrade methods. This will be somewhat more convoluted, but perhaps we can leverage the existing boost::mutex to implement those upgrades atomically.
I'm unclear how this would interact with thread interruption. Have not looked into this enough to have insights at this moment.

Also, as for code style wrt per-platform ifdefs, exceptions (this patch throws if the pthread_* methods fail, which is unexpected), etc, I'm open to pointers so that I can polish the patch.

attachment set

alex@… — Tue, 17 Nov 2015 09:43:37 GMT

attachment → patch.2.diff

updated patch

alex@… — Tue, 17 Nov 2015 09:46:17 GMT

Updated the patch with a minor fix.

Also note that as mentioned in the original description, we can alternatively keep most of the existing implementation the same and use a pthread_spin_lock instead of a boost::mutex to guard the internal state. This will make the implementation quite similar to the win32 one.

viboes — Tue, 17 Nov 2015 22:19:40 GMT

Hi,

if you prefer, you can create your own git fork and propose a PR on github. This will allow me to check it without needing to do any merge.

Hmm, I missed that you talked of pthread_rwlock_xxx. I believe that this direction will be incompatible with the whole set of function (interruptions, upgrade, downgrade, ...), but who knows.

alex@… — Tue, 17 Nov 2015 22:55:18 GMT

Was unaware that boost has moved to github - a pleasant surprise :)

WRT the pthread_rwlock_xxx interface - I'm also starting to see that the alternate route (i.e. having a lightweight spin lock instead of a mutex, similar to the win32 implementation) might be more viable. I'll have a PR on github with this approach soon.

viboes — Mon, 11 Jan 2016 18:51:36 GMT

Hi, please, don't provide a patch with a lightweight spin lock for this class. I believe that it would merit another name. BTW, there are already spin lock in Boost (I don't remember if in detail or in smart_ptr).

Wed, 18 May 2016 17:16:17 GMT

I think I just ran into this bug with this repo: https://github.com/silverhammermba/sanity

Basically I'm timing how long it takes a bunch of threads to do concurrent reads on a std::unordered_map. In one case the reads are protected by std::lock_guard<std::mutex>, so each thread has to wait its turn. In the other case, the reads are protected by boost::shared_lock<boost::shared_mutex>, so other than checking in with the shared_mutex the reads should be concurrent.

One would think this is the ideal use-case for a shared lock: lots of reads, no writes. But on my Arch Linux box the shared_lock version is almost twice as slow as a naive mutex. My friend compiled the same code on Windows and saw the exact opposite, with shared_lock being almost twice as fast (as we would hope).

This is not just a matter of optimization, this makes boost::shared_mutex completely useless on Linux. Please upgrade the severity of this report.

viboes — Mon, 08 Aug 2016 23:27:44 GMT

Sorry for been late. I will take a look at asap.

Mon, 29 Aug 2016 14:56:59 GMT

Joining this ticket

I'll be very happy if it will be fixed soon

viboes — Wed, 31 Aug 2016 16:43:25 GMT

Hi, I'm the maintainer of Boost.Thread, but I didn't wrote the code for shared mutex.

Any help on this issue will be recognized by the whole Boost community.

I've no time to work on this issue now.

Please, fork the git repository and prepare a PR.

alex@… — Thu, 01 Sep 2016 06:54:55 GMT

I can take a stab at this.

anonymous — Tue, 21 Feb 2017 08:50:35 GMT

Any progress?

cc set

Andrey Semashev — Mon, 06 Mar 2017 17:35:50 GMT

cc Andrey.Semashev@… added

Nilanjan — Wed, 13 Dec 2017 02:08:35 GMT

Hi, any progress on this?