From: Matteo Martincigh Date: Tue, 15 Oct 2019 08:35:29 +0000 (+0100) Subject: IVGCVSW-3939 Code refactoring and minor fixes X-Git-Tag: submit/tizen/20200316.035456~155 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=8d9590e5510b8ebdc4e0b2b31ce4b653b46fc02a;p=platform%2Fupstream%2Farmnn.git IVGCVSW-3939 Code refactoring and minor fixes * Fixed value masking in SendPeriodicCounterCapturePacket and updated the pertinent unit tests * Code refactoring and cleanup * Added extra comments to the ProfilingService stop/reset procedure Signed-off-by: Matteo Martincigh Change-Id: Ibaf2fede76e06d5b8ce7258a4820a60e5993559f --- diff --git a/src/profiling/PeriodicCounterCapture.cpp b/src/profiling/PeriodicCounterCapture.cpp index 5ba1318..f888bc0 100644 --- a/src/profiling/PeriodicCounterCapture.cpp +++ b/src/profiling/PeriodicCounterCapture.cpp @@ -30,9 +30,7 @@ void PeriodicCounterCapture::Start() m_KeepRunning.store(true); // Start the new capture thread. - m_PeriodCaptureThread = std::thread(&PeriodicCounterCapture::Capture, - this, - std::ref(m_ReadCounterValues)); + m_PeriodCaptureThread = std::thread(&PeriodicCounterCapture::Capture, this, std::ref(m_ReadCounterValues)); } void PeriodicCounterCapture::Stop() @@ -47,6 +45,7 @@ void PeriodicCounterCapture::Stop() m_PeriodCaptureThread.join(); } + // Mark the capture thread as not running m_IsRunning = false; } @@ -114,7 +113,6 @@ void PeriodicCounterCapture::Capture(const IReadCounterValues& readCounterValues } while (m_KeepRunning.load()); - } } // namespace profiling diff --git a/src/profiling/ProfilingService.cpp b/src/profiling/ProfilingService.cpp index b87773f..1cc9262 100644 --- a/src/profiling/ProfilingService.cpp +++ b/src/profiling/ProfilingService.cpp @@ -313,8 +313,9 @@ void ProfilingService::InitializeCounterValue(uint16_t counterUid) void ProfilingService::Reset() { - // Reset the profiling service + // Stop the profiling service... Stop(); + // ...then delete all the counter data and configuration... m_CounterIndex.clear(); m_CounterValues.clear(); @@ -333,13 +334,14 @@ void ProfilingService::Stop() m_SendCounterPacket.Stop(false); m_PeriodicCounterCapture.Stop(); - // ...then destroy the profiling connection... + // ...then close and destroy the profiling connection... if (m_ProfilingConnection != nullptr && m_ProfilingConnection->IsOpen()) { m_ProfilingConnection->Close(); } m_ProfilingConnection.reset(); + // ...then move to the "NotConnected" state m_StateMachine.TransitionToState(ProfilingState::NotConnected); } diff --git a/src/profiling/SendCounterPacket.cpp b/src/profiling/SendCounterPacket.cpp index 41adf37..0ac6ecf 100644 --- a/src/profiling/SendCounterPacket.cpp +++ b/src/profiling/SendCounterPacket.cpp @@ -811,12 +811,15 @@ void SendCounterPacket::SendCounterDirectoryPacket(const ICounterDirectory& coun void SendCounterPacket::SendPeriodicCounterCapturePacket(uint64_t timestamp, const IndexValuePairsVector& values) { + uint32_t uint16_t_size = sizeof(uint16_t); + uint32_t uint32_t_size = sizeof(uint32_t); + uint32_t uint64_t_size = sizeof(uint64_t); + uint32_t packetFamily = 1; uint32_t packetClass = 0; uint32_t packetType = 0; - uint32_t headerSize = numeric_cast(2 * sizeof(uint32_t)); - uint32_t bodySize = numeric_cast((1 * sizeof(uint64_t)) + - (values.size() * (sizeof(uint16_t) + sizeof(uint32_t)))); + uint32_t headerSize = 2 * uint32_t_size; + uint32_t bodySize = uint64_t_size + numeric_cast(values.size()) * (uint16_t_size + uint32_t_size); uint32_t totalSize = headerSize + bodySize; uint32_t offset = 0; uint32_t reserved = 0; @@ -833,22 +836,24 @@ void SendCounterPacket::SendPeriodicCounterCapturePacket(uint64_t timestamp, con // Create header. WriteUint32(writeBuffer, offset, - ((packetFamily & 0x3F) << 26) | ((packetClass & 0x3FF) << 19) | ((packetType & 0x3FFF) << 16)); - offset += numeric_cast(sizeof(uint32_t)); + ((packetFamily & 0x0000003F) << 26) | + ((packetClass & 0x0000007F) << 19) | + ((packetType & 0x00000007) << 16)); + offset += uint32_t_size; WriteUint32(writeBuffer, offset, bodySize); // Copy captured Timestamp. - offset += numeric_cast(sizeof(uint32_t)); + offset += uint32_t_size; WriteUint64(writeBuffer, offset, timestamp); // Copy selectedCounterIds. - offset += numeric_cast(sizeof(uint64_t)); + offset += uint64_t_size; for (const auto& pair: values) { WriteUint16(writeBuffer, offset, pair.first); - offset += numeric_cast(sizeof(uint16_t)); + offset += uint16_t_size; WriteUint32(writeBuffer, offset, pair.second); - offset += numeric_cast(sizeof(uint32_t)); + offset += uint32_t_size; } m_BufferManager.Commit(writeBuffer, totalSize); @@ -857,11 +862,13 @@ void SendCounterPacket::SendPeriodicCounterCapturePacket(uint64_t timestamp, con void SendCounterPacket::SendPeriodicCounterSelectionPacket(uint32_t capturePeriod, const std::vector& selectedCounterIds) { + uint32_t uint16_t_size = sizeof(uint16_t); + uint32_t uint32_t_size = sizeof(uint32_t); + uint32_t packetFamily = 0; uint32_t packetId = 4; - uint32_t headerSize = numeric_cast(2 * sizeof(uint32_t)); - uint32_t bodySize = numeric_cast((1 * sizeof(uint32_t)) + - (selectedCounterIds.size() * sizeof(uint16_t))); + uint32_t headerSize = 2 * uint32_t_size; + uint32_t bodySize = uint32_t_size + numeric_cast(selectedCounterIds.size()) * uint16_t_size; uint32_t totalSize = headerSize + bodySize; uint32_t offset = 0; uint32_t reserved = 0; @@ -877,19 +884,19 @@ void SendCounterPacket::SendPeriodicCounterSelectionPacket(uint32_t capturePerio // Create header. WriteUint32(writeBuffer, offset, ((packetFamily & 0x3F) << 26) | ((packetId & 0x3FF) << 16)); - offset += numeric_cast(sizeof(uint32_t)); + offset += uint32_t_size; WriteUint32(writeBuffer, offset, bodySize); // Copy capturePeriod. - offset += numeric_cast(sizeof(uint32_t)); + offset += uint32_t_size; WriteUint32(writeBuffer, offset, capturePeriod); // Copy selectedCounterIds. - offset += numeric_cast(sizeof(uint32_t)); + offset += uint32_t_size; for(const uint16_t& id: selectedCounterIds) { WriteUint16(writeBuffer, offset, id); - offset += numeric_cast(sizeof(uint16_t)); + offset += uint16_t_size; } m_BufferManager.Commit(writeBuffer, totalSize); diff --git a/src/profiling/SocketProfilingConnection.cpp b/src/profiling/SocketProfilingConnection.cpp index 6f7101b..2c4ff76 100644 --- a/src/profiling/SocketProfilingConnection.cpp +++ b/src/profiling/SocketProfilingConnection.cpp @@ -78,44 +78,42 @@ bool SocketProfilingConnection::WritePacket(const unsigned char* buffer, uint32_ Packet SocketProfilingConnection::ReadPacket(uint32_t timeout) { - // Is there currently at least a headers worth of data waiting to be read? - int bytes_available; + // Is there currently at least a header worth of data waiting to be read? + int bytes_available = 0; ioctl(m_Socket[0].fd, FIONREAD, &bytes_available); if (bytes_available >= 8) { // Yes there is. Read it: return ReceivePacket(); } - else - { - // Poll for data on the socket or until timeout occurs - int pollResult = poll(m_Socket, 1, static_cast(timeout)); - switch (pollResult) - { - case -1: // Error - throw armnn::RuntimeException(std::string("Read failure from socket: ") + strerror(errno)); + // Poll for data on the socket or until timeout occurs + int pollResult = poll(m_Socket, 1, static_cast(timeout)); - case 0: // Timeout - throw TimeoutException("Timeout while reading from socket"); + switch (pollResult) + { + case -1: // Error + throw armnn::RuntimeException(std::string("Read failure from socket: ") + strerror(errno)); - default: // Normal poll return but it could still contain an error signal + case 0: // Timeout + throw TimeoutException("Timeout while reading from socket"); - // Check if the socket reported an error - if (m_Socket[0].revents & (POLLNVAL | POLLERR | POLLHUP)) - { - throw armnn::RuntimeException(std::string("Socket reported an error: ") + strerror(errno)); - } + default: // Normal poll return but it could still contain an error signal - // Check if there is data to read - if (!(m_Socket[0].revents & (POLLIN))) - { - // This is a very odd case. The file descriptor was woken up but no data was written. - throw armnn::RuntimeException("Poll resulted in : no data to read"); - } + // Check if the socket reported an error + if (m_Socket[0].revents & (POLLNVAL | POLLERR | POLLHUP)) + { + throw armnn::RuntimeException(std::string("Socket reported an error: ") + strerror(errno)); + } - return ReceivePacket(); + // Check if there is data to read + if (!(m_Socket[0].revents & (POLLIN))) + { + // This is a very odd case. The file descriptor was woken up but no data was written. + throw armnn::RuntimeException("Poll resulted in : no data to read"); } + + return ReceivePacket(); } } @@ -127,6 +125,7 @@ Packet SocketProfilingConnection::ReceivePacket() // What do we do here if there's not a valid 8 byte header to read? throw armnn::RuntimeException("The received packet did not contains a valid MIPE header"); } + // stream_metadata_identifier is the first 4 bytes uint32_t metadataIdentifier = 0; std::memcpy(&metadataIdentifier, header, sizeof(metadataIdentifier)); @@ -135,10 +134,10 @@ Packet SocketProfilingConnection::ReceivePacket() uint32_t dataLength = 0; std::memcpy(&dataLength, header + 4u, sizeof(dataLength)); - std::unique_ptr packetData; + std::unique_ptr packetData; if (dataLength > 0) { - packetData = std::make_unique(dataLength); + packetData = std::make_unique(dataLength); ssize_t receivedLength = recv(m_Socket[0].fd, packetData.get(), dataLength, 0); if (receivedLength < 0) { diff --git a/src/profiling/test/ProfilingTests.cpp b/src/profiling/test/ProfilingTests.cpp index b32a55c..50af75e 100644 --- a/src/profiling/test/ProfilingTests.cpp +++ b/src/profiling/test/ProfilingTests.cpp @@ -2119,10 +2119,10 @@ BOOST_AUTO_TEST_CASE(CheckPeriodicCounterCaptureThread) uint32_t headerWord0 = ReadUint32(buffer, 0); uint32_t headerWord1 = ReadUint32(buffer, 4); - BOOST_TEST(((headerWord0 >> 26) & 0x3F) == 1); // packet family - BOOST_TEST(((headerWord0 >> 19) & 0x3F) == 0); // packet class - BOOST_TEST(((headerWord0 >> 16) & 0x3) == 0); // packet type - BOOST_TEST(headerWord1 == 20); // data length + BOOST_TEST(((headerWord0 >> 26) & 0x0000003F) == 1); // packet family + BOOST_TEST(((headerWord0 >> 19) & 0x0000007F) == 0); // packet class + BOOST_TEST(((headerWord0 >> 16) & 0x00000007) == 0); // packet type + BOOST_TEST(headerWord1 == 20); uint32_t offset = 16; uint16_t readIndex = ReadUint16(buffer, offset); diff --git a/src/profiling/test/SendCounterPacketTests.cpp b/src/profiling/test/SendCounterPacketTests.cpp index 00dad38..f0ba347 100644 --- a/src/profiling/test/SendCounterPacketTests.cpp +++ b/src/profiling/test/SendCounterPacketTests.cpp @@ -217,11 +217,11 @@ BOOST_AUTO_TEST_CASE(SendPeriodicCounterCapturePacketTest) uint32_t headerWord1 = ReadUint32(readBuffer2, 4); uint64_t readTimestamp = ReadUint64(readBuffer2, 8); - BOOST_TEST(((headerWord0 >> 26) & 0x3F) == 1); // packet family - BOOST_TEST(((headerWord0 >> 19) & 0x3F) == 0); // packet class - BOOST_TEST(((headerWord0 >> 16) & 0x3) == 0); // packet type - BOOST_TEST(headerWord1 == 8); // data length - BOOST_TEST(time == readTimestamp); // capture period + BOOST_TEST(((headerWord0 >> 26) & 0x0000003F) == 1); // packet family + BOOST_TEST(((headerWord0 >> 19) & 0x0000007F) == 0); // packet class + BOOST_TEST(((headerWord0 >> 16) & 0x00000007) == 0); // packet type + BOOST_TEST(headerWord1 == 8); // data length + BOOST_TEST(time == readTimestamp); // capture period // Full packet message MockBufferManager mockBuffer3(512); @@ -240,11 +240,11 @@ BOOST_AUTO_TEST_CASE(SendPeriodicCounterCapturePacketTest) headerWord1 = ReadUint32(readBuffer3, 4); uint64_t readTimestamp2 = ReadUint64(readBuffer3, 8); - BOOST_TEST(((headerWord0 >> 26) & 0x3F) == 1); // packet family - BOOST_TEST(((headerWord0 >> 19) & 0x3F) == 0); // packet class - BOOST_TEST(((headerWord0 >> 16) & 0x3) == 0); // packet type - BOOST_TEST(headerWord1 == 38); // data length - BOOST_TEST(time == readTimestamp2); // capture period + BOOST_TEST(((headerWord0 >> 26) & 0x0000003F) == 1); // packet family + BOOST_TEST(((headerWord0 >> 19) & 0x0000007F) == 0); // packet class + BOOST_TEST(((headerWord0 >> 16) & 0x00000007) == 0); // packet type + BOOST_TEST(headerWord1 == 38); // data length + BOOST_TEST(time == readTimestamp2); // capture period uint16_t counterIndex = 0; uint32_t counterValue = 100;