4
\$\begingroup\$

I'm quite new to C++ and want to focus on writing performant multithreaded code because I will try to port our company internal GUI framework which is currently implemented in C#. So I'd love to get some recommendations about code style and Performance on a reader-writer spin lock that favors writers I wrote:

namespace UI
{
    namespace Threading
    {
        /**
         * \brief ReaderWriterSpinLock which favors writers
         */
        class ReaderWriterSpinLock
        {
        public:
            ReaderWriterSpinLock() = default;
            ~ReaderWriterSpinLock() = default;
            ReaderWriterSpinLock(ReaderWriterSpinLock&) = delete;
            ReaderWriterSpinLock(ReaderWriterSpinLock&&) = delete;
            ReaderWriterSpinLock& operator =(ReaderWriterSpinLock&) = delete;
            ReaderWriterSpinLock& operator =(ReaderWriterSpinLock&&) = delete;

            void AcquireReaderLock()
            {
                int computedValue, initialValue;
                do
                {
                    int count{0};
                    while (m_waitingWriters != 0 || (initialValue = m_lockCount) < 0)
                    {
                        if (++count % MAX_SPIN_COUNT == 0)
                            std::this_thread::yield();
                    }
                    computedValue = initialValue + 1;
                }
                while (!m_lockCount.compare_exchange_strong(initialValue, computedValue));
            }

            void ReleaseReaderLock()
            {
                assert(m_lockCount > 0);
                --m_lockCount;
            }

            void AcquireWriterLock()
            {
                int computedValue, initialValue;
                ++m_waitingWriters;
                do
                {
                    int count{0};
                    while (m_lockCount != 0)
                    {
                        if (++count % MAX_SPIN_COUNT == 0)
                            std::this_thread::yield();
                    }
                    initialValue = 0;
                    computedValue = -1;
                }
                while (!m_lockCount.compare_exchange_strong(initialValue, computedValue));
                --m_waitingWriters;
            }

            void ReleaseWriterLock()
            {
                assert(m_lockCount == -1);
                ++m_lockCount;
            }

        private:
            static constexpr int MAX_SPIN_COUNT = 1000;
            std::atomic<int> m_lockCount{0};
            std::atomic<int> m_waitingWriters{0};
        };
    }
}

Explanation: A negative m_lockCount denotes the active writers (because there can only be one writer at a time the minimum is -1) and a positive count denotes the active readers. When aquiring the reader lock it spins (and yields every MAX_SPIN_COUNT spins) until there are no waiting writers (because it should favor those) and there are no active writers (m_lockCount >= 0), then it tries to update the lock count and repeats the process if it fails. When releasing the lock, the lock count can get decremented without further checks. The methods for the writer lock are quite similar with the difference of temporarily incrementing the m_waitingWriters instead of checking them in the acquire method.

I know that you should normally rely on library functions but in my test it was ten times faster for acquiring the writer lock and twice as fast for readers+writers (with four readers trying to acquire the reader lock in a loop) than the boost::shared_mutex in combination with the boost::shared_lock and boost::unique_lock. So it would actually be worth it.

Also in my application the thread count equals the processor count so the yield should rarely be necessary and the spin lock will only be used for methods which only need a few cycles.

I also wanted to explicitly delete the move and copy constructors because it makes sense for such a class (even though they are deleted implicitly because of the atomic fields). Is this a good idea?

\$\endgroup\$
2
  • \$\begingroup\$ I can't see how this works. You are going to have to write a lot of documentation to convince me that that does anything. Which is probably why it is very fast. \$\endgroup\$ Commented Feb 19, 2019 at 17:25
  • \$\begingroup\$ @MartinYork Fair point. I've added an explanation. Hope this makes it clear but lockfree code is rarely easy to understand. \$\endgroup\$ Commented Feb 19, 2019 at 17:47

1 Answer 1

Reset to default
2
\$\begingroup\$

On Threading

lockfree code is rarely easy to understand.

I would beg to differ. It usually just hides the complex situations.

On Spin Locks.

Spin locks are generally not a good idea. You should be really sure that a thread caught in a spin lock will escape quickly (otherwise you are going to melt your processor).

But you mitigate the problem by using std::this_thread::yield() so this is not such a huge problem. So it is not what I would call a classic spin lock. More a spin with yield.

Guaranteed escape

The only issue I see is that you don't guarantee escape of the lock for a particular thread.

Some particular unlucky thread may be caught trying to obtain a lock while other threads zip passed and keep getting the lock forcing the unlucky thread to keep re-trying to get out of the acquire.

So your spin lock has the potential for some resource starvation (or worst case making the code serial).

This is usually achieved by maintaining an order.

Documentation

You need to put the explanation into the code (as comments). Putting it as an explanation for this site was nice and allowed me to understand the code. But without it the code is undecipherable.

RAII

You have designed your code that requires matched calls to method (this is bad practice). Also you have not show RAII lockers for this class so we must assume its use is not exception safe.

All four of your methods:

        void AcquireReaderLock()
        void ReleaseReaderLock()
        void AcquireWriterLock()
        void ReleaseWriterLock()

Should be private members. You should only allow access to these methods via an RAII lock guard. Look at std::lock for an example.

Best practice

Minor best practice violations:

// One declaration per line
int computedValue, initialValue;

// Why not initialize these on declaration.
initialValue = 0;
computedValue = -1;
// There is no need to ever set them again.
\$\endgroup\$
2
  • \$\begingroup\$ Is the guaranteed escape always important? Because it doesn't add to the total runtime of all threads but just devides the wait time equally at the cost of more instructions. In my case it doesn't really matter if one thread takes longer to acquire the lock. Thanks for all the advice though, it helps a lot! \$\endgroup\$ Commented Feb 19, 2019 at 20:16
  • 1
    \$\begingroup\$ Is guaranteed escape always important: No. Can it be the source of surprising behavior: Yes. You have to way on a case by case basis. Without more information about your use case its impossible to say more. It could be just fine. \$\endgroup\$ Commented Feb 19, 2019 at 21:41

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.