From c9c26d489840a840ff96f94144b07fed197c5598 Mon Sep 17 00:00:00 2001 From: alexX512 Date: Mon, 8 Aug 2022 20:53:02 +0000 Subject: [PATCH] Fix review --- src/Common/CacheBase.h | 6 ++---- src/Common/SLRUCachePolicy.h | 30 ++++++++++++++++----------- src/Common/tests/gtest_lru_cache.cpp | 12 +++++------ src/Common/tests/gtest_slru_cahce.cpp | 22 ++++++++++---------- src/IO/UncompressedCache.h | 2 +- src/Storages/MarkCache.h | 2 +- 6 files changed, 39 insertions(+), 35 deletions(-) diff --git a/src/Common/CacheBase.h b/src/Common/CacheBase.h index b8aa45a3443..40c41d9f087 100644 --- a/src/Common/CacheBase.h +++ b/src/Common/CacheBase.h @@ -37,14 +37,12 @@ public: using Mapped = TMapped; using MappedPtr = std::shared_ptr; - CacheBase(size_t max_size, size_t max_elements_size = 0) : CacheBase(default_cache_policy_name, max_size, max_elements_size) {} - /// TODO: Rewrite to custom struct with fields for all cache policies. - CacheBase(String cache_policy_name, size_t max_size, size_t max_elements_size = 0, double size_ratio = 0.5) + CacheBase(size_t max_size, size_t max_elements_size = 0, String cache_policy_name = "", double size_ratio = 0.5) { auto on_weight_loss_function = [&](size_t weight_loss) { onRemoveOverflowWeightLoss(weight_loss); }; - if (cache_policy_name == "") + if (cache_policy_name.empty()) { cache_policy_name = default_cache_policy_name; } diff --git a/src/Common/SLRUCachePolicy.h b/src/Common/SLRUCachePolicy.h index 02b9133626e..10b043ebaca 100644 --- a/src/Common/SLRUCachePolicy.h +++ b/src/Common/SLRUCachePolicy.h @@ -183,21 +183,27 @@ protected: size_t current_weight_lost = 0; size_t queue_size = queue.size(); - auto need_remove = [&]() -> bool + std::function need_remove; + if (is_protected) { - if (is_protected) + /// Check if after remove all elements from probationary part there will be no more than max elements + /// in protected queue and weight of all protected elements will be less then max protected weight. + /// It's not possible to check only cells.size() > max_elements_size + /// because protected elements move to probationary part and still remain in cache. + need_remove = [&]() { - /// Check if after remove all elements from probationary part there will be no more than max elements - /// in protected queue and weight of all protected elements will be less then max protected weight. - /// It's not possible to check only cells.size() > max_elements_size - /// because protected elements move to probationary part and still remain in cache. return ((max_elements_size != 0 && cells.size() - probationary_queue.size() > max_elements_size) - || (current_weight_size > max_weight_size)) - && (queue_size > 0); - } - return ((max_elements_size != 0 && cells.size() > max_elements_size) || (current_weight_size > max_weight_size)) - && (queue_size > 0); - }; + || (current_weight_size > max_weight_size)) && (queue_size > 0); + }; + } + else + { + need_remove = [&]() + { + return ((max_elements_size != 0 && cells.size() > max_elements_size) + || (current_weight_size > max_weight_size)) && (queue_size > 0); + }; + } while (need_remove()) { diff --git a/src/Common/tests/gtest_lru_cache.cpp b/src/Common/tests/gtest_lru_cache.cpp index 9a2cb354bd5..f74d1eb9464 100644 --- a/src/Common/tests/gtest_lru_cache.cpp +++ b/src/Common/tests/gtest_lru_cache.cpp @@ -6,7 +6,7 @@ TEST(LRUCache, set) { using SimpleCacheBase = DB::CacheBase; - auto lru_cache = SimpleCacheBase("LRU", /*max_size*/ 10, /*max_elements_size*/ 10); + auto lru_cache = SimpleCacheBase(/*max_size*/ 10, /*max_elements_size*/ 10, "LRU"); lru_cache.set(1, std::make_shared(2)); lru_cache.set(2, std::make_shared(3)); @@ -19,7 +19,7 @@ TEST(LRUCache, set) TEST(LRUCache, update) { using SimpleCacheBase = DB::CacheBase; - auto lru_cache = SimpleCacheBase("LRU", /*max_size*/ 10, /*max_elements_size*/ 10); + auto lru_cache = SimpleCacheBase(/*max_size*/ 10, /*max_elements_size*/ 10, "LRU"); lru_cache.set(1, std::make_shared(2)); lru_cache.set(1, std::make_shared(3)); auto val = lru_cache.get(1); @@ -30,7 +30,7 @@ TEST(LRUCache, update) TEST(LRUCache, get) { using SimpleCacheBase = DB::CacheBase; - auto lru_cache = SimpleCacheBase("LRU", /*max_size*/ 10, /*max_elements_size*/ 10); + auto lru_cache = SimpleCacheBase(/*max_size*/ 10, /*max_elements_size*/ 10, "LRU"); lru_cache.set(1, std::make_shared(2)); lru_cache.set(2, std::make_shared(3)); SimpleCacheBase::MappedPtr value = lru_cache.get(1); @@ -50,7 +50,7 @@ struct ValueWeight TEST(LRUCache, evictOnSize) { using SimpleCacheBase = DB::CacheBase; - auto lru_cache = SimpleCacheBase("LRU", /*max_size*/ 20, /*max_elements_size*/ 3); + auto lru_cache = SimpleCacheBase(/*max_size*/ 20, /*max_elements_size*/ 3, "LRU"); lru_cache.set(1, std::make_shared(2)); lru_cache.set(2, std::make_shared(3)); lru_cache.set(3, std::make_shared(4)); @@ -66,7 +66,7 @@ TEST(LRUCache, evictOnSize) TEST(LRUCache, evictOnWeight) { using SimpleCacheBase = DB::CacheBase, ValueWeight>; - auto lru_cache = SimpleCacheBase("LRU", /*max_size*/ 10, /*max_elements_size*/ 10); + auto lru_cache = SimpleCacheBase(/*max_size*/ 10, /*max_elements_size*/ 10, "LRU"); lru_cache.set(1, std::make_shared(2)); lru_cache.set(2, std::make_shared(3)); lru_cache.set(3, std::make_shared(4)); @@ -87,7 +87,7 @@ TEST(LRUCache, evictOnWeight) TEST(LRUCache, getOrSet) { using SimpleCacheBase = DB::CacheBase, ValueWeight>; - auto lru_cache = SimpleCacheBase("LRU", /*max_size*/ 10, /*max_elements_size*/ 10); + auto lru_cache = SimpleCacheBase(/*max_size*/ 10, /*max_elements_size*/ 10, "LRU"); size_t x = 10; auto load_func = [&] { return std::make_shared(x); }; auto [value, loaded] = lru_cache.getOrSet(1, load_func); diff --git a/src/Common/tests/gtest_slru_cahce.cpp b/src/Common/tests/gtest_slru_cahce.cpp index a25d28c35dc..e06e03909b4 100644 --- a/src/Common/tests/gtest_slru_cahce.cpp +++ b/src/Common/tests/gtest_slru_cahce.cpp @@ -6,7 +6,7 @@ TEST(SLRUCache, set) { using SimpleCacheBase = DB::CacheBase; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(2, std::make_shared(3)); @@ -19,7 +19,7 @@ TEST(SLRUCache, set) TEST(SLRUCache, update) { using SimpleCacheBase = DB::CacheBase; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(1, std::make_shared(3)); @@ -31,7 +31,7 @@ TEST(SLRUCache, update) TEST(SLRUCache, get) { using SimpleCacheBase = DB::CacheBase; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(2, std::make_shared(3)); @@ -47,7 +47,7 @@ TEST(SLRUCache, get) TEST(SLRUCache, remove) { using SimpleCacheBase = DB::CacheBase; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(2, std::make_shared(3)); @@ -63,7 +63,7 @@ TEST(SLRUCache, remove) TEST(SLRUCache, removeFromProtected) { using SimpleCacheBase = DB::CacheBase; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/2, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/2, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(1, std::make_shared(3)); @@ -96,7 +96,7 @@ TEST(SLRUCache, removeFromProtected) TEST(SLRUCache, reset) { using SimpleCacheBase = DB::CacheBase; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(2, std::make_shared(3)); @@ -119,7 +119,7 @@ struct ValueWeight TEST(SLRUCache, evictOnElements) { using SimpleCacheBase = DB::CacheBase, ValueWeight>; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/1); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/1, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(2, std::make_shared(3)); @@ -140,7 +140,7 @@ TEST(SLRUCache, evictOnElements) TEST(SLRUCache, evictOnWeight) { using SimpleCacheBase = DB::CacheBase, ValueWeight>; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(2, std::make_shared(3)); slru_cache.set(3, std::make_shared(4)); @@ -161,7 +161,7 @@ TEST(SLRUCache, evictOnWeight) TEST(SLRUCache, evictFromProtectedPart) { using SimpleCacheBase = DB::CacheBase, ValueWeight>; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(1, std::make_shared(2)); @@ -177,7 +177,7 @@ TEST(SLRUCache, evictFromProtectedPart) TEST(SLRUCache, evictStreamProtected) { using SimpleCacheBase = DB::CacheBase, ValueWeight>; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); slru_cache.set(1, std::make_shared(2)); slru_cache.set(1, std::make_shared(2)); @@ -201,7 +201,7 @@ TEST(SLRUCache, evictStreamProtected) TEST(SLRUCache, getOrSet) { using SimpleCacheBase = DB::CacheBase, ValueWeight>; - auto slru_cache = SimpleCacheBase("SLRU", /*max_total_size=*/10, /*max_elements_size=*/0, /*size_ratio*/0.5); + auto slru_cache = SimpleCacheBase(/*max_total_size=*/10, /*max_elements_size=*/0, "SLRU", /*size_ratio*/0.5); size_t x = 5; auto load_func = [&] { return std::make_shared(x); }; auto [value, loaded] = slru_cache.getOrSet(1, load_func); diff --git a/src/IO/UncompressedCache.h b/src/IO/UncompressedCache.h index 438f768c735..3d1c907d364 100644 --- a/src/IO/UncompressedCache.h +++ b/src/IO/UncompressedCache.h @@ -43,7 +43,7 @@ private: public: explicit UncompressedCache(size_t max_size_in_bytes, const String & uncompressed_cache_policy = "") - : Base(uncompressed_cache_policy, max_size_in_bytes) {} + : Base(max_size_in_bytes, 0, uncompressed_cache_policy) {} /// Calculate key from path to file and offset. static UInt128 hash(const String & path_to_file, size_t offset) diff --git a/src/Storages/MarkCache.h b/src/Storages/MarkCache.h index b8924d43384..9095bf6bb35 100644 --- a/src/Storages/MarkCache.h +++ b/src/Storages/MarkCache.h @@ -41,7 +41,7 @@ private: public: explicit MarkCache(size_t max_size_in_bytes, const String & mark_cache_policy = "") - : Base(mark_cache_policy, max_size_in_bytes) {} + : Base(max_size_in_bytes, 0, mark_cache_policy) {} /// Calculate key from path to file and offset. static UInt128 hash(const String & path_to_file)