From 4c8c9856165d18f84df9c0e1b89194677d0b318d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 4 Sep 2019 18:09:50 +0200 Subject: [PATCH 01/13] Increase number of cached chunks --- src/engine/cachingreader.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index d52404df839..b73e467c34d 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -20,10 +20,17 @@ mixxx::Logger kLogger("CachingReader"); // TODO() Do we suffer cache misses if we use an audio buffer of above 23 ms? const SINT kDefaultHintFrames = 1024; -// currently CachingReaderWorker::kCachingReaderChunkLength is 65536 (0x10000); -// For 80 chunks we need 5242880 (0x500000) bytes (5 MiB) of Memory -//static -const SINT kNumberOfCachedChunksInMemory = 80; +// With CachingReaderChunk::kFrames = 8192 each chunk consumes +// 8192 frames * 2 channels/frame * 4-bytes per sample = 65 kB. +// +// NOTE(2019-09-04): https://bugs.launchpad.net/mixxx/+bug/1842679 +// According to the linked bug report reserving only 80 chunks in +// version 2.2.2 for doesn't seem to be sufficient to prevent running +// out of free chunks. Therefore we increased the number of cached +// chunks to 256: +// 80 chunks -> 5120 KB = 5 MB +// 256 chunks -> 16384 KB = 16 MB +const SINT kNumberOfCachedChunksInMemory = 256; } // anonymous namespace From 0c663d1729c0b0c3ddb6fdf4309389c36740ae5a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 4 Sep 2019 23:19:12 +0200 Subject: [PATCH 02/13] Reduce and attach size of FIFOs to number of cached chunks --- src/engine/cachingreader.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index b73e467c34d..39629bd42b2 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -38,8 +38,8 @@ const SINT kNumberOfCachedChunksInMemory = 256; CachingReader::CachingReader(QString group, UserSettingsPointer config) : m_pConfig(config), - m_chunkReadRequestFIFO(1024), - m_readerStatusFIFO(1024), + m_chunkReadRequestFIFO(kNumberOfCachedChunksInMemory), + m_readerStatusFIFO(kNumberOfCachedChunksInMemory), m_readerStatus(INVALID), m_mruCachingReaderChunk(nullptr), m_lruCachingReaderChunk(nullptr), From 09e96b8c9a043f9d40a2b5a4c83ff314a18e1a93 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 09:00:13 +0200 Subject: [PATCH 03/13] Add debug assertions to detect memory-leaks in cache --- src/engine/cachingreaderchunk.cpp | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/engine/cachingreaderchunk.cpp b/src/engine/cachingreaderchunk.cpp index 9eb2dbbf085..2c7af9f8416 100644 --- a/src/engine/cachingreaderchunk.cpp +++ b/src/engine/cachingreaderchunk.cpp @@ -126,22 +126,32 @@ CachingReaderChunkForOwner::~CachingReaderChunkForOwner() { } void CachingReaderChunkForOwner::init(SINT index) { - DEBUG_ASSERT(READ_PENDING != m_state); + // Must not be referenced in MRU/LRU list! + DEBUG_ASSERT(!m_pNext); + DEBUG_ASSERT(!m_pPrev); + // Must not be accessed by a worker! + DEBUG_ASSERT(m_state != READ_PENDING); CachingReaderChunk::init(index); m_state = READY; } void CachingReaderChunkForOwner::free() { - DEBUG_ASSERT(READ_PENDING != m_state); + // Must not be referenced in MRU/LRU list! + DEBUG_ASSERT(!m_pNext); + DEBUG_ASSERT(!m_pPrev); + // Must not be accessed by a worker! + DEBUG_ASSERT(m_state != READ_PENDING); CachingReaderChunk::init(kInvalidChunkIndex); m_state = FREE; } void CachingReaderChunkForOwner::insertIntoListBefore( CachingReaderChunkForOwner* pBefore) { - DEBUG_ASSERT(m_pNext == nullptr); - DEBUG_ASSERT(m_pPrev == nullptr); - DEBUG_ASSERT(m_state != READ_PENDING); // Must not be accessed by a worker! + // Must not be referenced in MRU/LRU list! + DEBUG_ASSERT(!m_pNext); + DEBUG_ASSERT(!m_pPrev); + // Must not be accessed by a worker! + DEBUG_ASSERT(m_state != READ_PENDING); m_pNext = pBefore; if (pBefore) { @@ -157,6 +167,7 @@ void CachingReaderChunkForOwner::insertIntoListBefore( void CachingReaderChunkForOwner::removeFromList( CachingReaderChunkForOwner** ppHead, CachingReaderChunkForOwner** ppTail) { + DEBUG_ASSERT(m_pNext || m_pPrev); // Remove this chunk from the double-linked list... CachingReaderChunkForOwner* pNext = m_pNext; CachingReaderChunkForOwner* pPrev = m_pPrev; From a66af2ecd6c94a531ba49143b7f0afa0d46c6926 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 13:11:41 +0200 Subject: [PATCH 04/13] Simplify double-linked list removal --- src/engine/cachingreaderchunk.cpp | 36 +++++++++++++++++++------------ 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/src/engine/cachingreaderchunk.cpp b/src/engine/cachingreaderchunk.cpp index 2c7af9f8416..2b7e41c3702 100644 --- a/src/engine/cachingreaderchunk.cpp +++ b/src/engine/cachingreaderchunk.cpp @@ -167,28 +167,36 @@ void CachingReaderChunkForOwner::insertIntoListBefore( void CachingReaderChunkForOwner::removeFromList( CachingReaderChunkForOwner** ppHead, CachingReaderChunkForOwner** ppTail) { - DEBUG_ASSERT(m_pNext || m_pPrev); + DEBUG_ASSERT(ppHead); + DEBUG_ASSERT(ppTail); + if (!m_pPrev && !m_pNext) { + // Not in linked list -> nothing to do + return; + } + // Remove this chunk from the double-linked list... - CachingReaderChunkForOwner* pNext = m_pNext; - CachingReaderChunkForOwner* pPrev = m_pPrev; - m_pNext = nullptr; + const auto pPrev = m_pPrev; + const auto pNext = m_pNext; m_pPrev = nullptr; + m_pNext = nullptr; - // ...reconnect the remaining list elements... - if (pNext) { - DEBUG_ASSERT(this == pNext->m_pPrev); - pNext->m_pPrev = pPrev; - } + // ...reconnect the adjacent list items and adjust head/tail if (pPrev) { DEBUG_ASSERT(this == pPrev->m_pNext); pPrev->m_pNext = pNext; - } - - // ...and adjust head/tail. - if (ppHead && (this == *ppHead)) { + } else { + // No predecessor + DEBUG_ASSERT(this == *ppHead); + // pNext becomes the new head *ppHead = pNext; } - if (ppTail && (this == *ppTail)) { + if (pNext) { + DEBUG_ASSERT(this == pNext->m_pPrev); + pNext->m_pPrev = pPrev; + } else { + // No successor + DEBUG_ASSERT(this == *ppTail); + // pPrev becomes the new tail *ppTail = pPrev; } } From b05a7181ec48be209fa28d3650a58b345f53e07c Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 13:21:08 +0200 Subject: [PATCH 05/13] Ensure complete initialization of caching worker update messages --- src/engine/cachingreader.cpp | 24 ++++++++++-------------- src/engine/cachingreaderworker.cpp | 21 +++++++++------------ src/engine/cachingreaderworker.h | 16 +++++++++++++++- 3 files changed, 34 insertions(+), 27 deletions(-) diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index 39629bd42b2..8ed0b4509cf 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -189,15 +189,11 @@ void CachingReader::newTrack(TrackPointer pTrack) { } void CachingReader::process() { - ReaderStatusUpdate status; - while (m_readerStatusFIFO.read(&status, 1) == 1) { - CachingReaderChunkForOwner* pChunk = static_cast(status.chunk); + ReaderStatusUpdate update; + while (m_readerStatusFIFO.read(&update, 1) == 1) { + auto pChunk = update.takeFromWorker(); if (pChunk) { - // Take over control of the chunk from the worker. - // This has to be done before freeing all chunks - // after a new track has been loaded (see below)! - pChunk->takeFromWorker(); - if (status.status == CHUNK_READ_SUCCESS) { + if (update.status == CHUNK_READ_SUCCESS) { // Insert or freshen the chunk in the MRU/LRU list after // obtaining ownership from the worker. freshenChunk(pChunk); @@ -206,12 +202,12 @@ void CachingReader::process() { freeChunk(pChunk); } } - if (status.status == TRACK_NOT_LOADED) { - m_readerStatus = status.status; - } else if (status.status == TRACK_LOADED) { - m_readerStatus = status.status; + if (update.status == TRACK_NOT_LOADED) { + m_readerStatus = update.status; + } else if (update.status == TRACK_LOADED) { + m_readerStatus = update.status; // Reset the max. readable frame index - m_readableFrameIndexRange = status.readableFrameIndexRange(); + m_readableFrameIndexRange = update.readableFrameIndexRange(); // Free all chunks with sample data from a previous track freeAllChunks(); } @@ -219,7 +215,7 @@ void CachingReader::process() { // Adjust the readable frame index range after loading or reading m_readableFrameIndexRange = intersect( m_readableFrameIndexRange, - status.readableFrameIndexRange()); + update.readableFrameIndexRange()); } else { // Reset the readable frame index range m_readableFrameIndexRange = mixxx::IndexRange(); diff --git a/src/engine/cachingreaderworker.cpp b/src/engine/cachingreaderworker.cpp index 34f9a9ac836..84b52e937e4 100644 --- a/src/engine/cachingreaderworker.cpp +++ b/src/engine/cachingreaderworker.cpp @@ -133,14 +133,14 @@ mixxx::AudioSourcePointer openAudioSourceForReading(const TrackPointer& pTrack, } // anonymous namespace void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) { - ReaderStatusUpdate status; - status.init(TRACK_NOT_LOADED); + ReaderStatusUpdate update; + update.init(TRACK_NOT_LOADED); if (!pTrack) { // Unload track m_pAudioSource.reset(); // Close open file handles m_readableFrameIndexRange = mixxx::IndexRange(); - m_pReaderStatusFIFO->writeBlocking(&status, 1); + m_pReaderStatusFIFO->writeBlocking(&update, 1); return; } @@ -152,7 +152,7 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) { // Must unlock before emitting to avoid deadlock kLogger.debug() << m_group << "loadTrack() load failed for\"" << filename << "\", unlocked reader lock"; - m_pReaderStatusFIFO->writeBlocking(&status, 1); + m_pReaderStatusFIFO->writeBlocking(&update, 1); emit(trackLoadFailed( pTrack, QString("The file '%1' could not be found.") .arg(QDir::toNativeSeparators(filename)))); @@ -167,7 +167,7 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) { // Must unlock before emitting to avoid deadlock kLogger.debug() << m_group << "loadTrack() load failed for\"" << filename << "\", file invalid, unlocked reader lock"; - m_pReaderStatusFIFO->writeBlocking(&status, 1); + m_pReaderStatusFIFO->writeBlocking(&update, 1); emit(trackLoadFailed( pTrack, QString("The file '%1' could not be loaded.").arg(filename))); return; @@ -183,18 +183,15 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) { // be decreased to avoid repeated reading of corrupt audio data. m_readableFrameIndexRange = m_pAudioSource->frameIndexRange(); - status.status = TRACK_LOADED; - status.readableFrameIndexRangeStart = m_readableFrameIndexRange.start(); - status.readableFrameIndexRangeEnd = m_readableFrameIndexRange.end(); - m_pReaderStatusFIFO->writeBlocking(&status, 1); + update.init(TRACK_LOADED, nullptr, m_pAudioSource->frameIndexRange()); + m_pReaderStatusFIFO->writeBlocking(&update, 1); // Clear the chunks to read list. CachingReaderChunkReadRequest request; while (m_pChunkReadRequestFIFO->read(&request, 1) == 1) { kLogger.debug() << "Cancelling read request for " << request.chunk->getIndex(); - status.status = CHUNK_READ_INVALID; - status.chunk = request.chunk; - m_pReaderStatusFIFO->writeBlocking(&status, 1); + update.init(CHUNK_READ_INVALID, request.chunk); + m_pReaderStatusFIFO->writeBlocking(&update, 1); } // Emit that the track is loaded. diff --git a/src/engine/cachingreaderworker.h b/src/engine/cachingreaderworker.h index 54c5a4c77e4..eab3c4a8c98 100644 --- a/src/engine/cachingreaderworker.h +++ b/src/engine/cachingreaderworker.h @@ -36,11 +36,14 @@ enum ReaderStatus { // POD with trivial ctor/dtor/copy for passing through FIFO typedef struct ReaderStatusUpdate { - ReaderStatus status; + private: CachingReaderChunk* chunk; SINT readableFrameIndexRangeStart; SINT readableFrameIndexRangeEnd; + public: + ReaderStatus status; + void init( ReaderStatus statusArg = INVALID, CachingReaderChunk* chunkArg = nullptr, @@ -51,6 +54,17 @@ typedef struct ReaderStatusUpdate { readableFrameIndexRangeEnd = readableFrameIndexRangeArg.end(); } + CachingReaderChunkForOwner* takeFromWorker() { + CachingReaderChunkForOwner* pChunk = nullptr; + if (chunk) { + DEBUG_ASSERT(dynamic_cast(chunk)); + pChunk = static_cast(chunk); + chunk = nullptr; + pChunk->takeFromWorker(); + } + return pChunk; + } + mixxx::IndexRange readableFrameIndexRange() const { return mixxx::IndexRange::between( readableFrameIndexRangeStart, From 5c89e8b6cbc7b1cfea4812f7e147b768358fcd25 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 14:40:49 +0200 Subject: [PATCH 06/13] Use default destructors and std::move --- src/engine/cachingreaderchunk.cpp | 12 +++--------- src/engine/cachingreaderchunk.h | 4 ++-- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/engine/cachingreaderchunk.cpp b/src/engine/cachingreaderchunk.cpp index 2b7e41c3702..09065bd3d2a 100644 --- a/src/engine/cachingreaderchunk.cpp +++ b/src/engine/cachingreaderchunk.cpp @@ -33,11 +33,8 @@ const SINT CachingReaderChunk::kSamples = CachingReaderChunk::CachingReaderChunk( mixxx::SampleBuffer::WritableSlice sampleBuffer) : m_index(kInvalidChunkIndex), - m_sampleBuffer(sampleBuffer) { - DEBUG_ASSERT(sampleBuffer.length() == kSamples); -} - -CachingReaderChunk::~CachingReaderChunk() { + m_sampleBuffer(std::move(sampleBuffer)) { + DEBUG_ASSERT(m_sampleBuffer.length() == kSamples); } void CachingReaderChunk::init(SINT index) { @@ -116,15 +113,12 @@ mixxx::IndexRange CachingReaderChunk::readBufferedSampleFramesReverse( CachingReaderChunkForOwner::CachingReaderChunkForOwner( mixxx::SampleBuffer::WritableSlice sampleBuffer) - : CachingReaderChunk(sampleBuffer), + : CachingReaderChunk(std::move(sampleBuffer)), m_state(FREE), m_pPrev(nullptr), m_pNext(nullptr) { } -CachingReaderChunkForOwner::~CachingReaderChunkForOwner() { -} - void CachingReaderChunkForOwner::init(SINT index) { // Must not be referenced in MRU/LRU list! DEBUG_ASSERT(!m_pNext); diff --git a/src/engine/cachingreaderchunk.h b/src/engine/cachingreaderchunk.h index 463e7366665..c7e282e6944 100644 --- a/src/engine/cachingreaderchunk.h +++ b/src/engine/cachingreaderchunk.h @@ -67,7 +67,7 @@ class CachingReaderChunk { protected: explicit CachingReaderChunk( mixxx::SampleBuffer::WritableSlice sampleBuffer); - virtual ~CachingReaderChunk(); + virtual ~CachingReaderChunk() = default; void init(SINT index); @@ -91,7 +91,7 @@ class CachingReaderChunkForOwner: public CachingReaderChunk { public: explicit CachingReaderChunkForOwner( mixxx::SampleBuffer::WritableSlice sampleBuffer); - ~CachingReaderChunkForOwner() override; + ~CachingReaderChunkForOwner() override = default; void init(SINT index); void free(); From 510a871c27a501069bbffe3074de1269a5950cec Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 14:51:19 +0200 Subject: [PATCH 07/13] Fix management of MRU/LRU chunk list --- src/engine/cachingreader.cpp | 109 ++++++++++++++++-------------- src/engine/cachingreader.h | 1 + src/engine/cachingreaderchunk.cpp | 101 ++++++++++++++++++++------- src/engine/cachingreaderchunk.h | 24 ++++--- 4 files changed, 150 insertions(+), 85 deletions(-) diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index 8ed0b4509cf..2afffecc779 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -80,8 +80,16 @@ CachingReader::~CachingReader() { qDeleteAll(m_chunks); } +void CachingReader::freeChunkFromList(CachingReaderChunkForOwner* pChunk) { + pChunk->removeFromList( + &m_mruCachingReaderChunk, + &m_lruCachingReaderChunk); + pChunk->free(); + m_freeChunks.push_back(pChunk); +} + void CachingReader::freeChunk(CachingReaderChunkForOwner* pChunk) { - DEBUG_ASSERT(pChunk != nullptr); + DEBUG_ASSERT(pChunk); DEBUG_ASSERT(pChunk->getState() != CachingReaderChunkForOwner::READ_PENDING); const int removed = m_allocatedCachingReaderChunks.remove(pChunk->getIndex()); @@ -89,10 +97,7 @@ void CachingReader::freeChunk(CachingReaderChunkForOwner* pChunk) { // because sometime you free a chunk right after you allocated it. DEBUG_ASSERT(removed <= 1); - pChunk->removeFromList( - &m_mruCachingReaderChunk, &m_lruCachingReaderChunk); - pChunk->free(); - m_freeChunks.push_back(pChunk); + freeChunkFromList(pChunk); } void CachingReader::freeAllChunks() { @@ -104,15 +109,13 @@ void CachingReader::freeAllChunks() { } if (pChunk->getState() != CachingReaderChunkForOwner::FREE) { - pChunk->removeFromList( - &m_mruCachingReaderChunk, &m_lruCachingReaderChunk); - pChunk->free(); - m_freeChunks.push_back(pChunk); + freeChunkFromList(pChunk); } } + DEBUG_ASSERT(!m_mruCachingReaderChunk); + DEBUG_ASSERT(!m_lruCachingReaderChunk); m_allocatedCachingReaderChunks.clear(); - m_mruCachingReaderChunk = nullptr; } CachingReaderChunkForOwner* CachingReader::allocateChunk(SINT chunkIndex) { @@ -129,55 +132,53 @@ CachingReaderChunkForOwner* CachingReader::allocateChunk(SINT chunkIndex) { } CachingReaderChunkForOwner* CachingReader::allocateChunkExpireLRU(SINT chunkIndex) { - CachingReaderChunkForOwner* pChunk = allocateChunk(chunkIndex); + auto pChunk = allocateChunk(chunkIndex); if (!pChunk) { - if (m_lruCachingReaderChunk == nullptr) { - kLogger.warning() << "ERROR: No LRU chunk to free in allocateChunkExpireLRU."; - return nullptr; + if (m_lruCachingReaderChunk) { + freeChunk(m_lruCachingReaderChunk); + pChunk = allocateChunk(chunkIndex); + } else { + kLogger.warning() << "No cached LRU chunk available for freeing"; } - freeChunk(m_lruCachingReaderChunk); - pChunk = allocateChunk(chunkIndex); } - //kLogger.debug() << "allocateChunkExpireLRU" << chunk << pChunk; + if (kLogger.traceEnabled()) { + kLogger.trace() << "allocateChunkExpireLRU" << chunkIndex << pChunk; + } return pChunk; } CachingReaderChunkForOwner* CachingReader::lookupChunk(SINT chunkIndex) { // Defaults to nullptr if it's not in the hash. - CachingReaderChunkForOwner* chunk = m_allocatedCachingReaderChunks.value(chunkIndex, nullptr); - - // Make sure the allocated number matches the indexed chunk number. - DEBUG_ASSERT(chunk == nullptr || chunkIndex == chunk->getIndex()); - - return chunk; + auto pChunk = m_allocatedCachingReaderChunks.value(chunkIndex, nullptr); + DEBUG_ASSERT(!pChunk || pChunk->getIndex() == chunkIndex); + return pChunk; } void CachingReader::freshenChunk(CachingReaderChunkForOwner* pChunk) { - DEBUG_ASSERT(pChunk != nullptr); - DEBUG_ASSERT(pChunk->getState() != CachingReaderChunkForOwner::READ_PENDING); - - // Remove the chunk from the LRU list - pChunk->removeFromList(&m_mruCachingReaderChunk, &m_lruCachingReaderChunk); - - // Adjust the least-recently-used item before inserting the - // chunk as the new most-recently-used item. - if (m_lruCachingReaderChunk == nullptr) { - if (m_mruCachingReaderChunk == nullptr) { - m_lruCachingReaderChunk = pChunk; - } else { - m_lruCachingReaderChunk = m_mruCachingReaderChunk; - } + DEBUG_ASSERT(pChunk); + DEBUG_ASSERT(pChunk->getState() == CachingReaderChunkForOwner::READY); + if (kLogger.traceEnabled()) { + kLogger.trace() + << "freshenChunk()" + << pChunk->getIndex() + << pChunk; } - // Insert the chunk as the new most-recently-used item. - pChunk->insertIntoListBefore(m_mruCachingReaderChunk); - m_mruCachingReaderChunk = pChunk; + // Remove the chunk from the MRU/LRU list + pChunk->removeFromList( + &m_mruCachingReaderChunk, + &m_lruCachingReaderChunk); + + // Reinsert has new head of MRU list + pChunk->insertIntoListBefore( + &m_mruCachingReaderChunk, + &m_lruCachingReaderChunk, + m_mruCachingReaderChunk); } CachingReaderChunkForOwner* CachingReader::lookupChunkAndFreshen(SINT chunkIndex) { - CachingReaderChunkForOwner* pChunk = lookupChunk(chunkIndex); - if (pChunk && - (pChunk->getState() != CachingReaderChunkForOwner::READ_PENDING)) { + auto pChunk = lookupChunk(chunkIndex); + if (pChunk && (pChunk->getState() == CachingReaderChunkForOwner::READY)) { freshenChunk(pChunk); } return pChunk; @@ -476,27 +477,33 @@ void CachingReader::hintAndMaybeWake(const HintVector& hintList) { const int lastChunkIndex = CachingReaderChunk::indexForFrame(readableFrameIndexRange.end() - 1); for (int chunkIndex = firstChunkIndex; chunkIndex <= lastChunkIndex; ++chunkIndex) { CachingReaderChunkForOwner* pChunk = lookupChunk(chunkIndex); - if (pChunk == nullptr) { + if (!pChunk) { shouldWake = true; pChunk = allocateChunkExpireLRU(chunkIndex); - if (pChunk == nullptr) { - kLogger.warning() << "ERROR: Couldn't allocate spare CachingReaderChunk to make CachingReaderChunkReadRequest."; + if (!pChunk) { + kLogger.warning() + << "Failed to allocate chunk" + << chunkIndex + << "for read request"; continue; } // Do not insert the allocated chunk into the MRU/LRU list, // because it will be handed over to the worker immediately CachingReaderChunkReadRequest request; request.giveToWorker(pChunk); - // kLogger.debug() << "Requesting read of chunk" << current << "into" << pChunk; - // kLogger.debug() << "Requesting read into " << request.chunk->data; + if (kLogger.traceEnabled()) { + kLogger.trace() + << "Requesting read of chunk" + << request.chunk; + } if (m_chunkReadRequestFIFO.write(&request, 1) != 1) { - kLogger.warning() << "ERROR: Could not submit read request for " - << chunkIndex; + kLogger.warning() + << "Failed to submit read request for chunk" + << chunkIndex; // Revoke the chunk from the worker and free it pChunk->takeFromWorker(); freeChunk(pChunk); } - //kLogger.debug() << "Checking chunk " << current << " shouldWake:" << shouldWake << " chunksToRead" << m_chunksToRead.size(); } else if (pChunk->getState() == CachingReaderChunkForOwner::READY) { // This will cause the chunk to be 'freshened' in the cache. The // chunk will be moved to the end of the LRU list. diff --git a/src/engine/cachingreader.h b/src/engine/cachingreader.h index 029b44246da..e148cd637e1 100644 --- a/src/engine/cachingreader.h +++ b/src/engine/cachingreader.h @@ -140,6 +140,7 @@ class CachingReader : public QObject { // Returns a CachingReaderChunk to the free list void freeChunk(CachingReaderChunkForOwner* pChunk); + void freeChunkFromList(CachingReaderChunkForOwner* pChunk); // Returns all allocated chunks to the free list void freeAllChunks(); diff --git a/src/engine/cachingreaderchunk.cpp b/src/engine/cachingreaderchunk.cpp index 09065bd3d2a..701fb00bf50 100644 --- a/src/engine/cachingreaderchunk.cpp +++ b/src/engine/cachingreaderchunk.cpp @@ -38,6 +38,7 @@ CachingReaderChunk::CachingReaderChunk( } void CachingReaderChunk::init(SINT index) { + DEBUG_ASSERT(m_index == kInvalidChunkIndex || index == kInvalidChunkIndex); m_index = index; m_bufferedSampleFrames.frameIndexRange() = mixxx::IndexRange(); } @@ -45,6 +46,7 @@ void CachingReaderChunk::init(SINT index) { // Frame index range of this chunk for the given audio source. mixxx::IndexRange CachingReaderChunk::frameIndexRange( const mixxx::AudioSourcePointer& pAudioSource) const { + DEBUG_ASSERT(m_index != kInvalidChunkIndex); if (!pAudioSource) { return mixxx::IndexRange(); } @@ -59,6 +61,7 @@ mixxx::IndexRange CachingReaderChunk::frameIndexRange( mixxx::IndexRange CachingReaderChunk::bufferSampleFrames( const mixxx::AudioSourcePointer& pAudioSource, mixxx::SampleBuffer::WritableSlice tempOutputBuffer) { + DEBUG_ASSERT(m_index != kInvalidChunkIndex); const auto sourceFrameIndexRange = frameIndexRange(pAudioSource); mixxx::AudioSourceStereoProxy audioSourceProxy( pAudioSource, @@ -76,6 +79,7 @@ mixxx::IndexRange CachingReaderChunk::bufferSampleFrames( mixxx::IndexRange CachingReaderChunk::readBufferedSampleFrames( CSAMPLE* sampleBuffer, const mixxx::IndexRange& frameIndexRange) const { + DEBUG_ASSERT(m_index != kInvalidChunkIndex); const auto copyableFrameIndexRange = intersect(frameIndexRange, m_bufferedSampleFrames.frameIndexRange()); if (!copyableFrameIndexRange.empty()) { @@ -95,6 +99,7 @@ mixxx::IndexRange CachingReaderChunk::readBufferedSampleFrames( mixxx::IndexRange CachingReaderChunk::readBufferedSampleFramesReverse( CSAMPLE* reverseSampleBuffer, const mixxx::IndexRange& frameIndexRange) const { + DEBUG_ASSERT(m_index != kInvalidChunkIndex); const auto copyableFrameIndexRange = intersect(frameIndexRange, m_bufferedSampleFrames.frameIndexRange()); if (!copyableFrameIndexRange.empty()) { @@ -120,77 +125,123 @@ CachingReaderChunkForOwner::CachingReaderChunkForOwner( } void CachingReaderChunkForOwner::init(SINT index) { + // Must not be accessed by a worker! + DEBUG_ASSERT(m_state != READ_PENDING); // Must not be referenced in MRU/LRU list! DEBUG_ASSERT(!m_pNext); DEBUG_ASSERT(!m_pPrev); - // Must not be accessed by a worker! - DEBUG_ASSERT(m_state != READ_PENDING); + CachingReaderChunk::init(index); m_state = READY; } void CachingReaderChunkForOwner::free() { + // Must not be accessed by a worker! + DEBUG_ASSERT(m_state != READ_PENDING); // Must not be referenced in MRU/LRU list! DEBUG_ASSERT(!m_pNext); DEBUG_ASSERT(!m_pPrev); - // Must not be accessed by a worker! - DEBUG_ASSERT(m_state != READ_PENDING); + CachingReaderChunk::init(kInvalidChunkIndex); m_state = FREE; } void CachingReaderChunkForOwner::insertIntoListBefore( + CachingReaderChunkForOwner** ppHead, + CachingReaderChunkForOwner** ppTail, CachingReaderChunkForOwner* pBefore) { - // Must not be referenced in MRU/LRU list! + DEBUG_ASSERT(m_state == READY); + // Both head and tail need to be adjusted + DEBUG_ASSERT(ppHead); + DEBUG_ASSERT(ppTail); + // Cannot insert before itself + DEBUG_ASSERT(this != pBefore); + // Must not yet be referenced in MRU/LRU list + DEBUG_ASSERT(this != *ppHead); + DEBUG_ASSERT(this != *ppTail); DEBUG_ASSERT(!m_pNext); DEBUG_ASSERT(!m_pPrev); - // Must not be accessed by a worker! - DEBUG_ASSERT(m_state != READ_PENDING); + if (kLogger.traceEnabled()) { + kLogger.trace() + << "insertIntoListBefore()" + << this + << ppHead << *ppHead + << ppTail << *ppTail + << pBefore; + } - m_pNext = pBefore; if (pBefore) { - if (pBefore->m_pPrev) { - m_pPrev = pBefore->m_pPrev; - DEBUG_ASSERT(m_pPrev->m_pNext == pBefore); + // List must already contain one or more item, i.e. has both + // a head and a tail + DEBUG_ASSERT(*ppHead); + DEBUG_ASSERT(*ppTail); + m_pPrev = pBefore->m_pPrev; + pBefore->m_pPrev = this; + m_pNext = pBefore; + if (*ppHead == pBefore) { + // Replace head + *ppHead = this; + } + } else { + // Append as new tail + m_pPrev = *ppTail; + *ppTail = this; + if (m_pPrev) { m_pPrev->m_pNext = this; } - pBefore->m_pPrev = this; + if (!*ppHead) { + // Initialize new head if the list was empty before + *ppHead = this; + } } } void CachingReaderChunkForOwner::removeFromList( CachingReaderChunkForOwner** ppHead, CachingReaderChunkForOwner** ppTail) { + DEBUG_ASSERT(m_state == READY); + // Both head and tail need to be adjusted DEBUG_ASSERT(ppHead); DEBUG_ASSERT(ppTail); - if (!m_pPrev && !m_pNext) { - // Not in linked list -> nothing to do - return; + if (kLogger.traceEnabled()) { + kLogger.trace() + << "removeFromList()" + << this + << ppHead << *ppHead + << ppTail << *ppTail; } - // Remove this chunk from the double-linked list... + // Disconnect this chunk from the double-linked list const auto pPrev = m_pPrev; const auto pNext = m_pNext; m_pPrev = nullptr; m_pNext = nullptr; - // ...reconnect the adjacent list items and adjust head/tail + // Reconnect the adjacent list items and adjust head/tail if needed if (pPrev) { DEBUG_ASSERT(this == pPrev->m_pNext); pPrev->m_pNext = pNext; } else { - // No predecessor - DEBUG_ASSERT(this == *ppHead); - // pNext becomes the new head - *ppHead = pNext; + // Only the current head item doesn't have a predecessor + if (this == *ppHead) { + // pNext becomes the new head + *ppHead = pNext; + } else { + // Item was not part the list and must not have any successor + DEBUG_ASSERT(!pPrev); + } } if (pNext) { DEBUG_ASSERT(this == pNext->m_pPrev); pNext->m_pPrev = pPrev; } else { - // No successor - DEBUG_ASSERT(this == *ppTail); - // pPrev becomes the new tail - *ppTail = pPrev; + // Only the current tail item doesn't have a successor + if (this == *ppTail) { + // pPrev becomes the new tail + *ppTail = pPrev; + } else { + // Item was not part the list and must not have any predecessor + DEBUG_ASSERT(!pPrev); + } } } diff --git a/src/engine/cachingreaderchunk.h b/src/engine/cachingreaderchunk.h index c7e282e6944..9f44ec3d7b9 100644 --- a/src/engine/cachingreaderchunk.h +++ b/src/engine/cachingreaderchunk.h @@ -108,24 +108,30 @@ class CachingReaderChunkForOwner: public CachingReaderChunk { // The state is controlled by the cache as the owner of each chunk! void giveToWorker() { - DEBUG_ASSERT(READY == m_state); + // Must not be referenced in MRU/LRU list! + DEBUG_ASSERT(!m_pPrev); + DEBUG_ASSERT(!m_pNext); + DEBUG_ASSERT(m_state == READY); m_state = READ_PENDING; } void takeFromWorker() { - DEBUG_ASSERT(READ_PENDING == m_state); + // Must not be referenced in MRU/LRU list! + DEBUG_ASSERT(!m_pPrev); + DEBUG_ASSERT(!m_pNext); + DEBUG_ASSERT(m_state == READ_PENDING); m_state = READY; } // Inserts a chunk into the double-linked list before the - // given chunk. If the list is currently empty simply pass - // pBefore = nullptr. Please note that if pBefore points to - // the head of the current list this chunk becomes the new - // head of the list. + // given chunk and adjusts the head/tail pointers. The + // chunk is inserted at the tail of the list if + // pBefore == nullptr. void insertIntoListBefore( + CachingReaderChunkForOwner** ppHead, + CachingReaderChunkForOwner** ppTail, CachingReaderChunkForOwner* pBefore); - // Removes a chunk from the double-linked list and optionally - // adjusts head/tail pointers. Pass ppHead/ppTail = nullptr if - // you prefer to adjust those pointers manually. + // Removes a chunk from the double-linked list and adjusts + // the head/tail pointers. void removeFromList( CachingReaderChunkForOwner** ppHead, CachingReaderChunkForOwner** ppTail); From cc2c242cf2fd364a003f902443f0bf320ef33317 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 16:31:58 +0200 Subject: [PATCH 08/13] Add notes about how to test the caching of audio data --- src/engine/cachingreader.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index 2afffecc779..16caf31f1d9 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -28,8 +28,14 @@ const SINT kDefaultHintFrames = 1024; // version 2.2.2 for doesn't seem to be sufficient to prevent running // out of free chunks. Therefore we increased the number of cached // chunks to 256: +// // 80 chunks -> 5120 KB = 5 MB // 256 chunks -> 16384 KB = 16 MB +// +// NOTE(uklotzde, 2019-09-05): Reduce this number to just few chunks +// (kNumberOfCachedChunksInMemory = 1, 2, 3, ...) for testing purposes +// to verify that the MRU/LRU cache works as expected. Even though +// massive drop outs are expected to occur Mixxx should run reliably! const SINT kNumberOfCachedChunksInMemory = 256; } // anonymous namespace From dc92cad298505e2655980da02983ddb40b4ade48 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 21:15:37 +0200 Subject: [PATCH 09/13] Clean up code and logs in CachingReaderWorker --- src/engine/cachingreaderworker.cpp | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/src/engine/cachingreaderworker.cpp b/src/engine/cachingreaderworker.cpp index 84b52e937e4..955267563cf 100644 --- a/src/engine/cachingreaderworker.cpp +++ b/src/engine/cachingreaderworker.cpp @@ -55,6 +55,7 @@ ReaderStatusUpdate CachingReaderWorker::processReadRequest( ReaderStatus status = bufferedFrameIndexRange.empty() ? CHUNK_READ_EOF : CHUNK_READ_SUCCESS; if (chunkFrameIndexRange != bufferedFrameIndexRange) { kLogger.warning() + << m_group << "Failed to read chunk samples for frame index range:" << "actual =" << bufferedFrameIndexRange << ", expected =" << chunkFrameIndexRange; @@ -120,18 +121,6 @@ void CachingReaderWorker::run() { } } -namespace { - -mixxx::AudioSourcePointer openAudioSourceForReading(const TrackPointer& pTrack, const mixxx::AudioSource::OpenParams& params) { - auto pAudioSource = SoundSourceProxy(pTrack).openAudioSource(params); - if (!pAudioSource) { - kLogger.warning() << "Failed to open file:" << pTrack->getLocation(); - } - return pAudioSource; -} - -} // anonymous namespace - void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) { ReaderStatusUpdate update; update.init(TRACK_NOT_LOADED); @@ -149,9 +138,10 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) { QString filename = pTrack->getLocation(); if (filename.isEmpty() || !pTrack->exists()) { - // Must unlock before emitting to avoid deadlock - kLogger.debug() << m_group << "loadTrack() load failed for\"" - << filename << "\", unlocked reader lock"; + kLogger.warning() + << m_group + << "File not found" + << filename; m_pReaderStatusFIFO->writeBlocking(&update, 1); emit(trackLoadFailed( pTrack, QString("The file '%1' could not be found.") @@ -161,12 +151,13 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) { mixxx::AudioSource::OpenParams config; config.setChannelCount(CachingReaderChunk::kChannels); - m_pAudioSource = openAudioSourceForReading(pTrack, config); + m_pAudioSource = SoundSourceProxy(pTrack).openAudioSource(config); if (!m_pAudioSource) { m_readableFrameIndexRange = mixxx::IndexRange(); - // Must unlock before emitting to avoid deadlock - kLogger.debug() << m_group << "loadTrack() load failed for\"" - << filename << "\", file invalid, unlocked reader lock"; + kLogger.warning() + << m_group + << "Failed to open file" + << filename; m_pReaderStatusFIFO->writeBlocking(&update, 1); emit(trackLoadFailed( pTrack, QString("The file '%1' could not be loaded.").arg(filename))); @@ -189,7 +180,6 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) { // Clear the chunks to read list. CachingReaderChunkReadRequest request; while (m_pChunkReadRequestFIFO->read(&request, 1) == 1) { - kLogger.debug() << "Cancelling read request for " << request.chunk->getIndex(); update.init(CHUNK_READ_INVALID, request.chunk); m_pReaderStatusFIFO->writeBlocking(&update, 1); } From 8ba0de6a1eace46980b3b0c5bdd5b9f1643b26c0 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 21:21:43 +0200 Subject: [PATCH 10/13] Reduce number of allocated chunks and leave a note --- src/engine/cachingreader.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index 16caf31f1d9..a294512e498 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -23,20 +23,19 @@ const SINT kDefaultHintFrames = 1024; // With CachingReaderChunk::kFrames = 8192 each chunk consumes // 8192 frames * 2 channels/frame * 4-bytes per sample = 65 kB. // -// NOTE(2019-09-04): https://bugs.launchpad.net/mixxx/+bug/1842679 -// According to the linked bug report reserving only 80 chunks in -// version 2.2.2 for doesn't seem to be sufficient to prevent running -// out of free chunks. Therefore we increased the number of cached -// chunks to 256: -// // 80 chunks -> 5120 KB = 5 MB -// 256 chunks -> 16384 KB = 16 MB +// +// Each deck (including sample decks) will use their own CachingReader. +// Consequently the total memory required for all allocated chunks depends +// on the number of decks. The amount of memory reserved for a single +// CachingReader must be multiplied by the number of decks to calculate +// the total amount! // // NOTE(uklotzde, 2019-09-05): Reduce this number to just few chunks // (kNumberOfCachedChunksInMemory = 1, 2, 3, ...) for testing purposes // to verify that the MRU/LRU cache works as expected. Even though // massive drop outs are expected to occur Mixxx should run reliably! -const SINT kNumberOfCachedChunksInMemory = 256; +const SINT kNumberOfCachedChunksInMemory = 80; } // anonymous namespace From 56625eb9e223bf5e0da8fc4d6a523f01c0e6ca88 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 21:31:06 +0200 Subject: [PATCH 11/13] Adjust logs in CachingReader --- src/engine/cachingreader.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index a294512e498..098b813289e 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -130,7 +130,6 @@ CachingReaderChunkForOwner* CachingReader::allocateChunk(SINT chunkIndex) { CachingReaderChunkForOwner* pChunk = m_freeChunks.takeFirst(); pChunk->init(chunkIndex); - //kLogger.debug() << "Allocating chunk" << pChunk << pChunk->getIndex(); m_allocatedCachingReaderChunks.insert(chunkIndex, pChunk); return pChunk; @@ -237,9 +236,10 @@ CachingReader::ReadResult CachingReader::read(SINT startSample, SINT numSamples, // Refuse to read from an invalid number of samples (numSamples % CachingReaderChunk::kChannels == 0) && (numSamples >= 0)) { kLogger.critical() - << "read() invalid arguments:" + << "Invalid arguments for read():" << "startSample =" << startSample - << "numSamples =" << numSamples; + << "numSamples =" << numSamples + << "reverse =" << reverse; return ReadResult::UNAVAILABLE; } VERIFY_OR_DEBUG_ASSERT(buffer) { @@ -290,10 +290,12 @@ CachingReader::ReadResult CachingReader::read(SINT startSample, SINT numSamples, remainingFrameIndexRange.start(), m_readableFrameIndexRange.start()); DEBUG_ASSERT(prerollFrameIndexRange.length() <= remainingFrameIndexRange.length()); - kLogger.debug() - << "Prepending" - << prerollFrameIndexRange.length() - << "frames of silence"; + if (kLogger.traceEnabled()) { + kLogger.trace() + << "Prepending" + << prerollFrameIndexRange.length() + << "frames of silence"; + } const SINT prerollFrames = prerollFrameIndexRange.length(); const SINT prerollSamples = CachingReaderChunk::frames2samples(prerollFrames); DEBUG_ASSERT(samplesRemaining >= prerollSamples); From 77b9013401bfbf9f302601f47c3c97709b119690 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 22:34:07 +0200 Subject: [PATCH 12/13] Limit the number of pending, in-flight read request (FIFO) --- src/engine/cachingreader.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index 098b813289e..54d98cd9a73 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -43,7 +43,19 @@ const SINT kNumberOfCachedChunksInMemory = 80; CachingReader::CachingReader(QString group, UserSettingsPointer config) : m_pConfig(config), - m_chunkReadRequestFIFO(kNumberOfCachedChunksInMemory), + // Limit the number of in-flight requests to the worker. This should + // prevent to overload the worker when it is not able to fetch those + // requests from the FIFO timely. Otherwise outdated requests pile up + // in the FIFO and it would take a long time to process them, just to + // discard the results that most likely have already become obsolete. + // TODO(XXX): Ideally the request FIFO would be implemented as a ring + // buffer, where new requests replace old requests when full. Those + // old requests need to be returned immediately to the CachingReader + // that must take ownership and free them!!! + m_chunkReadRequestFIFO(kNumberOfCachedChunksInMemory / 4), + // The capacity of the back channel must be equal to the number of + // allocated chunks, because the worker use writeBlocking(). Otherwise + // the worker could get stuck in a hot loop!!! m_readerStatusFIFO(kNumberOfCachedChunksInMemory), m_readerStatus(INVALID), m_mruCachingReaderChunk(nullptr), From 69d2414cb9a3bbbacd2556c4a7e941600f621343 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 6 Sep 2019 22:39:29 +0200 Subject: [PATCH 13/13] Fix blocking write for FIFO --- src/util/fifo.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/fifo.h b/src/util/fifo.h index 3ffd03e7663..1355430ddd7 100644 --- a/src/util/fifo.h +++ b/src/util/fifo.h @@ -43,10 +43,8 @@ class FIFO { } void writeBlocking(const DataType* pData, int count) { int written = 0; - while (written != count) { - int i = write(pData, count); - pData += i; - written += i; + while (written < count) { + written += write(pData + written, count - written); } } int aquireWriteRegions(int count,