From 1a4926da2dcaf8922e8ce6e589ff08a3fe0c73cb Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Wed, 9 Feb 2022 14:39:55 +0300 Subject: [PATCH] settings: const-correctness and simplify prefix search Cursor is in a weird spot, by being both the pointer to the data and the read-range that is used of specify the range in which we operate Next TODO would be to move even more things to 'const', but that would at least require to rework the cursor object and also remove it from the members list. --- code/espurna/migrate.cpp | 12 +--- code/espurna/settings.cpp | 11 ++++ code/espurna/settings.h | 5 +- code/espurna/settings_embedis.h | 99 ++++++++++++++++++++------------- 4 files changed, 79 insertions(+), 48 deletions(-) diff --git a/code/espurna/migrate.cpp b/code/espurna/migrate.cpp index e09ace89..0f7ca58d 100644 --- a/code/espurna/migrate.cpp +++ b/code/espurna/migrate.cpp @@ -36,15 +36,9 @@ namespace { void deletePrefixes(::settings::query::StringViewIterator prefixes) { std::vector to_purge; - ::settings::internal::foreach([&](::settings::kvs_type::KeyValueResult&& kv) { - const auto key = kv.key.read(); - for (auto it = prefixes.begin(); it != prefixes.end(); ++it) { - if (::settings::query::samePrefix(::settings::StringView{key}, (*it))) { - to_purge.push_back(std::move(key)); - return; - } - } - }); + ::settings::internal::foreach_prefix([&](::settings::StringView, String key, const ::settings::kvs_type::ReadResult&) { + to_purge.push_back(std::move(key)); + }, prefixes); for (const auto& key : to_purge) { delSetting(key); diff --git a/code/espurna/settings.cpp b/code/espurna/settings.cpp index ebfb9b50..66cae4a9 100644 --- a/code/espurna/settings.cpp +++ b/code/espurna/settings.cpp @@ -159,6 +159,17 @@ void foreach(KeyValueResultCallback&& callback) { kv_store.foreach(callback); } +void foreach_prefix(PrefixResultCallback&& callback, query::StringViewIterator prefixes) { + kv_store.foreach([&](kvs_type::KeyValueResult&& kv) { + auto key = kv.key.read(); + for (auto it = prefixes.begin(); it != prefixes.end(); ++it) { + if (::settings::query::samePrefix(::settings::StringView{key}, (*it))) { + callback((*it), std::move(key), kv.value); + } + } + }); +} + // -------------------------------------------------------------------------- template <> diff --git a/code/espurna/settings.h b/code/espurna/settings.h index 845f804a..df79a9df 100644 --- a/code/espurna/settings.h +++ b/code/espurna/settings.h @@ -72,7 +72,10 @@ size_t available(); size_t size(); using KeyValueResultCallback = std::function; -void foreach(KeyValueResultCallback&& callback); +void foreach(KeyValueResultCallback&&); + +using PrefixResultCallback = std::function; +void foreach_prefix(PrefixResultCallback&&, settings::query::StringViewIterator); // -------------------------------------------------------------------------- diff --git a/code/espurna/settings_embedis.h b/code/espurna/settings_embedis.h index b4e136ad..f44a82a2 100644 --- a/code/espurna/settings_embedis.h +++ b/code/espurna/settings_embedis.h @@ -119,6 +119,9 @@ class KeyValueStore { Cursor() = delete; + Cursor(const Cursor&) = default; + Cursor(Cursor&&) = default; + void reset(uint16_t begin_, uint16_t end_) { position = 0; begin = begin_; @@ -157,11 +160,11 @@ class KeyValueStore { return _storage.read(begin + position_); } - bool operator ==(const Cursor& other) const { + bool operator==(const Cursor& other) const { return (begin == other.begin) && (end == other.end); } - bool operator !=(const Cursor& other) const { + bool operator!=(const Cursor& other) const { return !(*this == other); } @@ -203,32 +206,56 @@ class KeyValueStore { struct ReadResult { friend class KeyValueStore; - explicit ReadResult(const Cursor& cursor_) : - length(0), - cursor(cursor_), - result(false) - {} + ReadResult() = delete; + ReadResult(const ReadResult&) = default; + ReadResult(ReadResult&&) = default; explicit ReadResult(RawStorageBase& storage) : - length(0), - cursor(storage), - result(false) + _cursor(storage) {} + ReadResult(RawStorageBase& storage, uint16_t begin, uint16_t end) : + _cursor(storage, begin, end), + _length((_cursor.size() > 2) ? _cursor.size() - 2 : 0), + _result(true) + {} + + ReadResult& operator=(const ReadResult&) = default; + ReadResult& operator=(ReadResult&&) = default; + explicit operator bool() const { - return result; + return _result; } - String read() { + uint16_t begin() const { + return _cursor.begin; + } + + uint16_t end() const { + return _cursor.end; + } + + uint16_t length() const { + return _length; + } + + void reset(uint16_t begin, uint16_t end) { + _cursor.reset(begin, end); + _length = _cursor.size() > 2 ? _cursor.size() - 2 : 0; + _result = true; + } + + String read() const { String out; - out.reserve(length); - if (!length) { + + if (!_length) { return out; } - decltype(length) index = 0; - cursor.resetBeginning(); - while (index < length) { + auto cursor { _cursor }; + out.reserve(_length); + decltype(_length) index = 0; + while (index < _length) { out += static_cast(cursor.read()); ++cursor; ++index; @@ -237,17 +264,16 @@ class KeyValueStore { return out; } - uint16_t length; - private: - Cursor cursor; - bool result; + bool _result { false }; + uint16_t _length { 0 }; + Cursor _cursor; }; // Internal storage consists of sequences of struct KeyValueResult { explicit operator bool() const { - return (key) && (value) && (key.length > 0); + return (key) && (value) && (key.length() > 0); } bool operator !() { @@ -327,21 +353,21 @@ class KeyValueStore { break; } - start_pos = kv.value.cursor.begin; + start_pos = kv.value.begin(); // in the very special case we can match the existing key, we either - if ((kv.key.length == key_len) && (kv.key.read() == key)) { - if (kv.value.length == value.length()) { + if ((kv.key.length() == key_len) && (kv.key.read() == key)) { + if (kv.value.length() == value.length()) { // - do nothing, as the value is already set if (kv.value.read() == value) { return true; } // - overwrite the space again, with the new kv of the same length - start_pos = kv.key.cursor.end; + start_pos = kv.key.end(); break; } // - or, erase the existing kv and place new kv at the end - to_erase.reset(kv.value.cursor.begin, kv.key.cursor.end); + to_erase.reset(kv.value.begin(), kv.key.end()); need_erase = true; } @@ -404,9 +430,9 @@ class KeyValueStore { auto to_erase = Cursor::fromEnd(_storage, _cursor.begin, _cursor.end); foreach([&](KeyValueResult&& kv) { - start_pos = kv.value.cursor.begin; - if (!to_erase && (kv.key.length == key_len) && (kv.key.read() == key)) { - to_erase.reset(kv.value.cursor.begin, kv.key.cursor.end); + start_pos = kv.value.begin(); + if (!to_erase && (kv.key.length() == key_len) && (kv.key.read() == key)) { + to_erase.reset(kv.value.begin(), kv.key.end()); } }); @@ -433,8 +459,8 @@ class KeyValueStore { size_t available() { size_t result = _cursor.size(); foreach([&result](KeyValueResult&& kv) { - result -= kv.key.cursor.size(); - result -= kv.value.cursor.size(); + result -= kv.key._cursor.size(); + result -= kv.value._cursor.size(); }); return result; @@ -465,7 +491,7 @@ class KeyValueStore { // no point in comparing keys when length does not match // (and we also don't want to allocate the string) - if (kv.key.length != len) { + if (kv.key.length() != len) { continue; } @@ -498,7 +524,7 @@ class KeyValueStore { // right now, just construct in place and assume that compiler will inline things KeyValueResult _read_kv() { auto key = _raw_read(); - if (!key || !key.length) { + if (!key || !key.length()) { return {_storage}; } @@ -595,10 +621,7 @@ class KeyValueStore { _cursor.position -= len; auto value_start = (_cursor.begin + _cursor.position); - out.cursor.reset(value_start, value_start + len + 2); - out.length = len; - out.result = true; - + out.reset(value_start, value_start + len + 2); _state = State::Begin; goto return_result;