From a0d30ffef179276570f8b281f396c257298ceb83 Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Sun, 26 Jul 2015 20:41:48 -0700 Subject: [PATCH] Storage: Use StringMap instead of ConcurrentQueue for updates. Updates are merged or themselves updated as user code changes. This avoids the need for the dispatcher to do this and also avoids the need for disabling updates when the dispatcher isn't running, because there's no risk of memory usage piling up. --- src/Storage.cpp | 65 ++++++++++++++- src/Storage.h | 31 ++++--- test/unit/StorageTest.cpp | 167 ++++++++++++++++++++++++-------------- 3 files changed, 189 insertions(+), 74 deletions(-) diff --git a/src/Storage.cpp b/src/Storage.cpp index 96b4baa886..6a9549a2af 100644 --- a/src/Storage.cpp +++ b/src/Storage.cpp @@ -18,11 +18,74 @@ using namespace nt; ATOMIC_STATIC_INIT(Storage) Storage::Storage() { - m_updates_enabled = false; + m_updates_delete_all = false; } Storage::~Storage() {} +void Storage::GetUpdates(UpdateMap* updates, bool* delete_all) { + std::lock_guard lock(m_mutex); + m_updates.swap(*updates); + m_updates.clear(); + *delete_all = m_updates_delete_all; + m_updates_delete_all = false; +} + +// Must be called with mutex already held +void Storage::AddUpdate(std::shared_ptr entry, + Update::Kind kind) { + if (kind == Update::kDeleteAll) { + m_updates_delete_all = true; + m_updates.clear(); + return; + } + auto& update = m_updates[entry->name()]; + switch (kind) { + case Update::kAssign: + update.entry = entry; + update.value = entry->value(); + update.flags = entry->flags(); + update.kind = Update::kAssign; + break; + case Update::kValueUpdate: + if (update.kind == Update::kNone) { + update.entry = entry; + update.kind = Update::kValueUpdate; + } else if (update.kind == Update::kFlagsUpdate) { + // Merge value+flags updates as we can only have a single update + // entry. + update.kind = Update::kValueFlagsUpdate; + } else if (update.kind == Update::kDelete) { + // Change delete + update into assign + update.kind = Update::kAssign; + update.flags = entry->flags(); + } + update.value = entry->value(); + break; + case Update::kFlagsUpdate: + if (update.kind == Update::kNone) { + update.entry = entry; + update.kind = Update::kFlagsUpdate; + } else if (update.kind == Update::kValueUpdate) { + // Merge value+flags updates as we can only have a single update + // entry. + update.kind = Update::kValueFlagsUpdate; + } else if (update.kind == Update::kDelete) { + return; // can't do a raw flags update + } + update.flags = entry->flags(); + break; + case Update::kDelete: + update.kind = Update::kDelete; + update.entry = entry; + update.value = nullptr; // release reference if any + break; + default: + assert(false && "unexpected update kind"); + break; + } +} + std::shared_ptr Storage::FindEntry(StringRef name) const { std::lock_guard lock(m_mutex); auto i = m_entries.find(name); diff --git a/src/Storage.h b/src/Storage.h index 9df9753265..420cefe100 100644 --- a/src/Storage.h +++ b/src/Storage.h @@ -87,21 +87,29 @@ class Storage { ~Storage(); struct Update { - enum Kind { kAssign, kValueUpdate, kFlagsUpdate, kDelete, kDeleteAll }; - Update(std::shared_ptr entry_, Kind kind_) - : entry(entry_), kind(kind_) {} + enum Kind { + kNone, + kAssign, + kValueUpdate, + kFlagsUpdate, + kValueFlagsUpdate, + kDelete, + kDeleteAll + }; + Update() : flags(0), kind(kNone) {} + std::shared_ptr entry; + std::shared_ptr value; + unsigned int flags; Kind kind; }; - typedef ConcurrentQueue UpdateQueue; + typedef llvm::StringMap UpdateMap; // Finds, but does not create entry. Returns nullptr if not found. std::shared_ptr FindEntry(StringRef name) const; // Accessors required by Dispatcher. - void EnableUpdates() { m_updates_enabled = true; } - void DisableUpdates() { m_updates_enabled = false; } - UpdateQueue& updates() { return m_updates; } + void GetUpdates(UpdateMap* updates, bool* delete_all); std::mutex& mutex() { return m_mutex; } // User functions @@ -124,17 +132,14 @@ class Storage { Storage(const Storage&) = delete; Storage& operator=(const Storage&) = delete; - template - void AddUpdate(Args&&... args) { - if (m_updates_enabled) m_updates.emplace(std::forward(args)...); - } + void AddUpdate(std::shared_ptr entry, Update::Kind kind); typedef llvm::StringMap> EntriesMap; mutable std::mutex m_mutex; EntriesMap m_entries; - UpdateQueue m_updates; - std::atomic_bool m_updates_enabled; + UpdateMap m_updates; + bool m_updates_delete_all; ATOMIC_STATIC_DECL(Storage) }; diff --git a/test/unit/StorageTest.cpp b/test/unit/StorageTest.cpp index 8fbb997715..1cf4f6833c 100644 --- a/test/unit/StorageTest.cpp +++ b/test/unit/StorageTest.cpp @@ -16,24 +16,23 @@ namespace nt { class StorageTest : public ::testing::Test { public: - StorageTest() { - storage.EnableUpdates(); - } Storage::EntriesMap& entries() { return storage.m_entries; } - Storage::UpdateQueue& updates() { return storage.updates(); } + void GetUpdates() { storage.GetUpdates(&updates, &delete_all); } std::shared_ptr GetEntry(StringRef name) { auto& entry = storage.m_entries[name]; if (!entry) entry = std::make_shared(name); return entry; } Storage storage; + Storage::UpdateMap updates; + bool delete_all; }; class StorageTestPopulateOne : public StorageTest { public: StorageTestPopulateOne() { storage.SetEntryTypeValue("foo", Value::MakeBoolean(true)); - while (!updates().empty()) updates().pop(); + GetUpdates(); } }; @@ -44,7 +43,7 @@ class StorageTestPopulated : public StorageTest { storage.SetEntryTypeValue("foo2", Value::MakeDouble(0.0)); storage.SetEntryTypeValue("bar", Value::MakeDouble(1.0)); storage.SetEntryTypeValue("bar2", Value::MakeBoolean(false)); - while (!updates().empty()) updates().pop(); + GetUpdates(); } }; @@ -87,7 +86,7 @@ class StorageTestPersistent : public StorageTest { Value::MakeStringArray(std::vector{"hello", "world\n"})); storage.SetEntryTypeValue(StringRef("\0\3\5\n", 4), Value::MakeBoolean(true)); - while (!updates().empty()) updates().pop(); + GetUpdates(); } }; @@ -98,7 +97,9 @@ class MockLoadWarn { TEST_F(StorageTest, Construct) { ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + ASSERT_TRUE(updates.empty()); + ASSERT_FALSE(delete_all); } TEST_F(StorageTest, FindEntryNotExist) { @@ -134,9 +135,9 @@ TEST_F(StorageTest, GetEntryValueNotExist) { TEST_F(StorageTest, GetEntryValueExist) { auto value = Value::MakeBoolean(true); storage.SetEntryTypeValue("foo", value); - ASSERT_EQ(1u, updates().size()); - updates().pop(); - ASSERT_EQ(value, storage.GetEntryValue("foo")); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + EXPECT_EQ(updates["foo"].value, storage.GetEntryValue("foo")); } TEST_F(StorageTest, SetEntryTypeValueAssignNew) { @@ -145,8 +146,10 @@ TEST_F(StorageTest, SetEntryTypeValueAssignNew) { storage.SetEntryTypeValue("foo", value); auto entry = GetEntry("foo"); EXPECT_EQ(value, entry->value()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + EXPECT_FALSE(delete_all); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kAssign, update.kind); } @@ -157,8 +160,10 @@ TEST_F(StorageTestPopulateOne, SetEntryTypeValueAssignTypeChange) { storage.SetEntryTypeValue("foo", value); auto entry = GetEntry("foo"); EXPECT_EQ(value, entry->value()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + EXPECT_FALSE(delete_all); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kAssign, update.kind); } @@ -170,7 +175,9 @@ TEST_F(StorageTestPopulateOne, SetEntryTypeValueEqualValue) { storage.SetEntryTypeValue("foo", value); auto entry = GetEntry("foo"); EXPECT_EQ(value, entry->value()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTestPopulateOne, SetEntryTypeValueDifferentValue) { @@ -179,8 +186,10 @@ TEST_F(StorageTestPopulateOne, SetEntryTypeValueDifferentValue) { storage.SetEntryTypeValue("foo", value); auto entry = GetEntry("foo"); EXPECT_EQ(value, entry->value()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + EXPECT_FALSE(delete_all); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kValueUpdate, update.kind); } @@ -189,13 +198,17 @@ TEST_F(StorageTest, SetEntryTypeValueEmptyName) { auto value = Value::MakeBoolean(true); storage.SetEntryTypeValue("", value); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTest, SetEntryTypeValueEmptyValue) { storage.SetEntryTypeValue("foo", nullptr); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTest, SetEntryValueAssignNew) { @@ -204,8 +217,10 @@ TEST_F(StorageTest, SetEntryValueAssignNew) { EXPECT_TRUE(storage.SetEntryValue("foo", value)); auto entry = GetEntry("foo"); EXPECT_EQ(value, entry->value()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + EXPECT_FALSE(delete_all); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kAssign, update.kind); } @@ -216,7 +231,9 @@ TEST_F(StorageTestPopulateOne, SetEntryValueAssignTypeChange) { EXPECT_FALSE(storage.SetEntryValue("foo", value)); auto entry = GetEntry("foo"); EXPECT_NE(value, entry->value()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTestPopulateOne, SetEntryValueEqualValue) { @@ -226,7 +243,9 @@ TEST_F(StorageTestPopulateOne, SetEntryValueEqualValue) { EXPECT_TRUE(storage.SetEntryValue("foo", value)); auto entry = GetEntry("foo"); EXPECT_EQ(value, entry->value()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTestPopulateOne, SetEntryValueDifferentValue) { @@ -235,8 +254,10 @@ TEST_F(StorageTestPopulateOne, SetEntryValueDifferentValue) { EXPECT_TRUE(storage.SetEntryValue("foo", value)); auto entry = GetEntry("foo"); EXPECT_EQ(value, entry->value()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + EXPECT_FALSE(delete_all); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kValueUpdate, update.kind); } @@ -245,20 +266,26 @@ TEST_F(StorageTest, SetEntryValueEmptyName) { auto value = Value::MakeBoolean(true); EXPECT_TRUE(storage.SetEntryValue("", value)); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTest, SetEntryValueEmptyValue) { EXPECT_TRUE(storage.SetEntryValue("foo", nullptr)); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTest, SetEntryFlagsNew) { // flags setting doesn't create an entry storage.SetEntryFlags("foo", 0u); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTestPopulateOne, SetEntryFlagsEqualValue) { @@ -267,7 +294,9 @@ TEST_F(StorageTestPopulateOne, SetEntryFlagsEqualValue) { storage.SetEntryFlags("foo", 0u); auto entry = GetEntry("foo"); EXPECT_EQ(0u, entry->flags()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + EXPECT_FALSE(delete_all); } TEST_F(StorageTestPopulateOne, SetEntryFlagsDifferentValue) { @@ -275,8 +304,9 @@ TEST_F(StorageTestPopulateOne, SetEntryFlagsDifferentValue) { storage.SetEntryFlags("foo", 1u); auto entry = GetEntry("foo"); EXPECT_EQ(1u, entry->flags()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kFlagsUpdate, update.kind); } @@ -284,47 +314,52 @@ TEST_F(StorageTestPopulateOne, SetEntryFlagsDifferentValue) { TEST_F(StorageTest, SetEntryFlagsEmptyName) { storage.SetEntryFlags("", 0u); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); } TEST_F(StorageTest, GetEntryFlagsNotExist) { ASSERT_EQ(0u, storage.GetEntryFlags("foo")); ASSERT_TRUE(entries().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); } TEST_F(StorageTestPopulateOne, GetEntryFlagsExist) { storage.SetEntryFlags("foo", 1u); - while (!updates().empty()) updates().pop(); + GetUpdates(); ASSERT_EQ(1u, storage.GetEntryFlags("foo")); } TEST_F(StorageTest, DeleteEntryNotExist) { storage.DeleteEntry("foo"); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); } TEST_F(StorageTestPopulateOne, DeleteEntryExist) { auto entry = GetEntry("foo"); storage.DeleteEntry("foo"); ASSERT_TRUE(entries().empty()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kDelete, update.kind); } TEST_F(StorageTest, DeleteAllEntriesEmpty) { storage.DeleteAllEntries(); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + EXPECT_TRUE(updates.empty()); } TEST_F(StorageTestPopulated, DeleteAllEntries) { storage.DeleteAllEntries(); ASSERT_TRUE(entries().empty()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); - EXPECT_EQ(nullptr, update.entry); - EXPECT_EQ(Storage::Update::kDeleteAll, update.kind); + GetUpdates(); + EXPECT_TRUE(updates.empty()); + ASSERT_TRUE(delete_all); } TEST_F(StorageTestPopulated, GetEntryInfoAll) { @@ -443,7 +478,8 @@ TEST_F(StorageTest, LoadPersistentBadHeader) { EXPECT_CALL(warn, Warn(1, "header line mismatch, ignoring rest of file")); EXPECT_FALSE(storage.LoadPersistent(iss2, warn_func)); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + ASSERT_TRUE(updates.empty()); } TEST_F(StorageTest, LoadPersistentCommentHeader) { @@ -455,7 +491,8 @@ TEST_F(StorageTest, LoadPersistentCommentHeader) { "\n; comment\n# comment\n[NetworkTables Storage 3.0]\n"); EXPECT_TRUE(storage.LoadPersistent(iss, warn_func)); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + ASSERT_TRUE(updates.empty()); } TEST_F(StorageTest, LoadPersistentEmptyName) { @@ -467,7 +504,8 @@ TEST_F(StorageTest, LoadPersistentEmptyName) { "[NetworkTables Storage 3.0]\nboolean \"\"=true\n"); EXPECT_TRUE(storage.LoadPersistent(iss, warn_func)); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + ASSERT_TRUE(updates.empty()); } TEST_F(StorageTest, LoadPersistentAssign) { @@ -481,10 +519,13 @@ TEST_F(StorageTest, LoadPersistentAssign) { auto entry = GetEntry("foo"); EXPECT_EQ(*Value::MakeBoolean(true), *entry->value()); EXPECT_EQ(NT_PERSISTENT, entry->flags()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kAssign, update.kind); + EXPECT_EQ(entry->value(), update.value); + EXPECT_EQ(NT_PERSISTENT, update.flags); } TEST_F(StorageTestPopulateOne, LoadPersistentUpdateFlags) { @@ -498,10 +539,12 @@ TEST_F(StorageTestPopulateOne, LoadPersistentUpdateFlags) { auto entry = GetEntry("foo"); EXPECT_EQ(*Value::MakeBoolean(true), *entry->value()); EXPECT_EQ(NT_PERSISTENT, entry->flags()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + ASSERT_EQ(1u, updates.size()); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kFlagsUpdate, update.kind); + EXPECT_EQ(NT_PERSISTENT, update.flags); } TEST_F(StorageTestPopulateOne, LoadPersistentUpdateValue) { @@ -510,7 +553,7 @@ TEST_F(StorageTestPopulateOne, LoadPersistentUpdateValue) { [&](std::size_t line, const char* msg) { warn.Warn(line, msg); }; GetEntry("foo")->set_flags(NT_PERSISTENT); - while (!updates().empty()) updates().pop(); + GetUpdates(); std::istringstream iss( "[NetworkTables Storage 3.0]\nboolean \"foo\"=false\n"); @@ -518,10 +561,12 @@ TEST_F(StorageTestPopulateOne, LoadPersistentUpdateValue) { auto entry = GetEntry("foo"); EXPECT_EQ(*Value::MakeBoolean(false), *entry->value()); EXPECT_EQ(NT_PERSISTENT, entry->flags()); - ASSERT_EQ(1u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + EXPECT_EQ(1u, updates.size()); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); EXPECT_EQ(Storage::Update::kValueUpdate, update.kind); + EXPECT_EQ(entry->value(), update.value); } TEST_F(StorageTestPopulateOne, LoadPersistentUpdateValueFlags) { @@ -535,13 +580,13 @@ TEST_F(StorageTestPopulateOne, LoadPersistentUpdateValueFlags) { auto entry = GetEntry("foo"); EXPECT_EQ(*Value::MakeBoolean(false), *entry->value()); EXPECT_EQ(NT_PERSISTENT, entry->flags()); - ASSERT_EQ(2u, updates().size()); - auto update = updates().pop(); + GetUpdates(); + ASSERT_EQ(1u, updates.size()); + auto& update = updates["foo"]; EXPECT_EQ(entry, update.entry); - EXPECT_EQ(Storage::Update::kValueUpdate, update.kind); - update = updates().pop(); - EXPECT_EQ(entry, update.entry); - EXPECT_EQ(Storage::Update::kFlagsUpdate, update.kind); + EXPECT_EQ(Storage::Update::kValueFlagsUpdate, update.kind); + EXPECT_EQ(entry->value(), update.value); + EXPECT_EQ(NT_PERSISTENT, update.flags); } TEST_F(StorageTest, LoadPersistent) { @@ -575,7 +620,8 @@ TEST_F(StorageTest, LoadPersistent) { std::istringstream iss(in); EXPECT_TRUE(storage.LoadPersistent(iss, warn_func)); ASSERT_EQ(21u, entries().size()); - ASSERT_EQ(21u, updates().size()); + GetUpdates(); + EXPECT_EQ(21u, updates.size()); EXPECT_EQ(*Value::MakeBoolean(true), *storage.GetEntryValue("boolean/true")); EXPECT_EQ(*Value::MakeBoolean(false), @@ -629,7 +675,8 @@ TEST_F(StorageTest, LoadPersistentWarn) { EXPECT_TRUE(storage.LoadPersistent(iss, warn_func)); ASSERT_TRUE(entries().empty()); - ASSERT_TRUE(updates().empty()); + GetUpdates(); + ASSERT_TRUE(updates.empty()); } } // namespace nt