Moving sp_thread_pause, sp_thread_sleep to Core
Unordered's concurrent flat map contains a hand-rolled spinlock (graciously provided by Peter Dimov) but it contains a dependency on SmartPtr for the following two headers: #include <boost/smart_ptr/detail/sp_thread_pause.hpp> #include <boost/smart_ptr/detail/sp_thread_sleep.hpp> I advocate that we move these to Core. They're small, self-contained and are useful in broader contexts. Anyone have thoughts or objections? - Christian
On Wednesday, May 31, 2023, Christian Mazakas wrote:
Unordered's concurrent flat map contains a hand-rolled spinlock (graciously provided by Peter Dimov) but it contains a dependency on SmartPtr for the following two headers:
#include <boost/smart_ptr/detail/sp_thread_pause.hpp> #include <boost/smart_ptr/detail/sp_thread_sleep.hpp>
I advocate that we move these to Core. They're small, self-contained and are useful in broader contexts.
Anyone have thoughts or objections?
These are appropriate for Core. No objections from me. Andrey/Peter? Glen
On 6/1/23 03:15, Glen Fernandes via Boost wrote:
On Wednesday, May 31, 2023, Christian Mazakas wrote:
Unordered's concurrent flat map contains a hand-rolled spinlock (graciously provided by Peter Dimov) but it contains a dependency on SmartPtr for the following two headers:
#include <boost/smart_ptr/detail/sp_thread_pause.hpp>
There is a better implementation in Boost.Atomic: https://github.com/boostorg/atomic/blob/2d45635e5b333f116030e3a779511d296c21...
#include <boost/smart_ptr/detail/sp_thread_sleep.hpp>
I advocate that we move these to Core. They're small, self-contained and are useful in broader contexts.
Anyone have thoughts or objections?
These are appropriate for Core. No objections from me. Andrey/Peter?
I would rather have those, along with a spin_mutex implementation, in Boost.Thread, but I realize Peter wouldn't want to depend on it in Boost.SmartPtr. Incidentally, we have a spin_mutex (with another pause implementation) in Boost.Sync. Peter, probably, wouldn't want to depend on it either, but perhaps it would be suitable for other libraries. I could start cleaning it up for a review, time permitting. Re. moving those components to Boost.Core, I don't like it, but I can be overruled.
Andrey Semashev wrote:
These are appropriate for Core. No objections from me. Andrey/Peter?
I would rather have those, along with a spin_mutex implementation, in Boost.Thread, but I realize Peter wouldn't want to depend on it in Boost.SmartPtr.
Incidentally, we have a spin_mutex (with another pause implementation) in Boost.Sync. Peter, probably, wouldn't want to depend on it either, but perhaps it would be suitable for other libraries. I could start cleaning it up for a review, time permitting.
Re. moving those components to Boost.Core, I don't like it, but I can be overruled.
The motivation here is that Unordered has now acquired a dependency on SmartPtr, just for those primitives, which is seen as undesirable. Putting them somewhere else, such as Thread, solves nothing, as a dependency on Thread for those is much worse. The alternative to moving them to Core is Unordered containing its own local copy.
On 6/1/23 15:28, Peter Dimov via Boost wrote:
Andrey Semashev wrote:
These are appropriate for Core. No objections from me. Andrey/Peter?
I would rather have those, along with a spin_mutex implementation, in Boost.Thread, but I realize Peter wouldn't want to depend on it in Boost.SmartPtr.
Incidentally, we have a spin_mutex (with another pause implementation) in Boost.Sync. Peter, probably, wouldn't want to depend on it either, but perhaps it would be suitable for other libraries. I could start cleaning it up for a review, time permitting.
Re. moving those components to Boost.Core, I don't like it, but I can be overruled.
The motivation here is that Unordered has now acquired a dependency on SmartPtr, just for those primitives, which is seen as undesirable.
Putting them somewhere else, such as Thread, solves nothing, as a dependency on Thread for those is much worse.
The alternative to moving them to Core is Unordered containing its own local copy.
That everyone depends on Core doesn't mean we should pile everything in it.
Andrey Semashev wrote:
On 6/1/23 15:28, Peter Dimov via Boost wrote:
The motivation here is that Unordered has now acquired a dependency on SmartPtr, just for those primitives, which is seen as undesirable.
Putting them somewhere else, such as Thread, solves nothing, as a dependency on Thread for those is much worse.
The alternative to moving them to Core is Unordered containing its own local copy.
That everyone depends on Core doesn't mean we should pile everything in it.
Moving an existing implementation detail of library A into Core because library B wants to use it doesn't feel wrong to me. We don't have a better place for things like "a portable implementation of __builtin_pause() if such a thing existed".
Gavin Lambert via Boost Unordered's concurrent flat map
Which is where, exactly? It does not appear to currently exist in the library as far as I can tell, not even in develop...
I apologize for forgetting this. It lives here: https://github.com/boostorg/unordered/tree/feature/cfoa There's also an open PR for this, if you'd like to offer some code review: https://github.com/boostorg/unordered/pull/188
Andrey Semashev via Boost There is a better implementation in Boost.Atomic:
Well, I'm always up for that!
I would rather have those, along with a spin_mutex implementation, in Boost.Thread, but I realize Peter wouldn't want to depend on it in Boost.SmartPtr.
I wouldn't like this either. Right now, SmartPtr is a header-only library and adding a dependency on Thread introduces a binary dependency which means `./vcpkg install boost-smart-ptr` now creates the `libboost_thread.a`. This applies similarly for Unordered as well. I'd prefer to avoid having a binary dependency _just_ for a small header-only component.
That everyone depends on Core doesn't mean we should pile everything in it.
I agree with this, which is why I figured I'd ask the mailing list. Do enough other Boost developers agree that these primitives are universal enough such that they should be in Core? If not, that's fine. There's always vendoring. After all, a little copy-paste is better than a little dependency or sayeth the great Rob Pike. But purporting that we agree they should be somewhere in Boost (and not in a detail header), where should they live? - Christian
On 2/06/2023 04:23, Christian Mazakas wrote:
I wouldn't like this either. Right now, SmartPtr is a header-only library and adding a dependency on Thread introduces a binary dependency which means `./vcpkg install boost-smart-ptr` now creates the `libboost_thread.a`.
Only because dep tools are sufficiently dumb that they can't recognise the difference between header-only and build-required code. Which isn't really their fault, since C++ doesn't make that easy, but still.
I agree with this, which is why I figured I'd ask the mailing list. Do enough other Boost developers agree that these primitives are universal enough such that they should be in Core?
I don't like it, as I said, but it's not like my opinion matters much. :) FWIW though, atomic_* on shared_ptr is entirely banned in my codebase precisely because of its spinlock implementation.
On 2/06/2023 11:43, Peter Dimov wrote:
Gavin Lambert wrote:
FWIW though, atomic_* on shared_ptr is entirely banned in my codebase precisely because of its spinlock implementation.
Which part is wrong?
Caused massive performance issues in my codebase. Not really a fault of your implementation per se (other than it's a spinlock at all), just a consequence of priority inversion plus too many collisions in the spinlock pool. Probably not something that would be a problem for most apps; I just have some special quirks. It's not only spinlocks (though they're worse than regular locks); any locks at all have to justify themselves pretty hard (I use a lot of lock-free techniques and structures, and non-blocking "locks" like strands and workqueues). I have a custom lock that behaves better in my environment, so it's nice when things that do require a lock either leave the app to handle that externally or allow it to be configured via template.
Gavin Lambert wrote:
On 2/06/2023 11:43, Peter Dimov wrote:
Gavin Lambert wrote:
FWIW though, atomic_* on shared_ptr is entirely banned in my codebase precisely because of its spinlock implementation.
Which part is wrong?
Caused massive performance issues in my codebase. Not really a fault of your implementation per se (other than it's a spinlock at all), just a consequence of priority inversion plus too many collisions in the spinlock pool. Probably not something that would be a problem for most apps; I just have some special quirks.
Priority inversion shouldn't be an issue because the SmartPtr spinlock now goes to sp_thread_sleep almost immediately (after just two tries.) The spinlock pool size should probably be increased. 41 is only good for about six cores and this isn't enough in 2023.
Andrey Semashev wrote:
#include <boost/smart_ptr/detail/sp_thread_pause.hpp>
There is a better implementation in Boost.Atomic:
https://github.com/boostorg/atomic/blob/2d45635e5b333f116030e3a779511d296c21...
Indeed. It's better in three ways: implementation for ARM, forceinline, and noexcept. I've applied these to sp_thread_pause (thanks) and in the process submitted a GCC feature request https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110096 I've also applied the noexcept to the other two functions, which lead me to discover an interesting issue with sp_thread_sleep: nanosleep is a POSIX cancelation point, which means that it can "throw": https://github.com/boostorg/core/commit/e088fb89292bb301e69f4b35ba09defb4063... which exposed the need for the following fix: https://github.com/boostorg/core/commit/23ef6d35316be0dfc4e110e701f2fccdeae9... Just thought these observations could be of interest to people here.
On 6/2/23 20:01, Peter Dimov via Boost wrote:
Andrey Semashev wrote:
#include <boost/smart_ptr/detail/sp_thread_pause.hpp>
There is a better implementation in Boost.Atomic:
https://github.com/boostorg/atomic/blob/2d45635e5b333f116030e3a779511d296c21...
Indeed.
It's better in three ways: implementation for ARM, forceinline, and noexcept.
I've applied these to sp_thread_pause (thanks) and in the process submitted a GCC feature request
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110096
I've also applied the noexcept to the other two functions, which lead me to discover an interesting issue with sp_thread_sleep: nanosleep is a POSIX cancelation point, which means that it can "throw":
https://github.com/boostorg/core/commit/e088fb89292bb301e69f4b35ba09defb4063...
which exposed the need for the following fix:
https://github.com/boostorg/core/commit/23ef6d35316be0dfc4e110e701f2fccdeae9...
Just thought these observations could be of interest to people here.
Does cancelling a thread count as a C++ exception, meaning, in particular, does it trigger std::terminate when it leaves a noexcept function? I know they reuse stack unwinding machinery to implement pthread_cleanup, but interacting with C++ constructs like that sounds like a bug. In any case, IMHO, pthread_cancel() should be banned in C++.
Andrey Semashev wrote:
Does cancelling a thread count as a C++ exception, meaning, in particular, does it trigger std::terminate when it leaves a noexcept function?
It does under Linux. Not sure about macOS; I didn't run the test with the noexcept applied but before the fix applied. But even if it doesn't terminate, you still don't want sp_thread_sleep to be a cancelation point because it's typically used in spinlock::lock and you don't want that to be a cancelation point either.
On 1/06/2023 06:42, Christian Mazakas wrote:
Unordered's concurrent flat map
Which is where, exactly? It does not appear to currently exist in the library as far as I can tell, not even in develop...
contains a hand-rolled spinlock (graciously provided by Peter Dimov) but it contains a dependency on SmartPtr for the following two headers:
#include <boost/smart_ptr/detail/sp_thread_pause.hpp> #include <boost/smart_ptr/detail/sp_thread_sleep.hpp>
I advocate that we move these to Core. They're small, self-contained and are useful in broader contexts.
Is there some reason why the spinlock itself shouldn't also be in Core, in that case? Or perhaps Boost.Compat should get a std::shared_mutex equivalent for this purpose instead? I dislike libraries bypassing the standard library for this sort of thing, as it makes it harder to deal with platform quirks or reason about blocking in latency-sensitive code.
participants (5)
-
Andrey Semashev -
Christian Mazakas -
Gavin Lambert -
Glen Fernandes -
Peter Dimov