From f5dc07d052e60bc6ebe6d27a743722ae1b6b8e27 Mon Sep 17 00:00:00 2001
From: Igor Nikonov <igor@clickhouse.com>
Date: Sun, 21 May 2023 22:01:28 +0000
Subject: [PATCH] tryReserve() cleanup

simplify removing eviction candidates
---
 src/Interpreters/Cache/FileCache.cpp | 23 ++++++++++-------------
 src/Interpreters/Cache/Metadata.h    |  2 +-
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp
index 9ab7943e263..55d7177fd4c 100644
--- a/src/Interpreters/Cache/FileCache.cpp
+++ b/src/Interpreters/Cache/FileCache.cpp
@@ -573,27 +573,25 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size)
     else
         queue_size += 1;
 
-    class EvictionCandidates final : public std::vector<FileSegmentMetadataPtr>
+    struct EvictionCandidates
     {
-    public:
         explicit EvictionCandidates(KeyMetadataPtr key_metadata_) : key_metadata(key_metadata_) {}
 
-        KeyMetadata & getMetadata() { return *key_metadata; }
-
         void add(FileSegmentMetadataPtr candidate)
         {
             candidate->removal_candidate = true;
-            push_back(candidate);
+            candidates.push_back(candidate);
         }
 
         ~EvictionCandidates()
         {
-            for (const auto & candidate : *this)
+            // todo: it looks redundant, - why is it needed?
+            for (const auto & candidate : candidates)
                 candidate->removal_candidate = false;
         }
 
-    private:
         KeyMetadataPtr key_metadata;
+        std::vector<FileSegmentMetadataPtr> candidates;
     };
 
     std::unordered_map<Key, EvictionCandidates> to_delete;
@@ -681,23 +679,22 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size)
 
     for (auto & [current_key, deletion_info] : to_delete)
     {
-        auto locked_key = deletion_info.getMetadata().tryLock();
+        auto locked_key = deletion_info.key_metadata->tryLock();
         if (!locked_key)
             continue; /// key could become invalid after we released the key lock above, just skip it.
 
-        for (auto it = deletion_info.begin(); it != deletion_info.end();)
+        for (const auto & candidate : deletion_info.candidates)
         {
-            chassert((*it)->releasable());
+            chassert(candidate->releasable());
 
-            auto segment = (*it)->file_segment;
+            auto segment = candidate->file_segment;
             locked_key->removeFileSegment(segment->offset(), segment->lock());
             segment->getQueueIterator()->remove(cache_lock);
 
             if (query_context)
                 query_context->remove(current_key, segment->offset(), cache_lock);
-
-            it = deletion_info.erase(it);
         }
+        deletion_info.candidates.clear();
     }
 
     /// queue_iteratir is std::nullopt here if no space has been reserved yet, a file_segment_metadata
diff --git a/src/Interpreters/Cache/Metadata.h b/src/Interpreters/Cache/Metadata.h
index 586c7e5c2a8..2e015b07ed0 100644
--- a/src/Interpreters/Cache/Metadata.h
+++ b/src/Interpreters/Cache/Metadata.h
@@ -24,7 +24,7 @@ struct FileSegmentMetadata : private boost::noncopyable
 
     bool valid() const { return !removal_candidate.load(); }
 
-    Priority::Iterator getQueueIterator() { return file_segment->getQueueIterator(); }
+    Priority::Iterator getQueueIterator() const { return file_segment->getQueueIterator(); }
 
     FileSegmentPtr file_segment;
     std::atomic<bool> removal_candidate{false};