From 43a7df2de34de043f6fee77e4077c3f4f0286575 Mon Sep 17 00:00:00 2001 From: Jean Pierre Cimalando Date: Sun, 19 Apr 2020 18:05:43 +0200 Subject: [PATCH 1/5] Do some cleanup in the Buffer class --- src/sfizz/AudioBuffer.h | 17 +++- src/sfizz/Buffer.h | 166 +++++++++++++++++++++++----------------- tests/BufferT.cpp | 20 ++--- 3 files changed, 119 insertions(+), 84 deletions(-) diff --git a/src/sfizz/AudioBuffer.h b/src/sfizz/AudioBuffer.h index dcd80588..931402de 100644 --- a/src/sfizz/AudioBuffer.h +++ b/src/sfizz/AudioBuffer.h @@ -64,12 +64,12 @@ public: * @return true if the resize worked * @return false otherwise */ - bool resize(size_t newSize) + bool resize(size_t newSize, std::nothrow_t) noexcept { bool returnedOK = true; for (size_t i = 0; i < numChannels; ++i) - returnedOK &= buffers[i]->resize(newSize); + returnedOK &= buffers[i]->resize(newSize, std::nothrow); if (returnedOK) numFrames = newSize; @@ -77,6 +77,19 @@ public: return returnedOK; } + /** + * @brief Resizes all the underlying buffers to a new size. + * + * @param newSize + */ + void resize(size_t newSize) + { + for (size_t i = 0; i < numChannels; ++i) + buffers[i]->resize(newSize); + + numFrames = newSize; + } + /** * @brief Return an iterator to a specific channel with a non-const type. * diff --git a/src/sfizz/Buffer.h b/src/sfizz/Buffer.h index c48372bc..a93fb990 100644 --- a/src/sfizz/Buffer.h +++ b/src/sfizz/Buffer.h @@ -45,8 +45,8 @@ namespace sfz class BufferCounter { public: - BufferCounter() = default; - ~BufferCounter() + BufferCounter() noexcept = default; + ~BufferCounter() noexcept { #ifndef NDEBUG std::cout << "Remaining buffers on destruction: " << numBuffers << '\n'; @@ -54,41 +54,41 @@ public: #endif } - void newBuffer(int size) + void newBuffer(int size) noexcept { numBuffers++; bytes.fetch_add(size); } - void bufferResized(int oldSize, int newSize) + void bufferResized(int oldSize, int newSize) noexcept { bytes.fetch_add(newSize); bytes.fetch_sub(oldSize); } - void bufferDeleted(int size) + void bufferDeleted(int size) noexcept { numBuffers--; bytes.fetch_sub(size); } - void bufferDeleted(size_t size) + void bufferDeleted(size_t size) noexcept { bufferDeleted(static_cast(size)); } - void bufferResized(size_t oldSize, size_t newSize) + void bufferResized(size_t oldSize, size_t newSize) noexcept { bufferResized(static_cast(oldSize), static_cast(newSize)); } - void newBuffer(size_t size) + void newBuffer(size_t size) noexcept { newBuffer(static_cast(size)); } - int getNumBuffers() { return numBuffers; } - int getTotalBytes() { return bytes; } + int getNumBuffers() const noexcept { return numBuffers; } + int getTotalBytes() const noexcept { return bytes; } private: std::atomic numBuffers { 0 }; std::atomic bytes { 0 }; @@ -128,9 +128,8 @@ public: * @brief Construct a new Buffer object that is empty * */ - Buffer() + Buffer() noexcept { - counter().newBuffer(0); } /** @@ -138,9 +137,8 @@ public: * * @param size */ - Buffer(size_t size) + explicit Buffer(size_t size) { - counter().newBuffer(0); resize(size); } @@ -152,7 +150,7 @@ public: * @return true if allocation succeeded * @return false otherwise */ - bool resize(size_t newSize) + bool resize(size_t newSize, std::nothrow_t) noexcept { if (newSize == 0) { clear(); @@ -160,19 +158,24 @@ public: } auto tempSize = newSize + 2 * AlignmentMask; // To ensure that we have leeway at the beginning and at the end - auto* newData = paddedData != nullptr ? std::realloc(paddedData, tempSize * sizeof(value_type)) : std::malloc(tempSize * sizeof(value_type)); + auto* newData = std::realloc(paddedData.get(), tempSize * sizeof(value_type)); if (newData == nullptr) { return false; } - counter().bufferResized(largerSize * sizeof(value_type), tempSize * sizeof(value_type)); + if (largerSize > 0) + _counter.bufferResized(largerSize * sizeof(value_type), tempSize * sizeof(value_type)); + else + _counter.newBuffer(tempSize * sizeof(value_type)); + largerSize = tempSize; alignedSize = newSize; - paddedData = static_cast(newData); + paddedData.release(); // realloc has invalidated the old pointer + paddedData.reset(static_cast(newData)); normalData = static_cast(align(Alignment, alignedSize, newData, tempSize)); normalEnd = normalData + alignedSize; - auto endMisalignment = (alignedSize & TypeAlignmentMask); - if (endMisalignment != 0) + auto endMisalignment = (alignedSize & TypeAlignmentMask); + if (endMisalignment != 0) _alignedEnd = normalEnd + Alignment - endMisalignment; else _alignedEnd = normalEnd; @@ -180,24 +183,32 @@ public: return true; } + /** + * @brief Resizes the buffer. Given that std::realloc may return either the same pointer + * or a new one, you need to account for both cases in the code if you are using deep pointers. + * + * @param newSize the new buffer size in bytes + */ + void resize(size_t newSize) + { + if (!resize(newSize, std::nothrow)) + throw std::bad_alloc(); + } + /** * @brief Clear the buffers and frees the underlying memory * */ - void clear() + void clear() noexcept { - counter().bufferResized(largerSize * sizeof(value_type), static_cast(0)); - largerSize = 0; - alignedSize = 0; - std::free(paddedData); - normalData = nullptr; - normalEnd = nullptr; - _alignedEnd = nullptr; + if (largerSize > 0) + _counter.bufferDeleted(largerSize * sizeof(value_type)); + _clear(); } - ~Buffer() + ~Buffer() noexcept { - counter().bufferDeleted(largerSize * sizeof(value_type)); - std::free(paddedData); + if (largerSize > 0) + _counter.bufferDeleted(largerSize * sizeof(value_type)); } /** @@ -207,10 +218,8 @@ public: */ Buffer(const Buffer& other) { - counter().newBuffer(0); - if (resize(other.size())) { - std::memcpy(this->data(), other.data(), other.size() * sizeof(value_type)); - } + resize(other.size()); + std::memcpy(this->data(), other.data(), other.size() * sizeof(value_type)); } /** @@ -218,45 +227,41 @@ public: * * @param other */ - Buffer(Buffer&& other) = delete; - // { - // if (this != &other) { - // counter().bufferDeleted(largerSize * sizeof(value_type)); - // std::free(paddedData); - // largerSize = std::exchange(other.largerSize, 0); - // alignedSize = std::exchange(other.alignedSize, 0); - // paddedData = std::exchange(other.paddedData, nullptr); - // normalData = std::exchange(other.normalData, nullptr); - // normalEnd = std::exchange(other.normalEnd, nullptr); - // _alignedEnd = std::exchange(other._alignedEnd, nullptr); - // } - // } + Buffer(Buffer&& other) noexcept + : largerSize(other.largerSize), + alignedSize(other.alignedSize), + normalData(other.normalData), + paddedData(std::move(other.paddedData)), + normalEnd(other.normalEnd), + _alignedEnd(other._alignedEnd) + { + other._clear(); + } Buffer& operator=(const Buffer& other) { if (this != &other) { - if (resize(other.size())) - std::memcpy(this->data(), other.data(), other.size() * sizeof(value_type)); + resize(other.size()); + std::memcpy(this->data(), other.data(), other.size() * sizeof(value_type)); } return *this; } - Buffer& operator=(Buffer&& other) = delete; - // { - // if (this != &other) { - // counter().bufferDeleted(largerSize * sizeof(value_type)); - // std::free(paddedData); - // largerSize = std::exchange(other.largerSize, 0); - // alignedSize = std::exchange(other.alignedSize, 0); - // paddedData = std::exchange(other.paddedData, nullptr); - // normalData = std::exchange(other.normalData, nullptr); - // normalEnd = std::exchange(other.normalEnd, nullptr); - // _alignedEnd = std::exchange(other._alignedEnd, nullptr); - // } - // return *this; - // } + Buffer& operator=(Buffer&& other) noexcept + { + if (this != &other) { + largerSize = other.largerSize; + alignedSize = other.alignedSize; + normalData = other.normalData; + paddedData = std::move(other.paddedData); + normalEnd = other.normalEnd; + _alignedEnd = other._alignedEnd; + other._clear(); + } + return *this; + } - Type& operator[](size_t idx) { return *(normalData + idx); } + Type& operator[](size_t idx) noexcept { return *(normalData + idx); } constexpr pointer data() const noexcept { return normalData; } constexpr size_type size() const noexcept { return alignedSize; } constexpr bool empty() const noexcept { return alignedSize == 0; } @@ -264,19 +269,27 @@ public: constexpr iterator end() const noexcept { return normalEnd; } constexpr pointer alignedEnd() const noexcept { return _alignedEnd; } - /** * @brief Return the buffer counter object. * - * TODO: In C++ 17 all of this can be static inline. - * * @return The buffer counter on which you can call various methods. */ - static BufferCounter& counter() + static BufferCounter& counter() noexcept { - static BufferCounter counter; - return counter; + return _counter; } + +private: + void _clear() + { + largerSize = 0; + alignedSize = 0; + paddedData.reset(); + normalData = nullptr; + normalEnd = nullptr; + _alignedEnd = nullptr; + } + private: static constexpr int AlignmentMask { Alignment - 1 }; static constexpr int TypeAlignment { Alignment / sizeof(value_type) }; @@ -295,13 +308,22 @@ private: return ptr = reinterpret_cast< void * >( aligned ); } + struct deleter { + void operator()(void *p) const noexcept { std::free(p); } + }; + size_type largerSize { 0 }; size_type alignedSize { 0 }; pointer normalData { nullptr }; - pointer paddedData { nullptr }; + std::unique_ptr paddedData; pointer normalEnd { nullptr }; pointer _alignedEnd { nullptr }; + static BufferCounter _counter; + LEAK_DETECTOR(Buffer); }; +template +BufferCounter Buffer::_counter; + } diff --git a/tests/BufferT.cpp b/tests/BufferT.cpp index 04e38724..f886f4d9 100644 --- a/tests/BufferT.cpp +++ b/tests/BufferT.cpp @@ -70,12 +70,12 @@ TEST_CASE("[Buffer] Resize 10 floats ") std::fill(buffer.begin(), buffer.end(), 1.0f); - REQUIRE(buffer.resize(smallSize)); + REQUIRE(buffer.resize(smallSize, std::nothrow)); checkBoundaries(buffer, smallSize); REQUIRE(std::all_of(buffer.begin(), buffer.end(), [](float value) { return value == 1.0f; })); - REQUIRE(buffer.resize(bigSize)); + REQUIRE(buffer.resize(bigSize, std::nothrow)); checkBoundaries(buffer, bigSize); for (auto i = 0; i < smallSize; ++i) REQUIRE(buffer[i] == 1.0f); @@ -92,12 +92,12 @@ TEST_CASE("[Buffer] Resize 4096 floats ") std::fill(buffer.begin(), buffer.end(), 1.0f); - REQUIRE(buffer.resize(smallSize)); + REQUIRE(buffer.resize(smallSize, std::nothrow)); checkBoundaries(buffer, smallSize); REQUIRE(std::all_of(buffer.begin(), buffer.end(), [](float value) { return value == 1.0f; })); - REQUIRE(buffer.resize(bigSize)); + REQUIRE(buffer.resize(bigSize, std::nothrow)); checkBoundaries(buffer, bigSize); for (auto i = 0; i < smallSize; ++i) REQUIRE(buffer[i] == 1.0f); @@ -114,12 +114,12 @@ TEST_CASE("[Buffer] Resize 65536 floats ") std::fill(buffer.begin(), buffer.end(), 1.0f); - REQUIRE(buffer.resize(smallSize)); + REQUIRE(buffer.resize(smallSize, std::nothrow)); checkBoundaries(buffer, smallSize); REQUIRE(std::all_of(buffer.begin(), buffer.end(), [](float value) { return value == 1.0f; })); - REQUIRE(buffer.resize(bigSize)); + REQUIRE(buffer.resize(bigSize, std::nothrow)); checkBoundaries(buffer, bigSize); for (auto i = 0; i < smallSize; ++i) REQUIRE(buffer[i] == 1.0f); @@ -140,8 +140,8 @@ TEST_CASE("[Buffer] Copy and move") checkBoundaries(copyConstructed, baseSize); REQUIRE(std::all_of(copyConstructed.begin(), copyConstructed.end(), [](float value) { return value == 1.0f; })); - // sfz::Buffer moveConstructed { std::move(buffer) }; - // REQUIRE(buffer.empty()); - // checkBoundaries(moveConstructed, baseSize); - // REQUIRE(std::all_of(moveConstructed.begin(), moveConstructed.end(), [](auto value) { return value == 1.0f; })); + sfz::Buffer moveConstructed { std::move(buffer) }; + REQUIRE(buffer.empty()); + checkBoundaries(moveConstructed, baseSize); + REQUIRE(std::all_of(moveConstructed.begin(), moveConstructed.end(), [](float value) { return value == 1.0f; })); } From f7e80452c4d2a217c2a3758296d3c1b06ac20c68 Mon Sep 17 00:00:00 2001 From: Jean Pierre Cimalando Date: Sun, 19 Apr 2020 18:31:27 +0200 Subject: [PATCH 2/5] Forgot to count the buffer deletion in move operator= --- src/sfizz/Buffer.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sfizz/Buffer.h b/src/sfizz/Buffer.h index a93fb990..ea47bb2b 100644 --- a/src/sfizz/Buffer.h +++ b/src/sfizz/Buffer.h @@ -250,6 +250,8 @@ public: Buffer& operator=(Buffer&& other) noexcept { if (this != &other) { + if (largerSize > 0) + _counter.bufferDeleted(largerSize * sizeof(value_type)); largerSize = other.largerSize; alignedSize = other.alignedSize; normalData = other.normalData; From 4562201455e33e69bb7f90c290f4f9e623e80471 Mon Sep 17 00:00:00 2001 From: Jean Pierre Cimalando Date: Sun, 19 Apr 2020 18:48:11 +0200 Subject: [PATCH 3/5] Add some tests for the buffer counter --- src/sfizz/Buffer.h | 8 ++++++++ tests/BufferT.cpp | 49 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/src/sfizz/Buffer.h b/src/sfizz/Buffer.h index ea47bb2b..5f75a888 100644 --- a/src/sfizz/Buffer.h +++ b/src/sfizz/Buffer.h @@ -211,6 +211,14 @@ public: _counter.bufferDeleted(largerSize * sizeof(value_type)); } + /** + * @brief Get the size of the larger block which is actually allocated + */ + size_t allocationSize() const noexcept + { + return largerSize; + } + /** * @brief Construct a new Buffer object from an existing one * diff --git a/tests/BufferT.cpp b/tests/BufferT.cpp index f886f4d9..dfad1483 100644 --- a/tests/BufferT.cpp +++ b/tests/BufferT.cpp @@ -145,3 +145,52 @@ TEST_CASE("[Buffer] Copy and move") checkBoundaries(moveConstructed, baseSize); REQUIRE(std::all_of(moveConstructed.begin(), moveConstructed.end(), [](float value) { return value == 1.0f; })); } + +TEST_CASE("[Buffer] Buffer counter") +{ + sfz::BufferCounter& counter = sfz::Buffer::counter(); + + REQUIRE(counter.getNumBuffers() == 0); + REQUIRE(counter.getTotalBytes() == 0); + + // create an empty buffer + sfz::Buffer b1; + REQUIRE(counter.getNumBuffers() == 0); + REQUIRE(counter.getTotalBytes() == 0); + + // clear an empty buffer + b1.clear(); + REQUIRE(counter.getNumBuffers() == 0); + REQUIRE(counter.getTotalBytes() == 0); + + // create a sized buffer + sfz::Buffer b2(5); + REQUIRE(counter.getNumBuffers() == 1); + REQUIRE(counter.getTotalBytes() == (b2.allocationSize()) * sizeof(float)); + + // resize an empty buffer + b1.resize(3); + REQUIRE(counter.getNumBuffers() == 2); + REQUIRE(counter.getTotalBytes() == (b1.allocationSize() + b2.allocationSize()) * sizeof(float)); + + // resize a non-empty buffer + b1.resize(7); + REQUIRE(counter.getNumBuffers() == 2); + REQUIRE(counter.getTotalBytes() == (b1.allocationSize() + b2.allocationSize()) * sizeof(float)); + + // clear a non-empty buffer + b2.clear(); + REQUIRE(counter.getNumBuffers() == 1); + REQUIRE(counter.getTotalBytes() == (b1.allocationSize()) * sizeof(float)); + + // move an empty buffer into a non-empty one + b1 = std::move(b2); + REQUIRE(counter.getNumBuffers() == 0); + REQUIRE(counter.getTotalBytes() == 0); + + // move a non-empty buffer into an empty one + b1.resize(3); + b2 = std::move(b1); + REQUIRE(counter.getNumBuffers() == 1); + REQUIRE(counter.getTotalBytes() == (b2.allocationSize()) * sizeof(float)); +} From 6ac903ac8fac3c89f92504205201f29993090c3b Mon Sep 17 00:00:00 2001 From: Jean Pierre Cimalando Date: Sun, 19 Apr 2020 19:15:06 +0200 Subject: [PATCH 4/5] Make the counter really singleton, remove non-working static init --- src/sfizz/Buffer.h | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/sfizz/Buffer.h b/src/sfizz/Buffer.h index 5f75a888..89acfa36 100644 --- a/src/sfizz/Buffer.h +++ b/src/sfizz/Buffer.h @@ -45,6 +45,16 @@ namespace sfz class BufferCounter { public: + /** + * @brief Return the buffer counter object. + */ + static BufferCounter& counter() noexcept + { + static BufferCounter counter; + return counter; + } + +protected: BufferCounter() noexcept = default; ~BufferCounter() noexcept { @@ -54,6 +64,7 @@ public: #endif } +public: void newBuffer(int size) noexcept { numBuffers++; @@ -164,9 +175,9 @@ public: } if (largerSize > 0) - _counter.bufferResized(largerSize * sizeof(value_type), tempSize * sizeof(value_type)); + counter().bufferResized(largerSize * sizeof(value_type), tempSize * sizeof(value_type)); else - _counter.newBuffer(tempSize * sizeof(value_type)); + counter().newBuffer(tempSize * sizeof(value_type)); largerSize = tempSize; alignedSize = newSize; @@ -202,13 +213,13 @@ public: void clear() noexcept { if (largerSize > 0) - _counter.bufferDeleted(largerSize * sizeof(value_type)); + counter().bufferDeleted(largerSize * sizeof(value_type)); _clear(); } ~Buffer() noexcept { if (largerSize > 0) - _counter.bufferDeleted(largerSize * sizeof(value_type)); + counter().bufferDeleted(largerSize * sizeof(value_type)); } /** @@ -259,7 +270,7 @@ public: { if (this != &other) { if (largerSize > 0) - _counter.bufferDeleted(largerSize * sizeof(value_type)); + counter().bufferDeleted(largerSize * sizeof(value_type)); largerSize = other.largerSize; alignedSize = other.alignedSize; normalData = other.normalData; @@ -286,7 +297,7 @@ public: */ static BufferCounter& counter() noexcept { - return _counter; + return BufferCounter::counter(); } private: @@ -328,12 +339,7 @@ private: std::unique_ptr paddedData; pointer normalEnd { nullptr }; pointer _alignedEnd { nullptr }; - static BufferCounter _counter; - LEAK_DETECTOR(Buffer); }; -template -BufferCounter Buffer::_counter; - } From 99bce82ba4950d61467957f7ec8a2cc2e19fded8 Mon Sep 17 00:00:00 2001 From: Jean Pierre Cimalando Date: Sun, 19 Apr 2020 19:31:52 +0200 Subject: [PATCH 5/5] Fix the test --- tests/BufferT.cpp | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/tests/BufferT.cpp b/tests/BufferT.cpp index dfad1483..8d43291d 100644 --- a/tests/BufferT.cpp +++ b/tests/BufferT.cpp @@ -150,47 +150,54 @@ TEST_CASE("[Buffer] Buffer counter") { sfz::BufferCounter& counter = sfz::Buffer::counter(); - REQUIRE(counter.getNumBuffers() == 0); - REQUIRE(counter.getTotalBytes() == 0); + // handle the eventuality that the buffer counter does not start at zero + const int initialNumBuffers = counter.getNumBuffers(); + const int initialTotalBytes = counter.getTotalBytes(); + auto haveNumBuffers = [&](int n) -> bool { + return n == counter.getNumBuffers() - initialNumBuffers; + }; + auto haveTotalAllocation = [&](int n) -> bool { + return n * static_cast(sizeof(float)) == counter.getTotalBytes() - initialTotalBytes; + }; // create an empty buffer sfz::Buffer b1; - REQUIRE(counter.getNumBuffers() == 0); - REQUIRE(counter.getTotalBytes() == 0); + REQUIRE(haveNumBuffers(0)); + REQUIRE(haveTotalAllocation(0)); // clear an empty buffer b1.clear(); - REQUIRE(counter.getNumBuffers() == 0); - REQUIRE(counter.getTotalBytes() == 0); + REQUIRE(haveNumBuffers(0)); + REQUIRE(haveTotalAllocation(0)); // create a sized buffer sfz::Buffer b2(5); - REQUIRE(counter.getNumBuffers() == 1); - REQUIRE(counter.getTotalBytes() == (b2.allocationSize()) * sizeof(float)); + REQUIRE(haveNumBuffers(1)); + REQUIRE(haveTotalAllocation(b2.allocationSize())); // resize an empty buffer b1.resize(3); - REQUIRE(counter.getNumBuffers() == 2); - REQUIRE(counter.getTotalBytes() == (b1.allocationSize() + b2.allocationSize()) * sizeof(float)); + REQUIRE(haveNumBuffers(2)); + REQUIRE(haveTotalAllocation(b1.allocationSize() + b2.allocationSize())); // resize a non-empty buffer b1.resize(7); - REQUIRE(counter.getNumBuffers() == 2); - REQUIRE(counter.getTotalBytes() == (b1.allocationSize() + b2.allocationSize()) * sizeof(float)); + REQUIRE(haveNumBuffers(2)); + REQUIRE(haveTotalAllocation(b1.allocationSize() + b2.allocationSize())); // clear a non-empty buffer b2.clear(); - REQUIRE(counter.getNumBuffers() == 1); - REQUIRE(counter.getTotalBytes() == (b1.allocationSize()) * sizeof(float)); + REQUIRE(haveNumBuffers(1)); + REQUIRE(haveTotalAllocation(b1.allocationSize())); // move an empty buffer into a non-empty one b1 = std::move(b2); - REQUIRE(counter.getNumBuffers() == 0); - REQUIRE(counter.getTotalBytes() == 0); + REQUIRE(haveNumBuffers(0)); + REQUIRE(haveTotalAllocation(0)); // move a non-empty buffer into an empty one b1.resize(3); b2 = std::move(b1); - REQUIRE(counter.getNumBuffers() == 1); - REQUIRE(counter.getTotalBytes() == (b2.allocationSize()) * sizeof(float)); + REQUIRE(haveNumBuffers(1)); + REQUIRE(haveTotalAllocation(b2.allocationSize())); }