Less contention in cache (Part 3)

src/Common/ProfileEvents.cpp

@@ -459,7 +459,8 @@ The server successfully detected this situation and will download merged part fr
     M(FilesystemCacheLoadMetadataMicroseconds, "Time spent loading filesystem cache metadata") \
     M(FilesystemCacheEvictedBytes, "Number of bytes evicted from filesystem cache") \
     M(FilesystemCacheEvictedFileSegments, "Number of file segments evicted from filesystem cache") \
-    M(FilesystemCacheEvictionSkippedFileSegments, "Number of file segments skipped for eviction because of being unreleasable") \
+    M(FilesystemCacheEvictionSkippedFileSegments, "Number of file segments skipped for eviction because of being in unreleasable state") \
+    M(FilesystemCacheEvictionSkippedEvictingFileSegments, "Number of file segments skipped for eviction because of being in evicting state") \
     M(FilesystemCacheEvictionTries, "Number of filesystem cache eviction attempts") \
     M(FilesystemCacheLockKeyMicroseconds, "Lock cache key time") \
     M(FilesystemCacheLockMetadataMicroseconds, "Lock filesystem cache metadata time") \

src/Interpreters/Cache/EvictionCandidates.cpp

@@ -32,7 +32,7 @@ void EvictionCandidates::add(LockedKey & locked_key, const FileSegmentMetadataPt
     ++candidates_size;
 }
 
-void EvictionCandidates::evict(FileCacheQueryLimit::QueryContext * query_context, const CacheGuard::Lock & lock)
+void EvictionCandidates::evict()
 {
     if (candidates.empty())
         return;
@@ -59,14 +59,27 @@ void EvictionCandidates::evict(FileCacheQueryLimit::QueryContext * query_context
             ProfileEvents::increment(ProfileEvents::FilesystemCacheEvictedBytes, segment->range().size());
 
             locked_key->removeFileSegment(segment->offset(), segment->lock());
-            queue_it->remove(lock);
+            queue_it->invalidate();
-
-            if (query_context)
-                query_context->remove(segment->key(), segment->offset(), lock);
 
             to_evict.pop_back();
         }
     }
 }
 
+void EvictionCandidates::finalize(FileCacheQueryLimit::QueryContext * query_context, const CacheGuard::Lock & lock)
+{
+    for (const auto & it : invalidated_queue_entries)
+    {
+        if (query_context)
+        {
+            const auto & entry = it->getEntry();
+            query_context->remove(entry->key, entry->offset, lock);
+        }
+        it->remove(lock);
+    }
+
+    if (finalize_eviction_func)
+        finalize_eviction_func(lock);
+}
+
 }

src/Interpreters/Cache/EvictionCandidates.h

@@ -11,7 +11,9 @@ public:
 
     void add(LockedKey & locked_key, const FileSegmentMetadataPtr & candidate);
 
-    void evict(FileCacheQueryLimit::QueryContext * query_context, const CacheGuard::Lock &);
+    void evict();
+
+    void finalize(FileCacheQueryLimit::QueryContext * query_context, const CacheGuard::Lock & lock);
 
     size_t size() const { return candidates_size; }
 
@@ -19,6 +21,9 @@ public:
 
     auto end() const { return candidates.end(); }
 
+    using FinalizeEvictionFunc = std::function<void(const CacheGuard::Lock & lk)>;
+    void setFinalizeEvictionFunc(FinalizeEvictionFunc && func) { finalize_eviction_func = func; }
+
 private:
     struct KeyCandidates
     {
@@ -28,6 +33,8 @@ private:
 
     std::unordered_map<FileCacheKey, KeyCandidates> candidates;
     size_t candidates_size = 0;
+    std::vector<IFileCachePriority::IteratorPtr> invalidated_queue_entries;
+    FinalizeEvictionFunc finalize_eviction_func;
 };
 
 using EvictionCandidatesPtr = std::unique_ptr<EvictionCandidates>;

src/Interpreters/Cache/FileCache.cpp

@@ -814,12 +814,10 @@ bool FileCache::tryReserve(
     }
 
     EvictionCandidates eviction_candidates;
-    IFileCachePriority::FinalizeEvictionFunc finalize_eviction_func;
 
     if (query_priority)
     {
-        if (!query_priority->collectCandidatesForEviction(
+        if (!query_priority->collectCandidatesForEviction(size, reserve_stat, eviction_candidates, {}, user.user_id, cache_lock))
-                size, reserve_stat, eviction_candidates, {}, finalize_eviction_func, user.user_id, cache_lock))
             return false;
 
         LOG_TEST(log, "Query limits satisfied (while reserving for {}:{})", file_segment.key(), file_segment.offset());
@@ -832,26 +830,38 @@ bool FileCache::tryReserve(
     auto queue_iterator = file_segment.getQueueIterator();
     chassert(!queue_iterator || file_segment.getReservedSize() > 0);
 
-    if (!main_priority->collectCandidatesForEviction(
+    if (!main_priority->collectCandidatesForEviction(size, reserve_stat, eviction_candidates, queue_iterator, user.user_id, cache_lock))
-            size, reserve_stat, eviction_candidates, queue_iterator, finalize_eviction_func, user.user_id, cache_lock))
         return false;
 
+    /// Let's release cache lock if we are going to remove files from filesystem.
+    bool release_lock = eviction_candidates.size() > 0;
+    if (release_lock)
+        cache_lock.unlock();
+
     if (!file_segment.getKeyMetadata()->createBaseDirectory())
         return false;
 
-    eviction_candidates.evict(query_context.get(), cache_lock);
+    /// Remove eviction candidates from filesystem.
+    eviction_candidates.evict();
 
-    if (finalize_eviction_func)
+    /// Take cache lock again.
-        finalize_eviction_func(cache_lock);
+    if (release_lock)
+        cache_lock.lock();
 
+    /// Remove invalidated queue entries and execute (only for SLRU) finalize func.
+    eviction_candidates.finalize(query_context.get(), cache_lock);
+
+    /// Space reservation is incremental, so file_segment_metadata is created first (with state Empty),
+    /// and queue_iterator is assigned on first space reservation attempt
+    /// (so it is nullptr here if we are reserving for the first time).
     if (queue_iterator)
     {
-        queue_iterator->updateSize(size);
+        /// Increase size of queue entry.
+        queue_iterator->incrementSize(size, cache_lock);
     }
     else
     {
-        /// Space reservation is incremental, so file_segment_metadata is created first (with state empty),
+        /// Create a new queue entry and assign currently reserved size to it.
-        /// and getQueueIterator() is assigned on first space reservation attempt.
         queue_iterator = main_priority->add(file_segment.getKeyMetadata(), file_segment.offset(), size, user, cache_lock);
         file_segment.setQueueIterator(queue_iterator);
     }
@@ -863,7 +873,7 @@ bool FileCache::tryReserve(
     {
         auto query_queue_it = query_context->tryGet(file_segment.key(), file_segment.offset(), cache_lock);
         if (query_queue_it)
-            query_queue_it->updateSize(size);
+            query_queue_it->incrementSize(size, cache_lock);
         else
             query_context->add(file_segment.getKeyMetadata(), file_segment.offset(), size, user, cache_lock);
     }

src/Interpreters/Cache/IFileCachePriority.h

@@ -45,7 +45,12 @@ public:
 
         virtual size_t increasePriority(const CacheGuard::Lock &) = 0;
 
-        virtual void updateSize(int64_t size) = 0;
+        /// Note: IncrementSize unlike decrementSize requires a cache lock, because
+        /// it requires more consistency guarantees for eviction.
+
+        virtual void incrementSize(size_t size, const CacheGuard::Lock &) = 0;
+
+        virtual void decrementSize(size_t size) = 0;
 
         virtual void remove(const CacheGuard::Lock &) = 0;
 
@@ -93,13 +98,11 @@ public:
 
     virtual PriorityDumpPtr dump(const CacheGuard::Lock &) = 0;
 
-    using FinalizeEvictionFunc = std::function<void(const CacheGuard::Lock & lk)>;
     virtual bool collectCandidatesForEviction(
         size_t size,
         FileCacheReserveStat & stat,
         EvictionCandidates & res,
         IFileCachePriority::IteratorPtr reservee,
-        FinalizeEvictionFunc & finalize_eviction_func,
         const UserID & user_id,
         const CacheGuard::Lock &) = 0;
 

src/Interpreters/Cache/LRUFileCachePriority.cpp

@@ -15,6 +15,7 @@ namespace CurrentMetrics
 namespace ProfileEvents
 {
     extern const Event FilesystemCacheEvictionSkippedFileSegments;
+    extern const Event FilesystemCacheEvictionSkippedEvictingFileSegments;
     extern const Event FilesystemCacheEvictionTries;
     extern const Event FilesystemCacheEvictMicroseconds;
     extern const Event FilesystemCacheEvictedBytes;
@@ -223,7 +224,6 @@ bool LRUFileCachePriority::collectCandidatesForEviction(
     FileCacheReserveStat & stat,
     EvictionCandidates & res,
     IFileCachePriority::IteratorPtr,
-    FinalizeEvictionFunc &,
     const UserID &,
     const CacheGuard::Lock & lock)
 {
@@ -237,7 +237,11 @@ bool LRUFileCachePriority::collectCandidatesForEviction(
         const auto & file_segment = segment_metadata->file_segment;
         chassert(file_segment->assertCorrectness());
 
-        if (segment_metadata->releasable())
+        if (segment_metadata->evicting())
+        {
+            ProfileEvents::increment(ProfileEvents::FilesystemCacheEvictionSkippedEvictingFileSegments);
+        }
+        else if (segment_metadata->releasable())
         {
             res.add(locked_key, segment_metadata);
             stat.update(segment_metadata->size(), file_segment->getKind(), true);
@@ -375,20 +379,34 @@ void LRUFileCachePriority::LRUIterator::invalidate()
     entry->size = 0;
 }
 
-void LRUFileCachePriority::LRUIterator::updateSize(int64_t size)
+void LRUFileCachePriority::LRUIterator::incrementSize(size_t size, const CacheGuard::Lock &)
 {
     assertValid();
 
     const auto & entry = *iterator;
     LOG_TEST(
         cache_priority->log,
-        "Update size with {} in LRU queue for key: {}, offset: {}, previous size: {}",
+        "Increment size with {} in LRU queue for key: {}, offset: {}, previous size: {}",
         size, entry->key, entry->offset, entry->size);
 
     cache_priority->updateSize(size);
     entry->size += size;
 }
 
+void LRUFileCachePriority::LRUIterator::decrementSize(size_t size)
+{
+    assertValid();
+
+    const auto & entry = *iterator;
+    LOG_TEST(
+        cache_priority->log,
+        "Decrement size with {} in LRU queue for key: {}, offset: {}, previous size: {}",
+        size, entry->key, entry->offset, entry->size);
+
+    cache_priority->updateSize(-size);
+    entry->size -= size;
+}
+
 size_t LRUFileCachePriority::LRUIterator::increasePriority(const CacheGuard::Lock &)
 {
     assertValid();

src/Interpreters/Cache/LRUFileCachePriority.h

@@ -47,7 +47,6 @@ public:
         FileCacheReserveStat & stat,
         EvictionCandidates & res,
         IFileCachePriority::IteratorPtr reservee,
-        FinalizeEvictionFunc & finalize_eviction_func,
         const UserID & user_id,
         const CacheGuard::Lock &) override;
 
@@ -114,7 +113,9 @@ public:
 
     void invalidate() override;
 
-    void updateSize(int64_t size) override;
+    void incrementSize(size_t size, const CacheGuard::Lock &) override;
+
+    void decrementSize(size_t size) override;
 
     QueueEntryType getType() const override { return QueueEntryType::LRU; }
 

src/Interpreters/Cache/Metadata.cpp

@@ -1012,7 +1012,7 @@ void LockedKey::shrinkFileSegmentToDownloadedSize(
         file_segment->cache, key_metadata, file_segment->queue_iterator);
 
     if (diff)
-        metadata->getQueueIterator()->updateSize(-diff);
+        metadata->getQueueIterator()->decrementSize(diff);
 
     chassert(file_segment->assertCorrectnessUnlocked(segment_lock));
 }

src/Interpreters/Cache/SLRUFileCachePriority.cpp

@@ -101,7 +101,6 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
     FileCacheReserveStat & stat,
     EvictionCandidates & res,
     IFileCachePriority::IteratorPtr reservee,
-    FinalizeEvictionFunc & finalize_eviction_func,
     const UserID & user_id,
     const CacheGuard::Lock & lock)
 {
@@ -109,7 +108,7 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
     /// for a corresponding file segment, so it will be directly put into probationary queue.
     if (!reservee)
     {
-        return probationary_queue.collectCandidatesForEviction(size, stat, res, reservee, finalize_eviction_func, user_id, lock);
+        return probationary_queue.collectCandidatesForEviction(size, stat, res, reservee, user_id, lock);
     }
 
     /// If `it` not nullptr (e.g. is already in some queue),
@@ -117,7 +116,7 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
     /// (in order to know where we need to free space).
     if (!assert_cast<SLRUIterator *>(reservee.get())->is_protected)
     {
-        return probationary_queue.collectCandidatesForEviction(size, stat, res, reservee, finalize_eviction_func, user_id, lock);
+        return probationary_queue.collectCandidatesForEviction(size, stat, res, reservee, user_id, lock);
     }
 
     /// Entry is in protected queue.
@@ -132,18 +131,17 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
     /// required to make space for additionary `size` bytes for entry.
     auto downgrade_candidates = std::make_shared<EvictionCandidates>();
     FileCacheReserveStat downgrade_stat;
-    FinalizeEvictionFunc noop;
 
-    if (!protected_queue.collectCandidatesForEviction(size, downgrade_stat, *downgrade_candidates, reservee, noop, user_id, lock))
+    if (!protected_queue.collectCandidatesForEviction(size, downgrade_stat, *downgrade_candidates, reservee, user_id, lock))
         return false;
 
     const size_t size_to_downgrade = downgrade_stat.stat.releasable_size;
 
     if (!probationary_queue.canFit(size_to_downgrade, lock)
-        && !probationary_queue.collectCandidatesForEviction(size_to_downgrade, stat, res, reservee, noop, user_id, lock))
+        && !probationary_queue.collectCandidatesForEviction(size_to_downgrade, stat, res, reservee, user_id, lock))
         return false;
 
-    finalize_eviction_func = [=, this](const CacheGuard::Lock & lk) mutable
+    res.setFinalizeEvictionFunc([=, this](const CacheGuard::Lock & lk) mutable
     {
         for (const auto & [key, key_candidates] : *downgrade_candidates)
         {
@@ -154,7 +152,7 @@ bool SLRUFileCachePriority::collectCandidatesForEviction(
                 candidate_it->is_protected = false;
             }
         }
-    };
+    });
 
     return true;
 }
@@ -186,9 +184,8 @@ void SLRUFileCachePriority::increasePriority(SLRUIterator & iterator, const Cach
     /// queue to probationary queue.
     EvictionCandidates downgrade_candidates;
     FileCacheReserveStat downgrade_stat;
-    FinalizeEvictionFunc noop;
 
-    if (!protected_queue.collectCandidatesForEviction(size, downgrade_stat, downgrade_candidates, {}, noop, "", lock))
+    if (!protected_queue.collectCandidatesForEviction(size, downgrade_stat, downgrade_candidates, {}, "", lock))
     {
         /// We cannot make space for entry to be moved to protected queue
         /// (not enough releasable file segments).
@@ -211,7 +208,7 @@ void SLRUFileCachePriority::increasePriority(SLRUIterator & iterator, const Cach
 
     if (size_to_free)
     {
-        if (!probationary_queue.collectCandidatesForEviction(size_to_free, stat, eviction_candidates, {}, noop, {}, lock))
+        if (!probationary_queue.collectCandidatesForEviction(size_to_free, stat, eviction_candidates, {}, {}, lock))
         {
             /// "downgrade" candidates cannot be moved to probationary queue,
             /// so entry cannot be moved to protected queue as well.
@@ -220,7 +217,7 @@ void SLRUFileCachePriority::increasePriority(SLRUIterator & iterator, const Cach
             return;
         }
         /// Make space for "downgrade" candidates.
-        eviction_candidates.evict(nullptr, lock);
+        eviction_candidates.evict();
     }
 
     /// All checks passed, now we can move downgrade candidates to
@@ -294,10 +291,16 @@ size_t SLRUFileCachePriority::SLRUIterator::increasePriority(const CacheGuard::L
     return getEntry()->hits;
 }
 
-void SLRUFileCachePriority::SLRUIterator::updateSize(int64_t size)
+void SLRUFileCachePriority::SLRUIterator::incrementSize(size_t size, const CacheGuard::Lock & lock)
 {
     assertValid();
-    lru_iterator.updateSize(size);
+    lru_iterator.incrementSize(size, lock);
+}
+
+void SLRUFileCachePriority::SLRUIterator::decrementSize(size_t size)
+{
+    assertValid();
+    lru_iterator.decrementSize(size);
 }
 
 void SLRUFileCachePriority::SLRUIterator::invalidate()

src/Interpreters/Cache/SLRUFileCachePriority.h

@@ -44,7 +44,6 @@ public:
         FileCacheReserveStat & stat,
         EvictionCandidates & res,
         IFileCachePriority::IteratorPtr reservee,
-        FinalizeEvictionFunc & finalize_eviction_func,
         const UserID & user_id,
         const CacheGuard::Lock &) override;
 
@@ -80,7 +79,9 @@ public:
 
     void invalidate() override;
 
-    void updateSize(int64_t size) override;
+    void incrementSize(size_t size, const CacheGuard::Lock &) override;
+
+    void decrementSize(size_t size) override;
 
     QueueEntryType getType() const override { return is_protected ? QueueEntryType::SLRU_Protected : QueueEntryType::SLRU_Probationary; }