atomic spinlock mutex class

Question

This here is the follow-up to this question. I was recommended to implement a Lockable type (similar to std::mutex) that can work with std::lock_guard, std::scoped_lock, etc. instead of unnecessarily writing a class similar to std::lock_guard (thus violating D.R.Y). The result is a class named spinlock_mutex. The below example snippet is very similar to what I'm trying to do in my multithreaded program.

The new MRE (live):

#include <atomic>
#include <mutex>
#include <thread>
#include <stop_token>
#include <vector>
#include <array>
#include <exception>
#include <fmt/core.h>

namespace util
{

class [[ nodiscard ]] spinlock_mutex
{
public:
    using lockable_type = std::atomic_flag;

    [[ nodiscard ]] constexpr
    spinlock_mutex( ) noexcept = default;

    spinlock_mutex( const spinlock_mutex& ) noexcept = delete;
    spinlock_mutex& operator=( const spinlock_mutex& ) noexcept = delete;

    void lock( ) noexcept
    {
        while ( m_lockable.test_and_set( std::memory_order_acquire ) )
        {
            while ( m_lockable.test( std::memory_order_relaxed ) ) { }
        }
    }

    bool try_lock( ) noexcept
    {
        return m_lockable.test_and_set( std::memory_order_acquire );
    }

    void unlock( ) noexcept
    {
        m_lockable.clear( std::memory_order_release );
    }

private:
    lockable_type m_lockable { };
};

}


constinit util::spinlock_mutex mtx { };
constinit std::vector<std::exception_ptr> caught_exceptions { };

void func( const std::stop_source stop_src, const int base_value ) noexcept
{
    for ( auto counter { 0uz }; !stop_src.stop_requested( ) && counter < 3; ++counter )
    {
        try
        {
            fmt::print( "base value: {} --- {}\n",
                        base_value, counter + static_cast<decltype( counter )>( base_value ) );
        }
        catch ( ... )
        {
            stop_src.request_stop( );
            const std::lock_guard lock { mtx };
            caught_exceptions.emplace_back( std::current_exception( ) );
        }
    }
}

int main( )
{
    { // a scope to limit the lifetime of jthread objects

    std::stop_source stop_src { std::nostopstate };
    std::array<std::jthread, 8> threads { };

    try
    {
        stop_src = std::stop_source { };

        for ( auto value { 0 }; auto& thrd : threads )
        {
            thrd = std::jthread { func, stop_src, value };
            value += 5;
        }
    }
    catch ( const std::exception& ex )
    {
        stop_src.request_stop( );
        fmt::print( "{}\n", ex.what( ) );
    }

    }

    for ( const auto& ex_ptr : caught_exceptions )
    {
        if ( ex_ptr != nullptr )
        {
            try
            {
                std::rethrow_exception( ex_ptr );
            }
            catch ( const std::exception& ex )
            {
                fmt::print( "{}\n", ex.what( ) );
            }
        }
    }
}

I may have to mention that my aim is to inform the threads to return whenever an exception is caught in one thread. This is done using the copies of std::stop_source that are passed to each thread.

So the constinit util::spinlock_mutex mtx { }; will only need to be locked pretty rarely since exceptions are thrown rarely. Is this a valid case to use a spinlock?

With that said, do the above program and its flow control make any sense? I have thought about what could go wrong (e.g. deadlocks, uncaught exceptions, etc.) but nothing worrying caught my attention.

The last for loop is there to handle the exceptions after all the std::jthreads have been destructed. That's also why the jthreads are declared in an inner scope.

Answer 1

No need to explicitly delete the copy constructor

There is no need to delete the copy constructor and copy assignment operators. Since std::atomic_flag itself is non-copyable, your class will by default also be non-copyable.

Consider making try_lock() [[nodiscard]]

You made the class and the constructor itself [[nodiscard]], but it might make sense to apply this to try_lock() as well.

Use std::mutex unless you have a very good reason not to

While the spinlock class itself looks fine, I don't see why you would need to use a spinlock specifically for the use case you have. A std::mutex would do fine. Consider that exceptions will be on the slow path, the potential overhead of std::mutex is not something I would worry about at all.

Also consider that adding something to a std::vector can take a long time, especially if the vector needs to grow its storage. This will mean allocating new memory and copying the old contents over. With a std::mutex, other threads will be put to sleep while this happens. With your spinlock, those threads will waste a lot of CPU cycles.

Also consider that spinlocks work best if every logical thread is associated with its own hardware thread. If however the operating system decides to let two logical threads share the same hardware thread, it means that if one logical thread gets scheduled out while holding the spinlock, and another logical thread gets scheduled in that tries to lock the same spinlock, the second one will waste all of its time slice for nothing, whereas with a std::mutex this waste would not happen.

Avoid locking altogether

Even better than optimizing the type of lock is to not need a lock. If you know you only have 8 threads, you can declare a std::array of 8 caught exceptions, and give each thread a pointer to their own element inside that array. This way they can write to it without needing a lock.

Answer 2

Here is the final version of my spinlock_mutex class:

class [[ nodiscard ]] spinlock_mutex
{
public:
    using lockable_type = std::atomic_flag;

    [[ nodiscard ]] constexpr
    spinlock_mutex( ) noexcept = default;

    void
    lock( ) noexcept
    {
        while ( m_lockable.test_and_set( std::memory_order_acquire ) )
        {
            while ( m_lockable.test( std::memory_order_relaxed ) ) { }
        }
    }

    [[ nodiscard ]] bool
    try_lock( ) noexcept
    {
        return m_lockable.test_and_set( std::memory_order_acquire );
    }

    void
    unlock( ) noexcept
    {
        m_lockable.clear( std::memory_order_release );
    }

private:
    lockable_type m_lockable { };
};

It's not perfect but it can be useful in some cases.