From d6df009842007f6497b4de6f941d4957a925f5c3 Mon Sep 17 00:00:00 2001 From: kssenii Date: Fri, 19 May 2023 12:22:46 +0200 Subject: [PATCH 1/3] Fix --- src/Interpreters/Cache/FileCache.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 2d5744b630e..78670bb49e4 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -609,17 +609,19 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) if (releasable) { - removed_size += segment_metadata->size(); - --queue_size; - auto segment = segment_metadata->file_segment; if (segment->state() == FileSegment::State::DOWNLOADED) { const auto & key = segment->key(); + auto it = to_delete.find(key); if (it == to_delete.end()) it = to_delete.emplace(key, locked_key.getKeyMetadata()).first; it->second.add(segment_metadata); + + removed_size += segment_metadata->size(); + --queue_size; + return PriorityIterationResult::CONTINUE; } From 2d0ebba67f013325c6ff5bc0267e906310f2ea64 Mon Sep 17 00:00:00 2001 From: kssenii Date: Mon, 22 May 2023 12:58:56 +0200 Subject: [PATCH 2/3] Better --- src/Interpreters/Cache/FileCache.cpp | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 78670bb49e4..65fc489f5ad 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -563,17 +563,9 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) file_segment.key(), file_segment.offset()); } - size_t queue_size = main_priority->getElementsCount(cache_lock); - chassert(queue_size <= main_priority->getElementsLimit()); - /// A file_segment_metadata acquires a LRUQueue iterator on first successful space reservation attempt. auto queue_iterator = file_segment.getQueueIterator(); - if (queue_iterator) - chassert(file_segment.getReservedSize() > 0); - else - queue_size += 1; - - size_t removed_size = 0; + chassert(!queue_iterator || file_segment.getReservedSize() > 0); class EvictionCandidates final : public std::vector { @@ -599,6 +591,7 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) }; std::unordered_map to_delete; + size_t freeable_space = 0, freeable_count = 0; auto iterate_func = [&](LockedKey & locked_key, FileSegmentMetadataPtr segment_metadata) { @@ -619,8 +612,8 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) it = to_delete.emplace(key, locked_key.getKeyMetadata()).first; it->second.add(segment_metadata); - removed_size += segment_metadata->size(); - --queue_size; + freeable_space += segment_metadata->size(); + freeable_count += 1; return PriorityIterationResult::CONTINUE; } @@ -636,7 +629,7 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) { auto is_query_priority_overflow = [&] { - const size_t new_size = query_priority->getSize(cache_lock) + size - removed_size; + const size_t new_size = query_priority->getSize(cache_lock) + size - freeable_space; return new_size > query_priority->getSizeLimit(); }; @@ -656,9 +649,11 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) auto is_main_priority_overflow = [&] { /// max_size == 0 means unlimited cache size, - /// max_element_size means unlimited number of cache elements. - return (main_priority->getSizeLimit() != 0 && main_priority->getSize(cache_lock) + size - removed_size > main_priority->getSizeLimit()) - || (main_priority->getElementsLimit() != 0 && queue_size > main_priority->getElementsLimit()); + /// max_element_size == 0 means unlimited number of cache elements. + return (main_priority->getSizeLimit() != 0 + && (main_priority->getSize(cache_lock) + size - freeable_space > main_priority->getSizeLimit())) + || (main_priority->getElementsLimit() != 0 + && (main_priority->getElementsCount(cache_lock) + 1 - freeable_count > main_priority->getElementsLimit())); }; main_priority->iterate( From 6bfbbc94bf72e657b64de4c50f8a6c1cd3c087a0 Mon Sep 17 00:00:00 2001 From: kssenii Date: Mon, 22 May 2023 14:59:35 +0200 Subject: [PATCH 3/3] A little better --- src/Interpreters/Cache/FileCache.cpp | 32 +++++++++++++++++----------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 85648d9d96a..183ae686908 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -634,13 +634,16 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) return new_size > query_priority->getSizeLimit(); }; - query_priority->iterate( - [&](LockedKey & locked_key, FileSegmentMetadataPtr segment_metadata) - { return is_query_priority_overflow() ? iterate_func(locked_key, segment_metadata) : PriorityIterationResult::BREAK; }, - cache_lock); - if (is_query_priority_overflow()) - return false; + { + query_priority->iterate( + [&](LockedKey & locked_key, FileSegmentMetadataPtr segment_metadata) + { return is_query_priority_overflow() ? iterate_func(locked_key, segment_metadata) : PriorityIterationResult::BREAK; }, + cache_lock); + + if (is_query_priority_overflow()) + return false; + } LOG_TEST( log, "Query limits satisfied (while reserving for {}:{})", @@ -654,7 +657,7 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) const bool is_overflow = (main_priority->getSizeLimit() != 0 && (main_priority->getSize(cache_lock) + size - freeable_space > main_priority->getSizeLimit())) || (main_priority->getElementsLimit() != 0 - && (main_priority->getElementsCount(cache_lock) + 1 - freeable_count > main_priority->getElementsLimit())); + && freeable_count == 0 && main_priority->getElementsCount(cache_lock) == main_priority->getElementsLimit()); LOG_TEST( log, "Overflow: {}, size: {}, ready to remove: {}, current cache size: {}/{}, elements: {}/{}, while reserving for {}:{}", @@ -666,13 +669,16 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) return is_overflow; }; - main_priority->iterate( - [&](LockedKey & locked_key, FileSegmentMetadataPtr segment_metadata) - { return is_main_priority_overflow() ? iterate_func(locked_key, segment_metadata) : PriorityIterationResult::BREAK; }, - cache_lock); - if (is_main_priority_overflow()) - return false; + { + main_priority->iterate( + [&](LockedKey & locked_key, FileSegmentMetadataPtr segment_metadata) + { return is_main_priority_overflow() ? iterate_func(locked_key, segment_metadata) : PriorityIterationResult::BREAK; }, + cache_lock); + + if (is_main_priority_overflow()) + return false; + } if (!file_segment.getKeyMetadata()->createBaseDirectory()) return false;