From 8d3c6d319983850203c742ff50d774d7dad5e26b Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Mon, 23 Dec 2019 12:02:26 +0100 Subject: [PATCH 1/5] Docs --- src/sfizz/Oversampler.h | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/sfizz/Oversampler.h b/src/sfizz/Oversampler.h index 40135d8f..d464d63e 100644 --- a/src/sfizz/Oversampler.h +++ b/src/sfizz/Oversampler.h @@ -31,15 +31,36 @@ #include "Config.h" namespace sfz { - +/** + * @brief Wraps the internal oversampler in a single function that takes an + * AudioBuffer and oversamples it in another pre-allocated one. The + * Oversampler processes the file in chunks and can signal the frames + * processes using an atomic counter. + */ class Oversampler { public: - Oversampler() = delete; + /** + * @brief Construct a new Oversampler object + * + * @param factor + * @param chunkSize + */ Oversampler(Oversampling factor = Oversampling::x1, size_t chunkSize = config::chunkSize); + /** + * @brief Stream the oversampling of an input AudioBuffer into an output + * one, possibly signaling the caller along the way of the number of + * frames that are written. + * + * @param input + * @param output + * @param framesReady an atomic counter for the ready frames. If null no signaling is done. + */ + void stream(const AudioBuffer& input, AudioBuffer& output, std::atomic* framesReady = nullptr); + + Oversampler() = delete; Oversampler(const Oversampler&) = delete; Oversampler(Oversampler&&) = delete; - void stream(const AudioBuffer& input, AudioBuffer& output, std::atomic* framesReady = nullptr); private: Oversampling factor; size_t chunkSize; From 68dfadb09ef7cf0a42fe67ab1ceafdc624504d7b Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Mon, 23 Dec 2019 12:42:33 +0100 Subject: [PATCH 2/5] Refactor the filepool a bit --- src/sfizz/FilePool.cpp | 15 ++++++++++++ src/sfizz/FilePool.h | 52 ++++++++++++++++++++---------------------- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/src/sfizz/FilePool.cpp b/src/sfizz/FilePool.cpp index a48e1515..7e61dbf3 100644 --- a/src/sfizz/FilePool.cpp +++ b/src/sfizz/FilePool.cpp @@ -85,6 +85,20 @@ void streamFromFile(SndfileHandle& sndFile, uint32_t numFrames, sfz::Oversamplin oversampler.stream(*baseBuffer, output, filledFrames); } +sfz::FilePool::FilePool() + { + for (int i = 0; i < config::numBackgroundThreads; ++i) + threadPool.emplace_back( &FilePool::loadingThread, this ); + threadPool.emplace_back( &FilePool::clearingThread, this ); + } + +sfz::FilePool::~FilePool() +{ + quitThread = true; + for (auto& thread: threadPool) + thread.join(); +} + absl::optional sfz::FilePool::getFileInformation(const std::string& filename) noexcept { fs::path file { rootDirectory / filename }; @@ -227,6 +241,7 @@ void sfz::FilePool::loadingThread() noexcept DBG("Error enqueuing the file for " << promise->filename << " in the filledPromiseQueue"); std::this_thread::sleep_for(1ms); } + promise.reset(); } } diff --git a/src/sfizz/FilePool.h b/src/sfizz/FilePool.h index c6004267..f8423f54 100644 --- a/src/sfizz/FilePool.h +++ b/src/sfizz/FilePool.h @@ -62,46 +62,43 @@ struct FilePromise AudioBufferPtr preloadedData {}; AudioBuffer fileData {}; float sampleRate { config::defaultSampleRate }; + Oversampling oversamplingFactor { config::defaultOversamplingFactor }; std::atomic_size_t availableFrames { 0 }; std::atomic dataReady { false }; - Oversampling oversamplingFactor { config::defaultOversamplingFactor }; }; using FilePromisePtr = std::shared_ptr; /** - * @brief This is a singleton-designed class that holds all the preloaded - * data as well as functions to request new file data and collect the file - * handles to close after they are read. + * @brief This is a singleton-designed class that holds all the preloaded data + * as well as functions to request new file data and collect the file handles to + * close after they are read. * - * This object caches the file data that was already preloaded in case it is asked - * again by a region using the same sample. In this situation, both regions have a - * handle on the same preloaded data. + * This object caches the file data that was already preloaded in case it is + * asked again by a region using the same sample. In this situation, both + * regions have a handle on the same preloaded data. * - * The file request is immediately served using the preloaded data. A ticket is then - * provided to the voice that requested the file, and the file loading happens in the - * background. When the file is fully loaded, the background makes the full data available - * to the voice and consumes the ticket, while conserving a handle on this file. When the - * voice dies it releases its handle on the files, which should decrease the reference count - * to 1. A garbage collection thread then runs regularly to clear the memory of all file - * handles with a reference count of 1. + * The file request is immediately served using the preloaded data. A promise is + * then provided to the voice that requested the file, and the file loading + * happens in the background. File reads happen on whole samples but + * oversampling is done in chunks, and the promise contains a counter for the + * frames that are loaded. When the voice dies it releases its handle on the + * promise, which should decrease the reference count to 1. A garbage + * collection thread then runs regularly to clear the memory of all file handles + * with a reference count of 1. */ class FilePool { public: - FilePool() - { - for (int i = 0; i < config::numBackgroundThreads; ++i) - fileLoadingThreadPool.emplace_back( &FilePool::loadingThread, this ); - fileLoadingThreadPool.emplace_back( &FilePool::clearingThread, this ); - } + /** + * @brief Construct a new File Pool object. + * + * This creates the background threads based on config::numBackgroundThreads + * as well as the garbage collection thread. + */ + FilePool(); - ~FilePool() - { - quitThread = true; - for (auto& thread: fileLoadingThreadPool) - thread.join(); - } + ~FilePool(); /** * @brief Set the root directory from which to search for files to load * @@ -214,13 +211,14 @@ private: bool emptyQueue { false }; std::atomic threadsLoading { 0 }; + // File promises data structures along with their guards. std::vector temporaryFilePromises; std::vector promisesToClear; std::atomic addingPromisesToClear { false }; std::atomic canAddPromisesToClear { true }; absl::flat_hash_map preloadedFiles; - std::vector fileLoadingThreadPool { }; + std::vector threadPool { }; LEAK_DETECTOR(FilePool); }; } From 13f9baed338715a741f666d563e862435d833478 Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Mon, 23 Dec 2019 13:58:42 +0100 Subject: [PATCH 3/5] Added leak detectors on newer objects --- src/sfizz/FilePool.h | 2 ++ src/sfizz/Oversampler.h | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/sfizz/FilePool.h b/src/sfizz/FilePool.h index f8423f54..4007d33a 100644 --- a/src/sfizz/FilePool.h +++ b/src/sfizz/FilePool.h @@ -65,6 +65,8 @@ struct FilePromise Oversampling oversamplingFactor { config::defaultOversamplingFactor }; std::atomic_size_t availableFrames { 0 }; std::atomic dataReady { false }; + + LEAK_DETECTOR(FilePromise); }; using FilePromisePtr = std::shared_ptr; diff --git a/src/sfizz/Oversampler.h b/src/sfizz/Oversampler.h index d464d63e..d05cf756 100644 --- a/src/sfizz/Oversampler.h +++ b/src/sfizz/Oversampler.h @@ -64,6 +64,8 @@ public: private: Oversampling factor; size_t chunkSize; + + LEAK_DETECTOR(Oversampler); }; } From 7379ba37c02c38e731e7639b11e7f6621c0eb6f5 Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Mon, 23 Dec 2019 13:59:43 +0100 Subject: [PATCH 4/5] The file promise logic actually allocated Changed to add a pool of "empty" promises, but I wonder if all of this could be made a bit simpler? --- src/sfizz/Config.h | 1 + src/sfizz/FilePool.cpp | 65 ++++++++++++++++++++++++++++++------------ src/sfizz/FilePool.h | 12 ++++++++ 3 files changed, 60 insertions(+), 18 deletions(-) diff --git a/src/sfizz/Config.h b/src/sfizz/Config.h index 22be0a27..5f949be8 100644 --- a/src/sfizz/Config.h +++ b/src/sfizz/Config.h @@ -40,6 +40,7 @@ namespace config { constexpr int numBackgroundThreads { 4 }; constexpr int numVoices { 64 }; constexpr int maxVoices { 256 }; + constexpr int maxFilePromises { maxVoices * 2 }; constexpr int sustainCC { 64 }; constexpr int allSoundOffCC { 120 }; constexpr int resetCC { 121 }; diff --git a/src/sfizz/FilePool.cpp b/src/sfizz/FilePool.cpp index 7e61dbf3..945be27e 100644 --- a/src/sfizz/FilePool.cpp +++ b/src/sfizz/FilePool.cpp @@ -89,7 +89,11 @@ sfz::FilePool::FilePool() { for (int i = 0; i < config::numBackgroundThreads; ++i) threadPool.emplace_back( &FilePool::loadingThread, this ); + threadPool.emplace_back( &FilePool::clearingThread, this ); + + for (int i = 0; i < config::maxFilePromises; ++i) + emptyPromises.push_back(std::make_shared()); } sfz::FilePool::~FilePool() @@ -161,15 +165,21 @@ bool sfz::FilePool::preloadFile(const std::string& filename, uint32_t maxOffset) sfz::FilePromisePtr sfz::FilePool::getFilePromise(const std::string& filename) noexcept { - auto promise = std::make_shared(); + if (emptyPromises.empty()) + return {}; + const auto preloaded = preloadedFiles.find(filename); - if (preloaded != preloadedFiles.end()) { - promise->filename = preloaded->first; - promise->preloadedData = preloaded->second.preloadedData; - promise->sampleRate = preloaded->second.sampleRate; - promise->oversamplingFactor = oversamplingFactor; - promiseQueue.try_enqueue(promise); - } + if (preloaded == preloadedFiles.end()) + return {}; + + auto& promise = emptyPromises.back(); + promise->filename = preloaded->first; + promise->preloadedData = preloaded->second.preloadedData; + promise->sampleRate = preloaded->second.sampleRate; + promise->oversamplingFactor = oversamplingFactor; + promiseQueue.try_enqueue(promise); + emptyPromises.pop_back(); + return promise; } @@ -193,7 +203,10 @@ void sfz::FilePool::tryToClearPromises() while (addingPromisesToClear) std::this_thread::sleep_for(1ms); - promisesToClear.clear(); + for (auto& promise: promisesToClear) { + if (promise->dataReady) + promise->reset(); + } } void sfz::FilePool::clearingThread() @@ -261,21 +274,37 @@ void sfz::FilePool::cleanupPromises() noexcept if (!canAddPromisesToClear) return; + // The garbage collection cleared the data from these so we can move them + // back to the empty queue + auto clearedIterator = promisesToClear.begin(); + auto clearedSentinel = promisesToClear.end() - 1; + while (clearedIterator != promisesToClear.end()) { + if (clearedIterator->get()->dataReady == false) { + emptyPromises.push_back(*clearedIterator); + std::iter_swap(clearedIterator, clearedSentinel); + clearedSentinel--; + promisesToClear.pop_back(); + } else { + clearedIterator++; + } + } + FilePromisePtr promise; - // Remove stuff from the filled queue and put them in a linear storage + // Remove the promises from the filled queue and put them in a linear + // storage while (filledPromiseQueue.try_dequeue(promise)) temporaryFilePromises.push_back(promise); - auto promiseIterator = temporaryFilePromises.begin(); - auto sentinel = temporaryFilePromises.end() - 1; - while (promiseIterator != temporaryFilePromises.end()) { - if (promiseIterator->use_count() == 1) { - promisesToClear.push_back(*promiseIterator); - std::iter_swap(promiseIterator, sentinel); - sentinel--; + auto filledIterator = temporaryFilePromises.begin(); + auto filledSentinel = temporaryFilePromises.end() - 1; + while (filledIterator != temporaryFilePromises.end()) { + if (filledIterator->use_count() == 1) { + promisesToClear.push_back(*filledIterator); + std::iter_swap(filledIterator, filledSentinel); + filledSentinel--; temporaryFilePromises.pop_back(); } else { - promiseIterator++; + filledIterator++; } } } diff --git a/src/sfizz/FilePool.h b/src/sfizz/FilePool.h index 4007d33a..b01763b4 100644 --- a/src/sfizz/FilePool.h +++ b/src/sfizz/FilePool.h @@ -58,6 +58,17 @@ struct FilePromise return AudioSpan(*preloadedData); } + void reset() + { + fileData.reset(); + preloadedData.reset(); + filename = ""; + availableFrames = 0; + dataReady = false; + oversamplingFactor = config::defaultOversamplingFactor; + sampleRate = config::defaultSampleRate; + } + absl::string_view filename {}; AudioBufferPtr preloadedData {}; AudioBuffer fileData {}; @@ -214,6 +225,7 @@ private: std::atomic threadsLoading { 0 }; // File promises data structures along with their guards. + std::vector emptyPromises; std::vector temporaryFilePromises; std::vector promisesToClear; std::atomic addingPromisesToClear { false }; From b69b40734e2a3fcf4d00fea5f47987935ec7b96d Mon Sep 17 00:00:00 2001 From: Paul Ferrand Date: Mon, 23 Dec 2019 14:07:19 +0100 Subject: [PATCH 5/5] Plug an lsan leak Still not sure it's a real leak but let's pay a copy to be certain! --- src/sfizz/FilePool.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sfizz/FilePool.cpp b/src/sfizz/FilePool.cpp index 945be27e..cb13461f 100644 --- a/src/sfizz/FilePool.cpp +++ b/src/sfizz/FilePool.cpp @@ -172,7 +172,7 @@ sfz::FilePromisePtr sfz::FilePool::getFilePromise(const std::string& filename) n if (preloaded == preloadedFiles.end()) return {}; - auto& promise = emptyPromises.back(); + auto promise = emptyPromises.back(); promise->filename = preloaded->first; promise->preloadedData = preloaded->second.preloadedData; promise->sampleRate = preloaded->second.sampleRate;