From ebed0296b33b4ac300ef54582c25c820d0f30155 Mon Sep 17 00:00:00 2001 From: Matt Kline Date: Sat, 28 Apr 2018 11:54:46 -0800 Subject: [PATCH] Axe atomic int (#4326) * ThreadableJob: Move from AtomicInt to std::atomic This is the first in a series of commits that migrates away from AtomicInt towards the C++11 standard library types. This would allow the removal of AtomicInt and related Qt version-specific code. While we're at it, also make ProcessingState an `enum class` so that it's not implicitly convertible to integers. * LocklessAllocator: Switch from AtomicInt to std::atomic_int If it looks like some assignments around the Compare & Swap loops went missing, it's because compare_exchange_weak() updates the first argument to the current value when it fails (i.e., returns false). * BufferManager: Remove extra AtomicInt include * MixerWorkerThread: AtomicInt to std::atomic_int * NotePlayHandle: AtomicInt to std::atomic_int * Remove AtomicInt.h * Move from QAtomicPointer to std::atomic This removes some #ifdef trickery that was being used to get load-acquire and store-release from Qt5 and "plain" (presumably sequentially-consistent) loads and stores from Qt4. --- include/AtomicInt.h | 45 ---------------------------- include/BufferManager.h | 1 - include/FxMixer.h | 3 +- include/InstrumentPlayHandle.h | 3 +- include/LocklessAllocator.h | 9 +++--- include/LocklessList.h | 37 +++++++++-------------- include/MixerWorkerThread.h | 13 ++++---- include/NotePlayHandle.h | 3 +- include/ThreadableJob.h | 23 +++++++------- src/core/FxMixer.cpp | 12 ++++---- src/core/LocklessAllocator.cpp | 27 +++++++++-------- src/core/MixerWorkerThread.cpp | 10 +++---- src/core/NotePlayHandle.cpp | 8 ++--- src/core/PresetPreviewPlayHandle.cpp | 21 ++++--------- 14 files changed, 77 insertions(+), 138 deletions(-) delete mode 100644 include/AtomicInt.h diff --git a/include/AtomicInt.h b/include/AtomicInt.h deleted file mode 100644 index 3fd564d73..000000000 --- a/include/AtomicInt.h +++ /dev/null @@ -1,45 +0,0 @@ -/// \file AtomicInt.h -/// \brief Compatibility subclass of QAtomicInt for supporting both Qt4 and Qt5 - -#ifndef LMMS_ATOMIC_H -#define LMMS_ATOMIC_H - -#include - -#if QT_VERSION < 0x050300 - -class AtomicInt : public QAtomicInt -{ -public: - AtomicInt( int value = 0 ) : - QAtomicInt( value ) - { - } - - int fetchAndAndOrdered( int valueToAnd ) - { - int value; - do - { - value = (int)*this; - } - while( !testAndSetOrdered( value, value & valueToAnd ) ); - return value; - } - -#if QT_VERSION >= 0x050000 && QT_VERSION < 0x050300 - operator int() const - { - return loadAcquire(); - } -#endif - -}; - -#else - -typedef QAtomicInt AtomicInt; - -#endif // QT_VERSION < 0x050300 - -#endif diff --git a/include/BufferManager.h b/include/BufferManager.h index 845f5fad4..97c553ac9 100644 --- a/include/BufferManager.h +++ b/include/BufferManager.h @@ -26,7 +26,6 @@ #ifndef BUFFER_MANAGER_H #define BUFFER_MANAGER_H -#include "AtomicInt.h" #include "export.h" #include "lmms_basics.h" diff --git a/include/FxMixer.h b/include/FxMixer.h index 3eb47c33a..053480a6a 100644 --- a/include/FxMixer.h +++ b/include/FxMixer.h @@ -30,6 +30,7 @@ #include "JournallingObject.h" #include "ThreadableJob.h" +#include class FxRoute; typedef QVector FxRouteVector; @@ -70,7 +71,7 @@ class FxChannel : public ThreadableJob void unmuteForSolo(); - QAtomicInt m_dependenciesMet; + std::atomic_int m_dependenciesMet; void incrementDeps(); void processed(); diff --git a/include/InstrumentPlayHandle.h b/include/InstrumentPlayHandle.h index 726859237..f027b7d11 100644 --- a/include/InstrumentPlayHandle.h +++ b/include/InstrumentPlayHandle.h @@ -59,7 +59,8 @@ public: for( const NotePlayHandle * constNotePlayHandle : nphv ) { NotePlayHandle * notePlayHandle = const_cast( constNotePlayHandle ); - if( notePlayHandle->state() != ThreadableJob::Done && ! notePlayHandle->isFinished() ) + if( notePlayHandle->state() != ThreadableJob::ProcessingState::Done && + !notePlayHandle->isFinished()) { nphsLeft = true; notePlayHandle->process(); diff --git a/include/LocklessAllocator.h b/include/LocklessAllocator.h index 80f50514e..e7e265680 100644 --- a/include/LocklessAllocator.h +++ b/include/LocklessAllocator.h @@ -25,10 +25,9 @@ #ifndef LOCKLESS_ALLOCATOR_H #define LOCKLESS_ALLOCATOR_H +#include #include -#include "AtomicInt.h" - class LocklessAllocator { public: @@ -43,11 +42,11 @@ private: size_t m_capacity; size_t m_elementSize; - AtomicInt * m_freeState; + std::atomic_int * m_freeState; size_t m_freeStateSets; - AtomicInt m_available; - AtomicInt m_startIndex; + std::atomic_int m_available; + std::atomic_int m_startIndex; } ; diff --git a/include/LocklessList.h b/include/LocklessList.h index 46aa82514..05df56f41 100644 --- a/include/LocklessList.h +++ b/include/LocklessList.h @@ -25,10 +25,10 @@ #ifndef LOCKLESS_LIST_H #define LOCKLESS_LIST_H -#include - #include "LocklessAllocator.h" +#include + template class LocklessList { @@ -39,9 +39,10 @@ public: Element * next; } ; - LocklessList( size_t size ) + LocklessList( size_t size ) : + m_first(nullptr), + m_allocator(new LocklessAllocatorT(size)) { - m_allocator = new LocklessAllocatorT( size ); } ~LocklessList() @@ -53,39 +54,29 @@ public: { Element * e = m_allocator->alloc(); e->value = value; + e->next = m_first.load(std::memory_order_relaxed); - do + while (!m_first.compare_exchange_weak(e->next, e, + std::memory_order_release, + std::memory_order_relaxed)) { -#if QT_VERSION >= 0x050000 - e->next = m_first.loadAcquire(); -#else - e->next = m_first; -#endif + // Empty loop (compare_exchange_weak updates e->next) } - while( !m_first.testAndSetOrdered( e->next, e ) ); } Element * popList() { - return m_first.fetchAndStoreOrdered( NULL ); + return m_first.exchange(nullptr); } Element * first() { -#if QT_VERSION >= 0x050000 - return m_first.loadAcquire(); -#else - return m_first; -#endif + return m_first.load(std::memory_order_acquire); } void setFirst( Element * e ) { -#if QT_VERSION >= 0x050000 - m_first.storeRelease( e ); -#else - m_first = e; -#endif + m_first.store(e, std::memory_order_release); } void free( Element * e ) @@ -95,7 +86,7 @@ public: private: - QAtomicPointer m_first; + std::atomic m_first; LocklessAllocatorT * m_allocator; } ; diff --git a/include/MixerWorkerThread.h b/include/MixerWorkerThread.h index 8b900195f..ff9d7c213 100644 --- a/include/MixerWorkerThread.h +++ b/include/MixerWorkerThread.h @@ -25,10 +25,10 @@ #ifndef MIXER_WORKER_THREAD_H #define MIXER_WORKER_THREAD_H -#include -#include #include +#include + class QWaitCondition; class Mixer; class ThreadableJob; @@ -46,12 +46,14 @@ public: Dynamic // jobs can be added while processing queue } ; +#define JOB_QUEUE_SIZE 1024 JobQueue() : m_items(), m_queueSize( 0 ), m_itemsDone( 0 ), m_opMode( Static ) { + std::fill(m_items, m_items + JOB_QUEUE_SIZE, nullptr); } void reset( OperationMode _opMode ); @@ -62,10 +64,9 @@ public: void wait(); private: -#define JOB_QUEUE_SIZE 1024 - QAtomicPointer m_items[JOB_QUEUE_SIZE]; - AtomicInt m_queueSize; - AtomicInt m_itemsDone; + std::atomic m_items[JOB_QUEUE_SIZE]; + std::atomic_int m_queueSize; + std::atomic_int m_itemsDone; OperationMode m_opMode; } ; diff --git a/include/NotePlayHandle.h b/include/NotePlayHandle.h index d0805b1c6..f4b6c9ec4 100644 --- a/include/NotePlayHandle.h +++ b/include/NotePlayHandle.h @@ -28,7 +28,6 @@ #include -#include "AtomicInt.h" #include "BasicFilters.h" #include "Note.h" #include "PlayHandle.h" @@ -350,7 +349,7 @@ public: private: static NotePlayHandle ** s_available; static QReadWriteLock s_mutex; - static AtomicInt s_availableIndex; + static std::atomic_int s_availableIndex; static int s_size; }; diff --git a/include/ThreadableJob.h b/include/ThreadableJob.h index ef90e86de..b2b20e9be 100644 --- a/include/ThreadableJob.h +++ b/include/ThreadableJob.h @@ -25,16 +25,15 @@ #ifndef THREADABLE_JOB_H #define THREADABLE_JOB_H -#include "AtomicInt.h" - #include "lmms_basics.h" +#include class ThreadableJob { public: - enum ProcessingState + enum class ProcessingState : int { Unstarted, Queued, @@ -43,36 +42,37 @@ public: }; ThreadableJob() : - m_state( ThreadableJob::Unstarted ) + m_state(ProcessingState::Unstarted) { } inline ProcessingState state() const { - return static_cast( (int) m_state ); + return m_state.load(); } inline void reset() { - m_state = Unstarted; + m_state = ProcessingState::Unstarted; } inline void queue() { - m_state = Queued; + m_state = ProcessingState::Queued; } inline void done() { - m_state = Done; + m_state = ProcessingState::Done; } void process() { - if( m_state.testAndSetOrdered( Queued, InProgress ) ) + auto expected = ProcessingState::Queued; + if (m_state.compare_exchange_strong(expected, ProcessingState::InProgress)) { doProcessing(); - m_state = Done; + m_state = ProcessingState::Done; } } @@ -82,8 +82,7 @@ public: protected: virtual void doProcessing() = 0; - AtomicInt m_state; - + std::atomic m_state; } ; #endif diff --git a/src/core/FxMixer.cpp b/src/core/FxMixer.cpp index 5ac23639a..ac50cab99 100644 --- a/src/core/FxMixer.cpp +++ b/src/core/FxMixer.cpp @@ -71,7 +71,7 @@ FxChannel::FxChannel( int idx, Model * _parent ) : m_lock(), m_channelIndex( idx ), m_queued( false ), - m_dependenciesMet( 0 ) + m_dependenciesMet(0) { BufferManager::clear( m_buffer, Engine::mixer()->framesPerPeriod() ); } @@ -98,7 +98,7 @@ inline void FxChannel::processed() void FxChannel::incrementDeps() { - int i = m_dependenciesMet.fetchAndAddOrdered( 1 ) + 1; + int i = m_dependenciesMet++ + 1; if( i >= m_receives.size() && ! m_queued ) { m_queued = true; @@ -594,14 +594,14 @@ void FxMixer::masterMix( sampleFrame * _buf ) MixerWorkerThread::addJob( ch ); } } - while( m_fxChannels[0]->state() != ThreadableJob::Done ) + while (m_fxChannels[0]->state() != ThreadableJob::ProcessingState::Done) { bool found = false; for( FxChannel * ch : m_fxChannels ) { - int s = ch->state(); - if( s == ThreadableJob::Queued - || s == ThreadableJob::InProgress ) + const auto s = ch->state(); + if (s == ThreadableJob::ProcessingState::Queued + || s == ThreadableJob::ProcessingState::InProgress) { found = true; break; diff --git a/src/core/LocklessAllocator.cpp b/src/core/LocklessAllocator.cpp index 3133a1f08..4839a7bd9 100644 --- a/src/core/LocklessAllocator.cpp +++ b/src/core/LocklessAllocator.cpp @@ -24,6 +24,7 @@ #include "LocklessAllocator.h" +#include #include #include "lmmsconfig.h" @@ -55,9 +56,11 @@ LocklessAllocator::LocklessAllocator( size_t nmemb, size_t size ) m_pool = new char[m_capacity * m_elementSize]; m_freeStateSets = m_capacity / SIZEOF_SET; - m_freeState = new AtomicInt[m_freeStateSets]; + m_freeState = new std::atomic_int[m_freeStateSets]; + std::fill(m_freeState, m_freeState + m_freeStateSets, 0); m_available = m_capacity; + m_startIndex = 0; } @@ -101,27 +104,27 @@ static int ffs( int i ) void * LocklessAllocator::alloc() { - int available; + // Some of these CAS loops could probably use relaxed atomics, as discussed + // in http://en.cppreference.com/w/cpp/atomic/atomic/compare_exchange. + // Let's use sequentially-consistent ops to be safe for now. + int available = m_available.load(); do { - available = m_available; if( !available ) { fprintf( stderr, "LocklessAllocator: No free space\n" ); return NULL; } } - while( !m_available.testAndSetOrdered( available, available - 1 ) ); + while (!m_available.compare_exchange_weak(available, available - 1)); - size_t startIndex = m_startIndex.fetchAndAddOrdered( 1 ) - % m_freeStateSets; - for( size_t set = startIndex;; set = ( set + 1 ) % m_freeStateSets ) + const size_t startIndex = m_startIndex++ % m_freeStateSets; + for (size_t set = startIndex;; set = ( set + 1 ) % m_freeStateSets) { - for( int freeState = m_freeState[set]; freeState != -1; - freeState = m_freeState[set] ) + for (int freeState = m_freeState[set]; freeState != -1;) { int bit = ffs( ~freeState ) - 1; - if( m_freeState[set].testAndSetOrdered( freeState, + if (m_freeState[set].compare_exchange_weak(freeState, freeState | 1 << bit ) ) { return m_pool + ( SIZEOF_SET * set + bit ) @@ -151,11 +154,11 @@ invalid: size_t set = offset / SIZEOF_SET; int bit = offset % SIZEOF_SET; int mask = 1 << bit; - int prevState = m_freeState[set].fetchAndAndOrdered( ~mask ); + int prevState = m_freeState[set].fetch_and(~mask); if ( !( prevState & mask ) ) { fprintf( stderr, "LocklessAllocator: Block not in use\n" ); return; } - m_available.fetchAndAddOrdered( 1 ); + ++m_available; } diff --git a/src/core/MixerWorkerThread.cpp b/src/core/MixerWorkerThread.cpp index ca31226b0..10e772ea7 100644 --- a/src/core/MixerWorkerThread.cpp +++ b/src/core/MixerWorkerThread.cpp @@ -52,7 +52,7 @@ void MixerWorkerThread::JobQueue::addJob( ThreadableJob * _job ) // update job state _job->queue(); // actually queue the job via atomic operations - m_items[m_queueSize.fetchAndAddOrdered(1)] = _job; + m_items[m_queueSize++] = _job; } } @@ -61,17 +61,17 @@ void MixerWorkerThread::JobQueue::addJob( ThreadableJob * _job ) void MixerWorkerThread::JobQueue::run() { bool processedJob = true; - while( processedJob && (int) m_itemsDone < (int) m_queueSize ) + while (processedJob && m_itemsDone < m_queueSize) { processedJob = false; for( int i = 0; i < m_queueSize; ++i ) { - ThreadableJob * job = m_items[i].fetchAndStoreOrdered( NULL ); + ThreadableJob * job = m_items[i].exchange(nullptr); if( job ) { job->process(); processedJob = true; - m_itemsDone.fetchAndAddOrdered( 1 ); + ++m_itemsDone; } } // always exit loop if we're not in dynamic mode @@ -84,7 +84,7 @@ void MixerWorkerThread::JobQueue::run() void MixerWorkerThread::JobQueue::wait() { - while( (int) m_itemsDone < (int) m_queueSize ) + while (m_itemsDone < m_queueSize) { #if defined(LMMS_HOST_X86) || defined(LMMS_HOST_X86_64) _mm_pause(); diff --git a/src/core/NotePlayHandle.cpp b/src/core/NotePlayHandle.cpp index f9ddc0cbc..6379ad769 100644 --- a/src/core/NotePlayHandle.cpp +++ b/src/core/NotePlayHandle.cpp @@ -551,7 +551,7 @@ void NotePlayHandle::resize( const bpm_t _new_tempo ) NotePlayHandle ** NotePlayHandleManager::s_available; QReadWriteLock NotePlayHandleManager::s_mutex; -AtomicInt NotePlayHandleManager::s_availableIndex; +std::atomic_int NotePlayHandleManager::s_availableIndex; int NotePlayHandleManager::s_size; @@ -586,7 +586,7 @@ NotePlayHandle * NotePlayHandleManager::acquire( InstrumentTrack* instrumentTrac s_mutex.unlock(); } s_mutex.lockForRead(); - NotePlayHandle * nph = s_available[ s_availableIndex.fetchAndAddOrdered( -1 ) ]; + NotePlayHandle * nph = s_available[s_availableIndex--]; s_mutex.unlock(); new( (void*)nph ) NotePlayHandle( instrumentTrack, offset, frames, noteToPlay, parent, midiEventChannel, origin ); @@ -598,7 +598,7 @@ void NotePlayHandleManager::release( NotePlayHandle * nph ) { nph->NotePlayHandle::~NotePlayHandle(); s_mutex.lockForRead(); - s_available[ s_availableIndex.fetchAndAddOrdered( 1 ) + 1 ] = nph; + s_available[++s_availableIndex] = nph; s_mutex.unlock(); } @@ -614,7 +614,7 @@ void NotePlayHandleManager::extend( int c ) for( int i=0; i < c; ++i ) { - s_available[ s_availableIndex.fetchAndAddOrdered( 1 ) + 1 ] = n; + s_available[++s_availableIndex] = n; ++n; } } diff --git a/src/core/PresetPreviewPlayHandle.cpp b/src/core/PresetPreviewPlayHandle.cpp index b85d02474..dc36819b7 100644 --- a/src/core/PresetPreviewPlayHandle.cpp +++ b/src/core/PresetPreviewPlayHandle.cpp @@ -22,7 +22,6 @@ * */ -#include #include #include "PresetPreviewPlayHandle.h" @@ -34,7 +33,7 @@ #include "ProjectJournal.h" #include "TrackContainer.h" - +#include // invisible track-container which is needed as parent for preview-channels class PreviewTrackContainer : public TrackContainer @@ -67,25 +66,17 @@ public: NotePlayHandle* previewNote() { - #if QT_VERSION >= 0x050000 - return m_previewNote.loadAcquire(); - #else - return m_previewNote; - #endif + return m_previewNote.load(std::memory_order_acquire); } void setPreviewNote( NotePlayHandle * _note ) { - #if QT_VERSION >= 0x050000 - m_previewNote.storeRelease( _note ); - #else - m_previewNote = _note; - #endif + m_previewNote.store(_note, std::memory_order_release); } bool testAndSetPreviewNote( NotePlayHandle * expectedVal, NotePlayHandle * newVal ) { - return m_previewNote.testAndSetOrdered( expectedVal, newVal ); + return m_previewNote.compare_exchange_strong(expectedVal, newVal); } void lockData() @@ -111,7 +102,7 @@ public: private: InstrumentTrack* m_previewInstrumentTrack; - QAtomicPointer m_previewNote; + std::atomic m_previewNote; QMutex m_dataMutex; friend class PresetPreviewPlayHandle; @@ -125,7 +116,7 @@ PreviewTrackContainer * PresetPreviewPlayHandle::s_previewTC; PresetPreviewPlayHandle::PresetPreviewPlayHandle( const QString & _preset_file, bool _load_by_plugin, DataFile *dataFile ) : PlayHandle( TypePresetPreviewHandle ), - m_previewNote( NULL ) + m_previewNote(nullptr) { setUsesBuffer( false );