From ad34ff24dc0fc59bd30b71436c8e41344fa4fb1f Mon Sep 17 00:00:00 2001 From: kssenii Date: Tue, 31 Jan 2023 16:38:35 +0100 Subject: [PATCH] Fix memory leak --- src/Interpreters/Cache/FileCache.cpp | 21 +++++++++---------- src/Interpreters/Cache/FileCache.h | 14 ++++++------- src/Interpreters/Cache/FileSegment.h | 2 ++ .../Cache/LockedFileCachePriority.h | 1 + 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/Interpreters/Cache/FileCache.cpp b/src/Interpreters/Cache/FileCache.cpp index 03700c5780c..38601392aac 100644 --- a/src/Interpreters/Cache/FileCache.cpp +++ b/src/Interpreters/Cache/FileCache.cpp @@ -412,7 +412,7 @@ KeyTransactionPtr FileCache::createKeyTransaction(const Key & key, KeyNotFoundPo else if (!it->second || !it->second->guard) throw Exception(ErrorCodes::LOGICAL_ERROR, "Trash in metadata"); - return std::make_unique(key, it->second, cleanup_keys_metadata_queue, this); + return std::make_unique(key, *it->second, cleanup_keys_metadata_queue, this); } FileSegmentsHolderPtr FileCache::set(const Key & key, size_t offset, size_t size, const CreateFileSegmentSettings & settings) @@ -553,8 +553,8 @@ FileCache::KeyMetadata::iterator FileCache::addCell( result_state = state; } - auto file_segment = std::make_shared( - offset, size, key, key_transaction.getCreator(), this, result_state, settings); + auto creator = key_transaction.getCreator(); + auto file_segment = std::make_shared(offset, size, key, std::move(creator), this, result_state, settings); std::optional locked_queue(lock ? LockedCachePriority(*lock, *main_priority) : std::optional{}); @@ -845,13 +845,12 @@ void FileCache::removeAllReleasable() KeyTransaction::KeyTransaction( const Key & key_, - FileCache::KeyMetadataPtr offsets_, + FileCache::KeyMetadata & offsets_, KeysQueuePtr cleanup_keys_metadata_queue_, const FileCache * cache_) : key(key_) , cache(cache_) - , guard(offsets_->guard) - , lock(guard->lock()) + , lock(offsets_.guard->lock()) , offsets(offsets_) , cleanup_keys_metadata_queue(cleanup_keys_metadata_queue_) , log(&Poco::Logger::get("KeyTransaction")) @@ -885,7 +884,7 @@ void KeyTransaction::remove( log, "Remove from cache. Key: {}, offset: {}", key.toString(), offset); - auto * cell = offsets->get(offset); + auto * cell = offsets.get(offset); if (cell->queue_iterator) LockedCachePriorityIterator(cache_lock, cell->queue_iterator).remove(); @@ -893,7 +892,7 @@ void KeyTransaction::remove( const auto cache_file_path = cell->file_segment->getPathInLocalCache(); cell->file_segment->detach(segment_lock, *this); - offsets->erase(offset); + offsets.erase(offset); if (fs::exists(cache_file_path)) { @@ -919,7 +918,7 @@ void KeyTransaction::cleanupKeyDirectory() const return; /// Someone might still need this directory. - if (!offsets->empty()) + if (!offsets.empty()) return; /// Now `offsets` empty and the key lock is still locked. @@ -928,7 +927,7 @@ void KeyTransaction::cleanupKeyDirectory() const fs::path key_path = cache->getPathInLocalCache(key); if (fs::exists(key_path)) { - offsets->created_base_directory = false; + offsets.created_base_directory = false; fs::remove_all(key_path); } cleanup_keys_metadata_queue->add(key); @@ -1066,7 +1065,7 @@ void KeyTransaction::reduceSizeToDownloaded( * because of no space left in cache, we need to be able to cut cell's size to downloaded_size. */ - auto * cell = offsets->get(offset); + auto * cell = offsets.get(offset); const auto & file_segment = cell->file_segment; size_t downloaded_size = file_segment->downloaded_size; diff --git a/src/Interpreters/Cache/FileCache.h b/src/Interpreters/Cache/FileCache.h index 37b3c6b86eb..86c8bd63800 100644 --- a/src/Interpreters/Cache/FileCache.h +++ b/src/Interpreters/Cache/FileCache.h @@ -384,7 +384,7 @@ struct KeyTransactionCreator { KeyTransactionCreator( const FileCacheKey & key_, - FileCache::KeyMetadataPtr offsets_, + FileCache::KeyMetadata & offsets_, KeysQueuePtr cleanup_keys_metadata_queue_, const FileCache * cache_) : key(key_), offsets(offsets_), cleanup_keys_metadata_queue(cleanup_keys_metadata_queue_), cache(cache_) {} @@ -392,7 +392,7 @@ struct KeyTransactionCreator KeyTransactionPtr create(); FileCacheKey key; - FileCache::KeyMetadataPtr offsets; + FileCache::KeyMetadata & offsets; KeysQueuePtr cleanup_keys_metadata_queue; const FileCache * cache; }; @@ -404,7 +404,7 @@ struct KeyTransaction : private boost::noncopyable KeyTransaction( const Key & key_, - FileCache::KeyMetadataPtr offsets_, + FileCache::KeyMetadata & offsets_, KeysQueuePtr cleanup_keys_metadata_queue_, const FileCache * cache_); @@ -420,8 +420,8 @@ struct KeyTransaction : private boost::noncopyable bool isLastHolder(size_t offset); - FileCache::KeyMetadata & getOffsets() { return *offsets; } - const FileCache::KeyMetadata & getOffsets() const { return *offsets; } + FileCache::KeyMetadata & getOffsets() { return offsets; } + const FileCache::KeyMetadata & getOffsets() const { return offsets; } std::vector delete_offsets; @@ -432,10 +432,8 @@ private: Key key; const FileCache * cache; - KeyGuardPtr guard; KeyGuard::Lock lock; - - FileCache::KeyMetadataPtr offsets; + FileCache::KeyMetadata & offsets; KeysQueuePtr cleanup_keys_metadata_queue; Poco::Logger * log; diff --git a/src/Interpreters/Cache/FileSegment.h b/src/Interpreters/Cache/FileSegment.h index 74421f78b94..4d770971082 100644 --- a/src/Interpreters/Cache/FileSegment.h +++ b/src/Interpreters/Cache/FileSegment.h @@ -129,6 +129,8 @@ public: State download_state_, const CreateFileSegmentSettings & create_settings); + ~FileSegment() = default; + State state() const; static String stateToString(FileSegment::State state); diff --git a/src/Interpreters/Cache/LockedFileCachePriority.h b/src/Interpreters/Cache/LockedFileCachePriority.h index a5742b07a17..ee38a046603 100644 --- a/src/Interpreters/Cache/LockedFileCachePriority.h +++ b/src/Interpreters/Cache/LockedFileCachePriority.h @@ -1,3 +1,4 @@ +#pragma once #include