From 741b55c7c0c47187b37dc309fcb76242557a5a3e Mon Sep 17 00:00:00 2001 From: Matt Koscumb Date: Fri, 9 Nov 2018 16:25:02 -0800 Subject: [PATCH 1/3] Rather than return a raw pointer std::vector, thus inducing memory managment on the caller and breaking the raw-pointers-are-non-owning rule, IOfflineStorgage::GetRecords should just return the vector. std::vector is cheap to move, and thus can easily be returned from a function on any modern compiler without inducing a massive copy cost. --- lib/include/aria/IOfflineStorage.hpp | 4 ++-- lib/offline/MemoryStorage.cpp | 8 ++++---- lib/offline/MemoryStorage.hpp | 2 +- lib/offline/OfflineStorageHandler.cpp | 17 +++++++---------- lib/offline/OfflineStorageHandler.hpp | 2 +- lib/offline/OfflineStorage_SQLite.cpp | 8 ++++---- lib/offline/OfflineStorage_SQLite.hpp | 2 +- tests/common/MockIOfflineStorage.hpp | 2 +- tests/unittests/MemoryStorageTests.cpp | 26 +++++++++++--------------- 9 files changed, 32 insertions(+), 39 deletions(-) diff --git a/lib/include/aria/IOfflineStorage.hpp b/lib/include/aria/IOfflineStorage.hpp index ec0e71d9..50e0af87 100644 --- a/lib/include/aria/IOfflineStorage.hpp +++ b/lib/include/aria/IOfflineStorage.hpp @@ -101,7 +101,7 @@ namespace ARIASDK_NS_BEGIN { class IOfflineStorage { public: - virtual ~IOfflineStorage() {} + virtual ~IOfflineStorage() noexcept = default; /// /// Initialize the offline storage @@ -271,7 +271,7 @@ namespace ARIASDK_NS_BEGIN { /// lowest priority selected /// max count to be selected /// Value of the requested setting or an empty string - virtual std::vector* GetRecords(bool shutdown, EventLatency minLatency = EventLatency_Unspecified, unsigned maxCount = 0) = 0; + virtual std::vector GetRecords(bool shutdown, EventLatency minLatency = EventLatency_Unspecified, unsigned maxCount = 0) = 0; virtual bool ResizeDb() = 0; diff --git a/lib/offline/MemoryStorage.cpp b/lib/offline/MemoryStorage.cpp index 5e5b91ee..81188740 100644 --- a/lib/offline/MemoryStorage.cpp +++ b/lib/offline/MemoryStorage.cpp @@ -369,15 +369,15 @@ namespace ARIASDK_NS_BEGIN { return numRecords; } - std::vector* MemoryStorage::GetRecords(bool shutdown, EventLatency minLatency, unsigned maxCount) + std::vector MemoryStorage::GetRecords(bool shutdown, EventLatency minLatency, unsigned maxCount) { UNREFERENCED_PARAMETER(shutdown); UNREFERENCED_PARAMETER(minLatency); UNREFERENCED_PARAMETER(maxCount); - std::vector* records = new std::vector(); - auto consumer = [records](StorageRecord&& record) -> bool { - records->push_back(std::move(record)); + std::vector records; + auto consumer = [&records](StorageRecord&& record) -> bool { + records.push_back(std::move(record)); return true; // want more }; GetAndReserveRecords(consumer, 0, minLatency, maxCount); diff --git a/lib/offline/MemoryStorage.hpp b/lib/offline/MemoryStorage.hpp index 88c6d693..c55c91e2 100644 --- a/lib/offline/MemoryStorage.hpp +++ b/lib/offline/MemoryStorage.hpp @@ -57,7 +57,7 @@ namespace ARIASDK_NS_BEGIN { virtual size_t GetReservedCount(); - virtual std::vector* GetRecords(bool shutdown = false, EventLatency minLatency = EventLatency_Unspecified, unsigned maxCount = 0) override; + virtual std::vector GetRecords(bool shutdown = false, EventLatency minLatency = EventLatency_Unspecified, unsigned maxCount = 0) override; virtual bool ResizeDb() override; diff --git a/lib/offline/OfflineStorageHandler.cpp b/lib/offline/OfflineStorageHandler.cpp index bc74be3c..6713d3cc 100644 --- a/lib/offline/OfflineStorageHandler.cpp +++ b/lib/offline/OfflineStorageHandler.cpp @@ -147,7 +147,7 @@ namespace ARIASDK_NS_BEGIN { if ((m_offlineStorageMemory) && (dbSizeBeforeFlush > 0) && (m_offlineStorageDisk)) { - std::vector* records = m_offlineStorageMemory->GetRecords(false, EventLatency_Unspecified); + auto records = m_offlineStorageMemory->GetRecords(false, EventLatency_Unspecified); std::vector ids; size_t totalSaved = 0; @@ -156,12 +156,12 @@ namespace ARIASDK_NS_BEGIN { // if (sqlite) // sqlite->Execute("BEGIN"); - while (records->size()) + while (records.size()) { - ids.push_back(records->back().id); - if (m_offlineStorageDisk->StoreRecord(std::move(records->back()))) + ids.push_back(records.back().id); + if (m_offlineStorageDisk->StoreRecord(std::move(records.back()))) totalSaved++; - records->pop_back(); + records.pop_back(); } // TODO: [MG] - consider running the batch in transaction @@ -173,8 +173,6 @@ namespace ARIASDK_NS_BEGIN { bool fromMemory = true; m_offlineStorageMemory->DeleteRecords(ids, dummy, fromMemory); - delete records; - // Notify event listener about the records cached OnStorageRecordsSaved(totalSaved); @@ -305,7 +303,7 @@ namespace ARIASDK_NS_BEGIN { return returnValue; } - std::vector* OfflineStorageHandler::GetRecords(bool shutdown, EventLatency minLatency, unsigned maxCount) + std::vector OfflineStorageHandler::GetRecords(bool shutdown, EventLatency minLatency, unsigned maxCount) { // This method should not be called directly because it's a no-op assert(false); @@ -313,8 +311,7 @@ namespace ARIASDK_NS_BEGIN { UNREFERENCED_PARAMETER(shutdown); UNREFERENCED_PARAMETER(minLatency); UNREFERENCED_PARAMETER(maxCount); - std::vector* records = new std::vector(); - return records; + return std::vector{}; } void OfflineStorageHandler::DeleteRecords(std::vector const& ids, HttpHeaders headers, bool& fromMemory) diff --git a/lib/offline/OfflineStorageHandler.hpp b/lib/offline/OfflineStorageHandler.hpp index 0468e9e5..b5e072a7 100644 --- a/lib/offline/OfflineStorageHandler.hpp +++ b/lib/offline/OfflineStorageHandler.hpp @@ -35,7 +35,7 @@ namespace ARIASDK_NS_BEGIN { virtual size_t GetSize() override; virtual size_t GetRecordCount(EventLatency latency = EventLatency_Unspecified) const override; - virtual std::vector* GetRecords(bool shutdown, EventLatency minLatency = EventLatency_Unspecified, unsigned maxCount = 0) override; + virtual std::vector GetRecords(bool shutdown, EventLatency minLatency = EventLatency_Unspecified, unsigned maxCount = 0) override; virtual bool ResizeDb() override; virtual void OnStorageOpened(std::string const& type) override; diff --git a/lib/offline/OfflineStorage_SQLite.cpp b/lib/offline/OfflineStorage_SQLite.cpp index 9b089fa6..29adc299 100644 --- a/lib/offline/OfflineStorage_SQLite.cpp +++ b/lib/offline/OfflineStorage_SQLite.cpp @@ -311,9 +311,9 @@ namespace ARIASDK_NS_BEGIN { return m_lastReadCount; } - std::vector* OfflineStorage_SQLite::GetRecords(bool shutdown, EventLatency minLatency, unsigned maxCount) + std::vector OfflineStorage_SQLite::GetRecords(bool shutdown, EventLatency minLatency, unsigned maxCount) { - std::vector* records = new std::vector(); + std::vector records; StorageRecord record; if (shutdown) @@ -325,7 +325,7 @@ namespace ARIASDK_NS_BEGIN { while (selectStmt.getRow(record.id, record.tenantToken, latency, record.timestamp, record.retryCount, record.reservedUntil, record.blob)) { record.latency = static_cast(latency); - records->push_back(record); + records.push_back(record); } selectStmt.reset(); } @@ -339,7 +339,7 @@ namespace ARIASDK_NS_BEGIN { while (selectStmt.getRow(record.id, record.tenantToken, latency, record.timestamp, record.retryCount, record.reservedUntil, record.blob)) { record.latency = static_cast(latency); - records->push_back(record); + records.push_back(record); } selectStmt.reset(); } diff --git a/lib/offline/OfflineStorage_SQLite.hpp b/lib/offline/OfflineStorage_SQLite.hpp index 21e396d5..816e73cb 100644 --- a/lib/offline/OfflineStorage_SQLite.hpp +++ b/lib/offline/OfflineStorage_SQLite.hpp @@ -41,7 +41,7 @@ namespace ARIASDK_NS_BEGIN { virtual std::string GetSetting(std::string const& name) override; virtual size_t GetSize() override; virtual size_t GetRecordCount(EventLatency latency) const override; - virtual std::vector* GetRecords(bool shutdown, EventLatency minLatency = EventLatency_Normal, unsigned maxCount = 0) override; + virtual std::vector GetRecords(bool shutdown, EventLatency minLatency = EventLatency_Normal, unsigned maxCount = 0) override; virtual bool ResizeDb() override; protected: diff --git a/tests/common/MockIOfflineStorage.hpp b/tests/common/MockIOfflineStorage.hpp index ff7b3aa4..fcb7cdaf 100644 --- a/tests/common/MockIOfflineStorage.hpp +++ b/tests/common/MockIOfflineStorage.hpp @@ -25,7 +25,7 @@ class MockIOfflineStorage : public ARIASDK_NS::IOfflineStorage { MOCK_METHOD1(GetSetting, std::string(std::string const &)); MOCK_METHOD0(GetSize, size_t()); MOCK_CONST_METHOD1(GetRecordCount, size_t(ARIASDK_NS::EventLatency)); - MOCK_METHOD3(GetRecords, std::vector*(bool, ARIASDK_NS::EventLatency, unsigned)); + MOCK_METHOD3(GetRecords, std::vector(bool, ARIASDK_NS::EventLatency, unsigned)); MOCK_METHOD0(ResizeDb, bool()); }; diff --git a/tests/unittests/MemoryStorageTests.cpp b/tests/unittests/MemoryStorageTests.cpp index 3fe3f993..07c3371e 100644 --- a/tests/unittests/MemoryStorageTests.cpp +++ b/tests/unittests/MemoryStorageTests.cpp @@ -127,13 +127,12 @@ TEST(MemoryStorageTests, StoreAndGetRecords) EXPECT_THAT(storage.GetSize(), total_db_size); // Verify the contents size_t total_records = num_iterations * latencies.size(); - std::unique_ptr< std::vector > records; for (auto latency : { EventLatency_Max , EventLatency_RealTime , EventLatency_CostDeferred , EventLatency_Normal }) { - records.reset(storage.GetRecords(false, latency, 0)); - EXPECT_THAT(records->size(), num_iterations); - EXPECT_THAT(records->size(), storage.LastReadRecordCount()); - total_records -= records->size(); + auto records = storage.GetRecords(false, latency, 0); + EXPECT_THAT(records.size(), num_iterations); + EXPECT_THAT(records.size(), storage.LastReadRecordCount()); + total_records -= records.size(); } EXPECT_THAT(storage.GetSize(), 0); EXPECT_THAT(total_records, 0); @@ -146,10 +145,9 @@ TEST(MemoryStorageTests, StoreAndGetRecords) EXPECT_THAT(storage.GetSize(), total_db_size); // Verify the contents size_t total_records = num_iterations * latencies.size(); - std::unique_ptr< std::vector > records; - records.reset(storage.GetRecords(false, EventLatency_Normal, 0)); - EXPECT_THAT(records->size(), total_records); - EXPECT_THAT(records->size(), storage.LastReadRecordCount()); + auto records = storage.GetRecords(false, EventLatency_Normal, 0); + EXPECT_THAT(records.size(), total_records); + EXPECT_THAT(records.size(), storage.LastReadRecordCount()); EXPECT_THAT(storage.GetSize(), 0); } @@ -168,14 +166,13 @@ TEST(MemoryStorageTests, GetRecordsDeletesRecords) EXPECT_THAT(storage.GetSize(), total_db_size); // Retrieve those into records - std::unique_ptr< std::vector > records; - records.reset(storage.GetRecords()); + auto records = storage.GetRecords(); // Storage size is "zero" because all records are fetched EXPECT_THAT(storage.GetSize(), 0); EXPECT_THAT(storage.GetRecordCount(), 0); EXPECT_THAT(storage.GetReservedCount(), 0); - EXPECT_THAT(records->size(), num_iterations * 4); // 4 latencies + EXPECT_THAT(records.size(), num_iterations * 4); // 4 latencies } TEST(MemoryStorageTests, ReleaseRecords) @@ -262,12 +259,11 @@ TEST(MemoryStorageTests, MultiThreadPerfTest) addEvents(storage); // Retrieve them - std::unique_ptr< std::vector > records; - records.reset(storage.GetRecords()); + auto records = storage.GetRecords(); // Acquire IDs std::vector ids; - for (auto &record : *records) + for (const auto &record : records) ids.push_back(record.id); // Release records From 71ff57b10ed940b9fdc9378f96de71d3d0bb7f2e Mon Sep 17 00:00:00 2001 From: Matt Koscumb Date: Mon, 12 Nov 2018 09:57:40 -0800 Subject: [PATCH 2/3] Fix one more case of unnecessary unique_ptr> --- tests/unittests/MemoryStorageTests.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/unittests/MemoryStorageTests.cpp b/tests/unittests/MemoryStorageTests.cpp index 07c3371e..5654de39 100644 --- a/tests/unittests/MemoryStorageTests.cpp +++ b/tests/unittests/MemoryStorageTests.cpp @@ -189,11 +189,10 @@ TEST(MemoryStorageTests, ReleaseRecords) auto total_records = storage.GetRecordCount(); // Retrieve those into records - std::unique_ptr< std::vector > records; - records.reset(new std::vector); + std::vector records; auto consumer = [&records](StorageRecord&& record) -> bool { - records->push_back(std::move(record)); + records.push_back(std::move(record)); return true; // want more }; @@ -203,10 +202,10 @@ TEST(MemoryStorageTests, ReleaseRecords) EXPECT_THAT(storage.GetSize(), 0); EXPECT_THAT(storage.GetRecordCount(), 0); EXPECT_THAT(storage.GetReservedCount(), total_records); - EXPECT_THAT(records->size(), total_records); + EXPECT_THAT(records.size(), total_records); // Track IDs of all reserved records - for (auto &record : *records) + for (auto &record : records) { ids.push_back(record.id); } From 0bd999eeeadaddbde92bd82c8ea75f4d5344979e Mon Sep 17 00:00:00 2001 From: Matt Koscumb Date: Mon, 12 Nov 2018 10:01:08 -0800 Subject: [PATCH 3/3] Fix some whitespace tabs -> spaces. --- lib/include/aria/IOfflineStorage.hpp | 2 +- lib/offline/OfflineStorageHandler.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/include/aria/IOfflineStorage.hpp b/lib/include/aria/IOfflineStorage.hpp index 50e0af87..afedd344 100644 --- a/lib/include/aria/IOfflineStorage.hpp +++ b/lib/include/aria/IOfflineStorage.hpp @@ -101,7 +101,7 @@ namespace ARIASDK_NS_BEGIN { class IOfflineStorage { public: - virtual ~IOfflineStorage() noexcept = default; + virtual ~IOfflineStorage() noexcept = default; /// /// Initialize the offline storage diff --git a/lib/offline/OfflineStorageHandler.cpp b/lib/offline/OfflineStorageHandler.cpp index 6713d3cc..2b33b43b 100644 --- a/lib/offline/OfflineStorageHandler.cpp +++ b/lib/offline/OfflineStorageHandler.cpp @@ -311,7 +311,7 @@ namespace ARIASDK_NS_BEGIN { UNREFERENCED_PARAMETER(shutdown); UNREFERENCED_PARAMETER(minLatency); UNREFERENCED_PARAMETER(maxCount); - return std::vector{}; + return std::vector{}; } void OfflineStorageHandler::DeleteRecords(std::vector const& ids, HttpHeaders headers, bool& fromMemory)