IVGCVSW-3440 Fix intermittently failing send thread test

 * Simplified the implementation of the MockStreamCounterBuffer
 * Minor refactoring

Signed-off-by: Matteo Martincigh <matteo.martincigh@arm.com>
Change-Id: I3aed97daee0ac32d384e1f830e8cc57aae84950b

6 files changed, 83 insertions, 108 deletions

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<IPacketBuffer> BufferManager::Reserve(unsigned int requestedSize
     std::unique_lock<std::mutex> 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<IPacketBuffer> 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<IPacketBuffer> 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<uint32_t>(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<unsigned int>(processName.size()) > 0 ?
-                                   boost::numeric_cast<unsigned int>(processName.size()) + 1 : 0;
+    unsigned int processNameSize = processName.empty() ? 0 : boost::numeric_cast<unsigned int>(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<unsigned int>(processName.size()) > 0 ?
-                                   boost::numeric_cast<unsigned int>(processName.size()) + 1 : 0;
+    unsigned int processNameSize = processName.empty() ? 0 : boost::numeric_cast<unsigned int>(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<unsigned int>(processName.size()) > 0 ?
-                                   boost::numeric_cast<unsigned int>(processName.size()) + 1 : 0;
+    unsigned int processNameSize = processName.empty() ? 0 : boost::numeric_cast<unsigned int>(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<unsigned char[]>(m_MaxSize);
-    }
+        : m_MaxSize(maxSize)
+        , m_Size(0)
+        , m_Data(std::make_unique<unsigned char[]>(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<IPacketBuffer> buffer = std::make_unique<MockPacketBuffer>(maxPacketSize);
-            m_AvailableList.emplace_back(std::move(buffer));
-        }
-        m_ReadableList.reserve(numberOfBuffers);
-    }
-
+    using IPacketBufferPtr = std::unique_ptr<IPacketBuffer>;
+
+    MockStreamCounterBuffer(unsigned int maxBufferSize = 4096)
+        : m_MaxBufferSize(maxBufferSize)
+        , m_BufferList()
+        , m_CommittedSize(0)
+        , m_ReadableSize(0)
+        , m_ReadSize(0)
+    {}
     ~MockStreamCounterBuffer() {}
 
-    std::unique_ptr<IPacketBuffer> Reserve(unsigned int requestedSize, unsigned int& reservedSize) override
+    IPacketBufferPtr Reserve(unsigned int requestedSize, unsigned int& reservedSize) override
     {
-        std::unique_lock<std::mutex> availableListLock(m_AvailableMutex, std::defer_lock);
+        std::unique_lock<std::mutex> 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<IPacketBuffer> buffer = std::move(m_AvailableList.back());
-        m_AvailableList.pop_back();
-        availableListLock.unlock();
         reservedSize = requestedSize;
-        return buffer;
+        return std::make_unique<MockPacketBuffer>(requestedSize);
     }
 
-    void Commit(std::unique_ptr<IPacketBuffer>& packetBuffer, unsigned int size) override
+    void Commit(IPacketBufferPtr& packetBuffer, unsigned int size) override
     {
-        std::unique_lock<std::mutex> 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<std::mutex> lock(m_Mutex);
+
+        packetBuffer->Commit(size);
+        m_BufferList.push_back(std::move(packetBuffer));
         m_CommittedSize += size;
     }
 
-    void Release(std::unique_ptr<IPacketBuffer>& packetBuffer) override
+    void Release(IPacketBufferPtr& packetBuffer) override
     {
-        std::unique_lock<std::mutex> 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<std::mutex> lock(m_Mutex);
+
+        packetBuffer->Release();
     }
 
-    std::unique_ptr<IPacketBuffer> GetReadableBuffer() override
+    IPacketBufferPtr GetReadableBuffer() override
     {
-        std::unique_lock<std::mutex> readableListLock(m_ReadableMutex);
-        if (!m_ReadableList.empty())
+        std::unique_lock<std::mutex> lock(m_Mutex);
+
+        if (m_BufferList.empty())
         {
-            std::unique_ptr<IPacketBuffer> 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<IPacketBuffer>& packetBuffer) override
+    void MarkRead(IPacketBufferPtr& packetBuffer) override
     {
-        std::unique_lock<std::mutex> availableListLock(m_AvailableMutex, std::defer_lock);
-        // increase read size
+        std::unique_lock<std::mutex> 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<std::unique_ptr<IPacketBuffer>> m_AvailableList;
-    std::vector<std::unique_ptr<IPacketBuffer>> 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<IPacketBufferPtr> 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;
 };