diff --git a/src/Common/ProfileEvents.cpp b/src/Common/ProfileEvents.cpp index c1ac3d08245..7ca11a16cc2 100644 --- a/src/Common/ProfileEvents.cpp +++ b/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") \ diff --git a/src/Interpreters/Cache/EvictionCandidates.cpp b/src/Interpreters/Cache/EvictionCandidates.cpp index 7dceab4f95f..fb0b3809f80 100644 --- a/src/Interpreters/Cache/EvictionCandidates.cpp +++ b/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); - - if (query_context) - query_context->remove(segment->key(), segment->offset(), lock); + queue_it->invalidate(); 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); +} + } diff --git a/src/Interpreters/Cache/EvictionCandidates.h b/src/Interpreters/Cache/EvictionCandidates.h index 0557962d97f..7859762be09 100644 --- a/src/Interpreters/Cache/EvictionCandidates.h +++ b/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 setFinalizeEvictionFunc(FinalizeEvictionFunc && func) { finalize_eviction_func = func; } + private: struct KeyCandidates { @@ -28,6 +33,8 @@ private: std::unordered_map candidates; size_t candidates_size = 0; + std::vector invalidated_queue_entries; + FinalizeEvictionFunc finalize_eviction_func; }; using EvictionCandidatesPtr = std::unique_ptr; diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 9c705ddc27c..90508d74554 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/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( - size, reserve_stat, eviction_candidates, {}, finalize_eviction_func, user.user_id, cache_lock)) + if (!query_priority->collectCandidatesForEviction(size, reserve_stat, eviction_candidates, {}, 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( - size, reserve_stat, eviction_candidates, queue_iterator, finalize_eviction_func, user.user_id, cache_lock)) + if (!main_priority->collectCandidatesForEviction(size, reserve_stat, eviction_candidates, queue_iterator, 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) - finalize_eviction_func(cache_lock); + /// Take cache lock again. + 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), - /// and getQueueIterator() is assigned on first space reservation attempt. + /// Create a new queue entry and assign currently reserved size to it. 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); } diff --git a/src/Interpreters/Cache/IFileCachePriority.h b/src/Interpreters/Cache/IFileCachePriority.h index bc036166940..37270cf0873 100644 --- a/src/Interpreters/Cache/IFileCachePriority.h +++ b/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; 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; diff --git a/src/Interpreters/Cache/LRUFileCachePriority.cpp b/src/Interpreters/Cache/LRUFileCachePriority.cpp index bce03b60024..05bbc26e602 100644 --- a/src/Interpreters/Cache/LRUFileCachePriority.cpp +++ b/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(); diff --git a/src/Interpreters/Cache/LRUFileCachePriority.h b/src/Interpreters/Cache/LRUFileCachePriority.h index a74a4b8b621..d8907f678a2 100644 --- a/src/Interpreters/Cache/LRUFileCachePriority.h +++ b/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; } diff --git a/src/Interpreters/Cache/Metadata.cpp b/src/Interpreters/Cache/Metadata.cpp index 727f2762cca..eb3b4e44c8c 100644 --- a/src/Interpreters/Cache/Metadata.cpp +++ b/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)); } diff --git a/src/Interpreters/Cache/SLRUFileCachePriority.cpp b/src/Interpreters/Cache/SLRUFileCachePriority.cpp index 43f1c1012ba..543d6a03669 100644 --- a/src/Interpreters/Cache/SLRUFileCachePriority.cpp +++ b/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(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(); 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() diff --git a/src/Interpreters/Cache/SLRUFileCachePriority.h b/src/Interpreters/Cache/SLRUFileCachePriority.h index d97fa80a6c7..28e61396572 100644 --- a/src/Interpreters/Cache/SLRUFileCachePriority.h +++ b/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; }