From 61d6f7305b02e025ee10aa07e5499993a0e77cc1 Mon Sep 17 00:00:00 2001 From: Matteo Martincigh Date: Thu, 3 Oct 2019 11:21:18 +0100 Subject: IVGCVSW-3440 Fix intermittently failing send thread test * Simplified the implementation of the MockStreamCounterBuffer * Minor refactoring Signed-off-by: Matteo Martincigh Change-Id: I3aed97daee0ac32d384e1f830e8cc57aae84950b --- src/profiling/BufferManager.cpp | 4 +- src/profiling/IBufferManager.hpp | 2 +- src/profiling/SendCounterPacket.cpp | 9 +- src/profiling/test/BufferTests.cpp | 2 +- src/profiling/test/SendCounterPacketTests.cpp | 38 ++++--- src/profiling/test/SendCounterPacketTests.hpp | 136 +++++++++++--------------- 6 files changed, 83 insertions(+), 108 deletions(-) (limited to 'src') diff --git a/src/profiling/BufferManager.cpp b/src/profiling/BufferManager.cpp index 91ad1c5f3a..dbf4466bbb 100644 --- a/src/profiling/BufferManager.cpp +++ b/src/profiling/BufferManager.cpp @@ -32,8 +32,8 @@ std::unique_ptr BufferManager::Reserve(unsigned int requestedSize std::unique_lock availableListLock(m_AvailableMutex, std::defer_lock); if (requestedSize > m_MaxBufferSize) { - throw armnn::RuntimeException("Maximum buffer size that can be requested is [" + - std::to_string(m_MaxBufferSize) + "] bytes"); + throw armnn::InvalidArgumentException("The maximum buffer size that can be requested is [" + + std::to_string(m_MaxBufferSize) + "] bytes"); } availableListLock.lock(); if (m_AvailableList.empty()) diff --git a/src/profiling/IBufferManager.hpp b/src/profiling/IBufferManager.hpp index 190d9c4542..0acdf61b52 100644 --- a/src/profiling/IBufferManager.hpp +++ b/src/profiling/IBufferManager.hpp @@ -18,7 +18,7 @@ namespace profiling class IBufferManager { public: - virtual ~IBufferManager() {}; + virtual ~IBufferManager() {} virtual std::unique_ptr Reserve(unsigned int requestedSize, unsigned int& reservedSize) = 0; diff --git a/src/profiling/SendCounterPacket.cpp b/src/profiling/SendCounterPacket.cpp index 33eaeabc3e..0a2f08b095 100644 --- a/src/profiling/SendCounterPacket.cpp +++ b/src/profiling/SendCounterPacket.cpp @@ -977,17 +977,17 @@ void SendCounterPacket::Send() } // Wait condition lock scope - End - - + // Get the buffer to read from std::unique_ptr packetBuffer = m_BufferManager.GetReadableBuffer(); if (packetBuffer == nullptr) { + // Nothing to read from, ignore and continue continue; } - const unsigned char* readBuffer = packetBuffer->GetReadableData(); + // Get the data to send from the buffer + const unsigned char* readBuffer = packetBuffer->GetReadableData(); unsigned int readBufferSize = packetBuffer->GetSize(); - if (readBuffer == nullptr || readBufferSize == 0) { // Nothing to send, ignore and continue @@ -1001,6 +1001,7 @@ void SendCounterPacket::Send() m_ProfilingConnection.WritePacket(readBuffer, boost::numeric_cast(readBufferSize)); } + // Mark the packet buffer as read m_BufferManager.MarkRead(packetBuffer); } diff --git a/src/profiling/test/BufferTests.cpp b/src/profiling/test/BufferTests.cpp index b678350bb3..a2f3c30849 100644 --- a/src/profiling/test/BufferTests.cpp +++ b/src/profiling/test/BufferTests.cpp @@ -130,7 +130,7 @@ BOOST_AUTO_TEST_CASE(BufferReserveExceedingSpaceTest) unsigned int reservedSize = 0; // Cannot reserve buffer bigger than maximum buffer size - BOOST_CHECK_THROW(bufferManager.Reserve(1024, reservedSize), armnn::RuntimeException); + BOOST_CHECK_THROW(bufferManager.Reserve(1024, reservedSize), armnn::InvalidArgumentException); } BOOST_AUTO_TEST_CASE(BufferExhaustionTest) diff --git a/src/profiling/test/SendCounterPacketTests.cpp b/src/profiling/test/SendCounterPacketTests.cpp index 96596066c1..ba1d470a50 100644 --- a/src/profiling/test/SendCounterPacketTests.cpp +++ b/src/profiling/test/SendCounterPacketTests.cpp @@ -1694,7 +1694,7 @@ BOOST_AUTO_TEST_CASE(SendCounterDirectoryPacketTest7) BOOST_AUTO_TEST_CASE(SendThreadTest0) { MockProfilingConnection mockProfilingConnection; - MockStreamCounterBuffer mockStreamCounterBuffer(1, 0); + MockStreamCounterBuffer mockStreamCounterBuffer(0); SendCounterPacket sendCounterPacket(mockProfilingConnection, mockStreamCounterBuffer); // Try to start the send thread many times, it must only start once @@ -1718,7 +1718,7 @@ BOOST_AUTO_TEST_CASE(SendThreadTest1) unsigned int totalWrittenSize = 0; MockProfilingConnection mockProfilingConnection; - MockStreamCounterBuffer mockStreamCounterBuffer(5,1024); + MockStreamCounterBuffer mockStreamCounterBuffer(1024); SendCounterPacket sendCounterPacket(mockProfilingConnection, mockStreamCounterBuffer); sendCounterPacket.Start(); @@ -1732,8 +1732,7 @@ BOOST_AUTO_TEST_CASE(SendThreadTest1) // Get the size of the Stream Metadata Packet std::string processName = GetProcessName().substr(0, 60); - unsigned int processNameSize = boost::numeric_cast(processName.size()) > 0 ? - boost::numeric_cast(processName.size()) + 1 : 0; + unsigned int processNameSize = processName.empty() ? 0 : boost::numeric_cast(processName.size()) + 1; unsigned int streamMetadataPacketsize = 118 + processNameSize; totalWrittenSize += streamMetadataPacketsize; @@ -1811,15 +1810,15 @@ BOOST_AUTO_TEST_CASE(SendThreadTest1) sendCounterPacket.SetReadyToRead(); - // To test an exact value of the "read size" in the mock buffer, wait a second to allow the send thread to + // To test an exact value of the "read size" in the mock buffer, wait two seconds to allow the send thread to // read all what's remaining in the buffer std::this_thread::sleep_for(std::chrono::seconds(2)); sendCounterPacket.Stop(); - BOOST_CHECK(mockStreamCounterBuffer.GetReadableBufferSize() == totalWrittenSize); BOOST_CHECK(mockStreamCounterBuffer.GetCommittedSize() == totalWrittenSize); - BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() == totalWrittenSize); + BOOST_CHECK(mockStreamCounterBuffer.GetReadableSize() == totalWrittenSize); + BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() == totalWrittenSize); } BOOST_AUTO_TEST_CASE(SendThreadTest2) @@ -1827,7 +1826,7 @@ BOOST_AUTO_TEST_CASE(SendThreadTest2) unsigned int totalWrittenSize = 0; MockProfilingConnection mockProfilingConnection; - MockStreamCounterBuffer mockStreamCounterBuffer(5, 1024); + MockStreamCounterBuffer mockStreamCounterBuffer(1024); SendCounterPacket sendCounterPacket(mockProfilingConnection, mockStreamCounterBuffer); sendCounterPacket.Start(); @@ -1843,8 +1842,7 @@ BOOST_AUTO_TEST_CASE(SendThreadTest2) // Get the size of the Stream Metadata Packet std::string processName = GetProcessName().substr(0, 60); - unsigned int processNameSize = boost::numeric_cast(processName.size()) > 0 ? - boost::numeric_cast(processName.size()) + 1 : 0; + unsigned int processNameSize = processName.empty() ? 0 : boost::numeric_cast(processName.size()) + 1; unsigned int streamMetadataPacketsize = 118 + processNameSize; totalWrittenSize += streamMetadataPacketsize; @@ -1932,15 +1930,15 @@ BOOST_AUTO_TEST_CASE(SendThreadTest2) sendCounterPacket.SetReadyToRead(); - // To test an exact value of the "read size" in the mock buffer, wait a second to allow the send thread to + // To test an exact value of the "read size" in the mock buffer, wait two seconds to allow the send thread to // read all what's remaining in the buffer std::this_thread::sleep_for(std::chrono::seconds(2)); sendCounterPacket.Stop(); - BOOST_CHECK(mockStreamCounterBuffer.GetReadableBufferSize() == totalWrittenSize); BOOST_CHECK(mockStreamCounterBuffer.GetCommittedSize() == totalWrittenSize); - BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() == totalWrittenSize); + BOOST_CHECK(mockStreamCounterBuffer.GetReadableSize() == totalWrittenSize); + BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() == totalWrittenSize); } BOOST_AUTO_TEST_CASE(SendThreadTest3) @@ -1948,7 +1946,7 @@ BOOST_AUTO_TEST_CASE(SendThreadTest3) unsigned int totalWrittenSize = 0; MockProfilingConnection mockProfilingConnection; - MockStreamCounterBuffer mockStreamCounterBuffer(10, 1024); + MockStreamCounterBuffer mockStreamCounterBuffer(1024); SendCounterPacket sendCounterPacket(mockProfilingConnection, mockStreamCounterBuffer); sendCounterPacket.Start(); @@ -1961,8 +1959,7 @@ BOOST_AUTO_TEST_CASE(SendThreadTest3) // Get the size of the Stream Metadata Packet std::string processName = GetProcessName().substr(0, 60); - unsigned int processNameSize = boost::numeric_cast(processName.size()) > 0 ? - boost::numeric_cast(processName.size()) + 1 : 0; + unsigned int processNameSize = processName.empty() ? 0 : boost::numeric_cast(processName.size()) + 1; unsigned int streamMetadataPacketsize = 118 + processNameSize; totalWrittenSize += streamMetadataPacketsize; @@ -2040,10 +2037,11 @@ BOOST_AUTO_TEST_CASE(SendThreadTest3) // thread is not guaranteed to flush the buffer) sendCounterPacket.Stop(); - BOOST_CHECK(mockStreamCounterBuffer.GetReadableBufferSize() <= totalWrittenSize); - BOOST_CHECK(mockStreamCounterBuffer.GetCommittedSize() <= totalWrittenSize); - BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() <= totalWrittenSize); - BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() <= mockStreamCounterBuffer.GetCommittedSize()); + BOOST_CHECK(mockStreamCounterBuffer.GetCommittedSize() == totalWrittenSize); + BOOST_CHECK(mockStreamCounterBuffer.GetReadableSize() <= totalWrittenSize); + BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() <= totalWrittenSize); + BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() <= mockStreamCounterBuffer.GetReadableSize()); + BOOST_CHECK(mockStreamCounterBuffer.GetReadSize() <= mockStreamCounterBuffer.GetCommittedSize()); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/profiling/test/SendCounterPacketTests.hpp b/src/profiling/test/SendCounterPacketTests.hpp index 243731cb2c..3c3427c4c7 100644 --- a/src/profiling/test/SendCounterPacketTests.hpp +++ b/src/profiling/test/SendCounterPacketTests.hpp @@ -46,11 +46,10 @@ class MockPacketBuffer : public IPacketBuffer { public: MockPacketBuffer(unsigned int maxSize) - : m_MaxSize(maxSize), - m_Size(0) - { - m_Data = std::make_unique(m_MaxSize); - } + : m_MaxSize(maxSize) + , m_Size(0) + , m_Data(std::make_unique(m_MaxSize)) + {} ~MockPacketBuffer() {} @@ -58,7 +57,7 @@ public: unsigned int GetSize() const override { return m_Size; } - void MarkRead() override { m_Size = 0;} + void MarkRead() override { m_Size = 0; } void Commit(unsigned int size) override { m_Size = size; } @@ -126,113 +125,90 @@ private: class MockStreamCounterBuffer : public IBufferManager { public: - MockStreamCounterBuffer(unsigned int numberOfBuffers = 5, unsigned int maxPacketSize = 4096) - : m_MaxBufferSize(maxPacketSize) - , m_ReadableSize(0) - , m_CommittedSize(0) - , m_ReadSize(0) - { - m_AvailableList.reserve(numberOfBuffers); - for (unsigned int i = 0; i < numberOfBuffers; ++i) - { - std::unique_ptr buffer = std::make_unique(maxPacketSize); - m_AvailableList.emplace_back(std::move(buffer)); - } - m_ReadableList.reserve(numberOfBuffers); - } - + using IPacketBufferPtr = std::unique_ptr; + + MockStreamCounterBuffer(unsigned int maxBufferSize = 4096) + : m_MaxBufferSize(maxBufferSize) + , m_BufferList() + , m_CommittedSize(0) + , m_ReadableSize(0) + , m_ReadSize(0) + {} ~MockStreamCounterBuffer() {} - std::unique_ptr Reserve(unsigned int requestedSize, unsigned int& reservedSize) override + IPacketBufferPtr Reserve(unsigned int requestedSize, unsigned int& reservedSize) override { - std::unique_lock availableListLock(m_AvailableMutex, std::defer_lock); + std::unique_lock lock(m_Mutex); + + reservedSize = 0; if (requestedSize > m_MaxBufferSize) { - throw armnn::Exception("Maximum buffer size that can be requested is [" + - std::to_string(m_MaxBufferSize) + "] bytes"); - } - availableListLock.lock(); - if (m_AvailableList.empty()) - { - throw armnn::profiling::BufferExhaustion("Buffer not available"); + throw armnn::InvalidArgumentException("The maximum buffer size that can be requested is [" + + std::to_string(m_MaxBufferSize) + "] bytes"); } - std::unique_ptr buffer = std::move(m_AvailableList.back()); - m_AvailableList.pop_back(); - availableListLock.unlock(); reservedSize = requestedSize; - return buffer; + return std::make_unique(requestedSize); } - void Commit(std::unique_ptr& packetBuffer, unsigned int size) override + void Commit(IPacketBufferPtr& packetBuffer, unsigned int size) override { - std::unique_lock readableListLock(m_ReadableMutex, std::defer_lock); - packetBuffer.get()->Commit(size); - readableListLock.lock(); - m_ReadableList.push_back(std::move(packetBuffer)); - readableListLock.unlock(); - m_ReadDataAvailable.notify_one(); + std::unique_lock lock(m_Mutex); + + packetBuffer->Commit(size); + m_BufferList.push_back(std::move(packetBuffer)); m_CommittedSize += size; } - void Release(std::unique_ptr& packetBuffer) override + void Release(IPacketBufferPtr& packetBuffer) override { - std::unique_lock availableListLock(m_AvailableMutex, std::defer_lock); - packetBuffer.get()->Release(); - availableListLock.lock(); - m_AvailableList.push_back(std::move(packetBuffer)); - availableListLock.unlock(); - m_CommittedSize = 0; - m_ReadSize = 0; - m_ReadableSize = 0; + std::unique_lock lock(m_Mutex); + + packetBuffer->Release(); } - std::unique_ptr GetReadableBuffer() override + IPacketBufferPtr GetReadableBuffer() override { - std::unique_lock readableListLock(m_ReadableMutex); - if (!m_ReadableList.empty()) + std::unique_lock lock(m_Mutex); + + if (m_BufferList.empty()) { - std::unique_ptr buffer = std::move(m_ReadableList.back()); - m_ReadableSize+=buffer->GetSize(); - m_ReadableList.pop_back(); - readableListLock.unlock(); - return buffer; + return nullptr; } - return nullptr; + IPacketBufferPtr buffer = std::move(m_BufferList.back()); + m_BufferList.pop_back(); + m_ReadableSize += buffer->GetSize(); + return buffer; } - void MarkRead(std::unique_ptr& packetBuffer) override + void MarkRead(IPacketBufferPtr& packetBuffer) override { - std::unique_lock availableListLock(m_AvailableMutex, std::defer_lock); - // increase read size + std::unique_lock lock(m_Mutex); + m_ReadSize += packetBuffer->GetSize(); packetBuffer->MarkRead(); - availableListLock.lock(); - m_AvailableList.push_back(std::move(packetBuffer)); - availableListLock.unlock(); } - unsigned int GetReadableBufferSize() const - { - return m_ReadableSize; - } - unsigned int GetCommittedSize() const { return m_CommittedSize; } - unsigned int GetReadSize() const { return m_ReadSize; } + unsigned int GetCommittedSize() const { return m_CommittedSize; } + unsigned int GetReadableSize() const { return m_ReadableSize; } + unsigned int GetReadSize() const { return m_ReadSize; } private: + // The maximum buffer size when creating a new buffer unsigned int m_MaxBufferSize; - std::vector> m_AvailableList; - std::vector> m_ReadableList; - std::mutex m_AvailableMutex; - std::mutex m_ReadableMutex; - std::condition_variable m_ReadDataAvailable; - // The size of the buffer that can be read - unsigned int m_ReadableSize; + // A list of buffers + std::vector m_BufferList; + + // The mutex to synchronize this mock's methods + std::mutex m_Mutex; - // The size of the buffer that has been committed for reading + // The total size of the buffers that has been committed for reading unsigned int m_CommittedSize; - // The size of the buffer that has already been read + // The total size of the buffers that can be read + unsigned int m_ReadableSize; + + // The total size of the buffers that has already been read unsigned int m_ReadSize; }; -- cgit v1.2.1