From ce723ec32dcc128b1a39057d9f475f71b82e2446 Mon Sep 17 00:00:00 2001 From: kssenii Date: Thu, 13 Apr 2023 14:44:06 +0200 Subject: [PATCH] Fix style check, better priority->iterate --- .../IO/CachedOnDiskReadBufferFromFile.cpp | 2 +- src/Interpreters/Cache/FileCache.cpp | 52 +++++++------------ src/Interpreters/Cache/FileCache.h | 4 +- .../Cache/FileCache_fwd_internal.h | 25 +++++++++ src/Interpreters/Cache/FileSegment.cpp | 3 +- src/Interpreters/Cache/FileSegment.h | 12 +---- src/Interpreters/Cache/IFileCachePriority.h | 14 +---- .../Cache/LRUFileCachePriority.cpp | 17 +++++- src/Interpreters/Cache/Metadata.h | 5 +- src/Interpreters/Context.cpp | 1 - 10 files changed, 69 insertions(+), 66 deletions(-) create mode 100644 src/Interpreters/Cache/FileCache_fwd_internal.h diff --git a/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp b/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp index 528e2674383..7827e537708 100644 --- a/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp +++ b/src/Disks/IO/CachedOnDiskReadBufferFromFile.cpp @@ -632,7 +632,7 @@ void CachedOnDiskReadBufferFromFile::predownload(FileSegment & file_segment) bytes_to_predownload = 0; file_segment.setBroken(); - chassert(file_segment->state() == FileSegment::State::PARTIALLY_DOWNLOADED_NO_CONTINUATION); + chassert(file_segment.state() == FileSegment::State::PARTIALLY_DOWNLOADED_NO_CONTINUATION); LOG_TEST(log, "Bypassing cache because for {}", file_segment.getInfoForLog()); diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 82128e681e4..7bdb97e527d 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -315,9 +315,9 @@ void FileCache::fillHolesWithEmptyFileSegments( } else { - auto splitted = splitRangeIntoFileSegments( + auto split = splitRangeIntoFileSegments( locked_key, current_pos, hole_size, FileSegment::State::EMPTY, settings); - file_segments.splice(it, std::move(splitted)); + file_segments.splice(it, std::move(split)); } current_pos = segment_range.right + 1; @@ -342,9 +342,9 @@ void FileCache::fillHolesWithEmptyFileSegments( } else { - auto splitted = splitRangeIntoFileSegments( + auto split = splitRangeIntoFileSegments( locked_key, current_pos, hole_size, FileSegment::State::EMPTY, settings); - file_segments.splice(file_segments.end(), std::move(splitted)); + file_segments.splice(file_segments.end(), std::move(split)); } } } @@ -556,29 +556,25 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) size_t removed_size = 0; std::unordered_map to_delete; - auto iterate_func = [&](const PriorityEntry & entry, LockedKey & locked_key) + auto iterate_func = [&](LockedKey & locked_key, FileSegmentMetadataPtr segment_metadata) { - auto file_segment_metadata = locked_key.tryGetByOffset(entry.offset); - if (!file_segment_metadata) - return PriorityIterationResult::REMOVE_AND_CONTINUE; - - chassert(file_segment_metadata->file_segment->getQueueIterator()); - const bool is_persistent = allow_persistent_files && file_segment_metadata->file_segment->isPersistent(); - const bool releasable = file_segment_metadata->releasable() && !is_persistent; + chassert(segment_metadata->file_segment->getQueueIterator()); + const bool is_persistent = allow_persistent_files && segment_metadata->file_segment->isPersistent(); + const bool releasable = segment_metadata->releasable() && !is_persistent; if (releasable) { - removed_size += entry.size; + removed_size += segment_metadata->size(); --queue_size; - auto segment = file_segment_metadata->file_segment; + 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(file_segment_metadata); + it->second.add(segment_metadata); return PriorityIterationResult::CONTINUE; } @@ -598,8 +594,8 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) }; query_priority->iterate( - [&](const auto & entry, LockedKey & locked_key) - { return is_query_priority_overflow() ? iterate_func(entry, locked_key) : PriorityIterationResult::BREAK; }, + [&](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()) @@ -615,8 +611,8 @@ bool FileCache::tryReserve(FileSegment & file_segment, size_t size) }; main_priority->iterate( - [&](const auto & entry, LockedKey & locked_key) - { return is_main_priority_overflow() ? iterate_func(entry, locked_key) : PriorityIterationResult::BREAK; }, + [&](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()) @@ -696,15 +692,11 @@ void FileCache::removeAllReleasable() auto lock = cache_guard.lock(); - main_priority->iterate([&](const PriorityEntry & entry, LockedKey & locked_key) + main_priority->iterate([&](LockedKey & locked_key, FileSegmentMetadataPtr segment_metadata) { - auto file_segment_metadata = locked_key.tryGetByOffset(entry.offset); - if (!file_segment_metadata) - return PriorityIterationResult::REMOVE_AND_CONTINUE; - - if (file_segment_metadata->releasable()) + if (segment_metadata->releasable()) { - auto file_segment = file_segment_metadata->file_segment; + auto file_segment = segment_metadata->file_segment; locked_key.removeFileSegment(file_segment->offset(), file_segment->lock()); return PriorityIterationResult::REMOVE_AND_CONTINUE; } @@ -933,13 +925,9 @@ FileSegmentsHolderPtr FileCache::dumpQueue() assertInitialized(); FileSegments file_segments; - main_priority->iterate([&](const PriorityEntry & entry, LockedKey & locked_key) + main_priority->iterate([&](LockedKey &, FileSegmentMetadataPtr segment_metadata) { - auto file_segment_metadata = locked_key.tryGetByOffset(entry.offset); - if (!file_segment_metadata) - return PriorityIterationResult::REMOVE_AND_CONTINUE; - - file_segments.push_back(FileSegment::getSnapshot(file_segment_metadata->file_segment)); + file_segments.push_back(FileSegment::getSnapshot(segment_metadata->file_segment)); return PriorityIterationResult::CONTINUE; }, cache_guard.lock()); diff --git a/src/Interpreters/Cache/FileCache.h b/src/Interpreters/Cache/FileCache.h index 4e01751daaf..49c5dade3bf 100644 --- a/src/Interpreters/Cache/FileCache.h +++ b/src/Interpreters/Cache/FileCache.h @@ -18,15 +18,13 @@ #include #include #include +#include #include namespace DB { -struct LockedKey; -using LockedKeyPtr = std::shared_ptr; - namespace ErrorCodes { extern const int BAD_ARGUMENTS; diff --git a/src/Interpreters/Cache/FileCache_fwd_internal.h b/src/Interpreters/Cache/FileCache_fwd_internal.h new file mode 100644 index 00000000000..8c59f1f78b7 --- /dev/null +++ b/src/Interpreters/Cache/FileCache_fwd_internal.h @@ -0,0 +1,25 @@ +#include + +namespace DB +{ + +class FileCache; +using FileCachePtr = std::shared_ptr; + +class IFileCachePriority; +using FileCachePriorityPtr = std::unique_ptr; + +class FileSegment; +using FileSegmentPtr = std::shared_ptr; +using FileSegments = std::list; + +struct FileSegmentMetadata; +using FileSegmentMetadataPtr = std::shared_ptr; + +struct LockedKey; +using LockedKeyPtr = std::shared_ptr; + +struct KeyMetadata; +using KeyMetadataPtr = std::shared_ptr; + +} diff --git a/src/Interpreters/Cache/FileSegment.cpp b/src/Interpreters/Cache/FileSegment.cpp index 77749ed3233..63c666d04c5 100644 --- a/src/Interpreters/Cache/FileSegment.cpp +++ b/src/Interpreters/Cache/FileSegment.cpp @@ -380,10 +380,11 @@ FileSegment::State FileSegment::wait(size_t offset) chassert(!getDownloaderUnlocked(lock).empty()); chassert(!isDownloaderUnlocked(lock)); - cv.wait_for(lock, std::chrono::seconds(60), [&, this]() + [[maybe_unused]] auto ok = cv.wait_for(lock, std::chrono::seconds(60), [&, this]() { return download_state != State::DOWNLOADING || offset < getCurrentWriteOffset(true); }); + chassert(ok); } return download_state; diff --git a/src/Interpreters/Cache/FileSegment.h b/src/Interpreters/Cache/FileSegment.h index 67432a1de01..6defe969ba4 100644 --- a/src/Interpreters/Cache/FileSegment.h +++ b/src/Interpreters/Cache/FileSegment.h @@ -11,7 +11,7 @@ #include #include #include -#include +#include #include @@ -25,18 +25,8 @@ extern const Metric CacheFileSegments; namespace DB { -class FileCache; class ReadBufferFromFileBase; -class FileSegment; -using FileSegmentPtr = std::shared_ptr; -using FileSegments = std::list; -struct FileSegmentMetadata; -struct LockedKey; -using LockedKeyPtr = std::shared_ptr; -struct KeyMetadata; -using KeyMetadataPtr = std::shared_ptr; - /* * FileSegmentKind is used to specify the eviction policy for file segments. */ diff --git a/src/Interpreters/Cache/IFileCachePriority.h b/src/Interpreters/Cache/IFileCachePriority.h index 31a23e137ca..748089fbd1b 100644 --- a/src/Interpreters/Cache/IFileCachePriority.h +++ b/src/Interpreters/Cache/IFileCachePriority.h @@ -6,21 +6,11 @@ #include #include #include +#include namespace DB { -class IFileCachePriority; -using FileCachePriorityPtr = std::unique_ptr; -struct KeyMetadata; -using KeyMetadataPtr = std::shared_ptr; -struct LockedKey; - -namespace ErrorCodes -{ - extern const int LOGICAL_ERROR; -} - /// IFileCachePriority is used to maintain the priority of cached data. class IFileCachePriority : private boost::noncopyable { @@ -71,7 +61,7 @@ public: CONTINUE, REMOVE_AND_CONTINUE, }; - using IterateFunc = std::function; + using IterateFunc = std::function; IFileCachePriority(size_t max_size_, size_t max_elements_) : max_size(max_size_), max_elements(max_elements_) {} diff --git a/src/Interpreters/Cache/LRUFileCachePriority.cpp b/src/Interpreters/Cache/LRUFileCachePriority.cpp index 131db6714c3..9a0e2d8fb05 100644 --- a/src/Interpreters/Cache/LRUFileCachePriority.cpp +++ b/src/Interpreters/Cache/LRUFileCachePriority.cpp @@ -102,7 +102,22 @@ void LRUFileCachePriority::iterate(IterateFunc && func, const CacheGuard::Lock & continue; } - auto result = func(*it, *locked_key); + auto metadata = locked_key->tryGetByOffset(it->offset); + if (!metadata) + { + it = remove(it); + continue; + } + + if (metadata->size() != it->size) + { + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "Mismatch of file segment size in file segment metadata and priority queue: {} != {}", + it->size, metadata->size()); + } + + auto result = func(*locked_key, metadata); switch (result) { case IterationResult::BREAK: diff --git a/src/Interpreters/Cache/Metadata.h b/src/Interpreters/Cache/Metadata.h index 54dde8c9aee..cb2c68d929c 100644 --- a/src/Interpreters/Cache/Metadata.h +++ b/src/Interpreters/Cache/Metadata.h @@ -4,13 +4,10 @@ #include #include #include +#include namespace DB { -class FileSegment; -using FileSegmentPtr = std::shared_ptr; -struct LockedKey; -using LockedKeyPtr = std::shared_ptr; struct CleanupQueue; using CleanupQueuePtr = std::shared_ptr; diff --git a/src/Interpreters/Context.cpp b/src/Interpreters/Context.cpp index 49b50ce94b1..ba250ed3e5b 100644 --- a/src/Interpreters/Context.cpp +++ b/src/Interpreters/Context.cpp @@ -19,7 +19,6 @@ #include #include #include -#include #include #include #include