From dafa584fd10918ec322053a54fcb7f0f7c0ead9f Mon Sep 17 00:00:00 2001 From: mrambacher Date: Mon, 13 Sep 2021 08:45:13 -0700 Subject: [PATCH] Change the File System File Wrappers to std::unique_ptr (#8618) Summary: This allows the wrapper classes to own the wrapped object and eliminates confusion as to ownership. Previously, many classes implemented their own ownership solutions. Fixes https://github.com/facebook/rocksdb/issues/8606 Pull Request resolved: https://github.com/facebook/rocksdb/pull/8618 Reviewed By: pdillinger Differential Revision: D30136064 Pulled By: mrambacher fbshipit-source-id: d0bf471df8818dbb1770a86335fe98f761cca193 --- db/db_basic_test.cc | 6 +-- env/file_system_tracer.h | 66 ++++++++++++--------------- file/prefetch_test.cc | 6 +-- include/rocksdb/file_system.h | 64 ++++++++++++++++++++++++++ tools/simulated_hybrid_file_system.cc | 2 +- tools/simulated_hybrid_file_system.h | 6 +-- utilities/fault_injection_fs.cc | 2 +- utilities/fault_injection_fs.h | 8 ++-- 8 files changed, 107 insertions(+), 53 deletions(-) diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index a9e6922f9..9ebc0d708 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -3235,13 +3235,11 @@ INSTANTIATE_TEST_CASE_P(ParallelIO, DBBasicTestWithParallelIO, // Forward declaration class DeadlineFS; -class DeadlineRandomAccessFile : public FSRandomAccessFileWrapper { +class DeadlineRandomAccessFile : public FSRandomAccessFileOwnerWrapper { public: DeadlineRandomAccessFile(DeadlineFS& fs, std::unique_ptr& file) - : FSRandomAccessFileWrapper(file.get()), - fs_(fs), - file_(std::move(file)) {} + : FSRandomAccessFileOwnerWrapper(std::move(file)), fs_(fs) {} IOStatus Read(uint64_t offset, size_t len, const IOOptions& opts, Slice* result, char* scratch, diff --git a/env/file_system_tracer.h b/env/file_system_tracer.h index da87797d3..0c0b918c8 100644 --- a/env/file_system_tracer.h +++ b/env/file_system_tracer.h @@ -131,12 +131,12 @@ class FileSystemPtr { // for tracing. It overrides methods we are interested in tracing and extends // FSSequentialFileWrapper, which forwards all methods that are not explicitly // overridden. -class FSSequentialFileTracingWrapper : public FSSequentialFileWrapper { +class FSSequentialFileTracingWrapper : public FSSequentialFileOwnerWrapper { public: - FSSequentialFileTracingWrapper(FSSequentialFile* t, + FSSequentialFileTracingWrapper(std::unique_ptr&& t, std::shared_ptr io_tracer, const std::string& file_name) - : FSSequentialFileWrapper(t), + : FSSequentialFileOwnerWrapper(std::move(t)), io_tracer_(io_tracer), clock_(SystemClock::Default().get()), file_name_(file_name) {} @@ -169,9 +169,8 @@ class FSSequentialFilePtr { FSSequentialFilePtr(std::unique_ptr&& fs, const std::shared_ptr& io_tracer, const std::string& file_name) - : fs_(std::move(fs)), - io_tracer_(io_tracer), - fs_tracer_(fs_.get(), io_tracer_, + : io_tracer_(io_tracer), + fs_tracer_(std::move(fs), io_tracer_, file_name.substr(file_name.find_last_of("/\\") + 1) /* pass file name */) {} @@ -179,7 +178,7 @@ class FSSequentialFilePtr { if (io_tracer_ && io_tracer_->is_tracing_enabled()) { return const_cast(&fs_tracer_); } else { - return fs_.get(); + return fs_tracer_.target(); } } @@ -187,12 +186,11 @@ class FSSequentialFilePtr { if (io_tracer_ && io_tracer_->is_tracing_enabled()) { return const_cast(&fs_tracer_); } else { - return fs_.get(); + return fs_tracer_.target(); } } private: - std::unique_ptr fs_; std::shared_ptr io_tracer_; FSSequentialFileTracingWrapper fs_tracer_; }; @@ -203,12 +201,12 @@ class FSSequentialFilePtr { // binary format for tracing. It overrides methods we are interested in tracing // and extends FSRandomAccessFileWrapper, which forwards all methods that are // not explicitly overridden. -class FSRandomAccessFileTracingWrapper : public FSRandomAccessFileWrapper { +class FSRandomAccessFileTracingWrapper : public FSRandomAccessFileOwnerWrapper { public: - FSRandomAccessFileTracingWrapper(FSRandomAccessFile* t, + FSRandomAccessFileTracingWrapper(std::unique_ptr&& t, std::shared_ptr io_tracer, const std::string& file_name) - : FSRandomAccessFileWrapper(t), + : FSRandomAccessFileOwnerWrapper(std::move(t)), io_tracer_(io_tracer), clock_(SystemClock::Default().get()), file_name_(file_name) {} @@ -244,9 +242,8 @@ class FSRandomAccessFilePtr { FSRandomAccessFilePtr(std::unique_ptr&& fs, const std::shared_ptr& io_tracer, const std::string& file_name) - : fs_(std::move(fs)), - io_tracer_(io_tracer), - fs_tracer_(fs_.get(), io_tracer_, + : io_tracer_(io_tracer), + fs_tracer_(std::move(fs), io_tracer_, file_name.substr(file_name.find_last_of("/\\") + 1) /* pass file name */) {} @@ -254,7 +251,7 @@ class FSRandomAccessFilePtr { if (io_tracer_ && io_tracer_->is_tracing_enabled()) { return const_cast(&fs_tracer_); } else { - return fs_.get(); + return fs_tracer_.target(); } } @@ -262,12 +259,11 @@ class FSRandomAccessFilePtr { if (io_tracer_ && io_tracer_->is_tracing_enabled()) { return const_cast(&fs_tracer_); } else { - return fs_.get(); + return fs_tracer_.target(); } } private: - std::unique_ptr fs_; std::shared_ptr io_tracer_; FSRandomAccessFileTracingWrapper fs_tracer_; }; @@ -278,12 +274,12 @@ class FSRandomAccessFilePtr { // for tracing. It overrides methods we are interested in tracing and extends // FSWritableFileWrapper, which forwards all methods that are not explicitly // overridden. -class FSWritableFileTracingWrapper : public FSWritableFileWrapper { +class FSWritableFileTracingWrapper : public FSWritableFileOwnerWrapper { public: - FSWritableFileTracingWrapper(FSWritableFile* t, + FSWritableFileTracingWrapper(std::unique_ptr&& t, std::shared_ptr io_tracer, const std::string& file_name) - : FSWritableFileWrapper(t), + : FSWritableFileOwnerWrapper(std::move(t)), io_tracer_(io_tracer), clock_(SystemClock::Default().get()), file_name_(file_name) {} @@ -334,9 +330,9 @@ class FSWritableFilePtr { FSWritableFilePtr(std::unique_ptr&& fs, const std::shared_ptr& io_tracer, const std::string& file_name) - : fs_(std::move(fs)), io_tracer_(io_tracer) { + : io_tracer_(io_tracer) { fs_tracer_.reset(new FSWritableFileTracingWrapper( - fs_.get(), io_tracer_, + std::move(fs), io_tracer_, file_name.substr(file_name.find_last_of("/\\") + 1) /* pass file name */)); } @@ -345,26 +341,26 @@ class FSWritableFilePtr { if (io_tracer_ && io_tracer_->is_tracing_enabled()) { return fs_tracer_.get(); } else { - return fs_.get(); + return fs_tracer_->target(); } } FSWritableFile* get() const { if (io_tracer_ && io_tracer_->is_tracing_enabled()) { return fs_tracer_.get(); + } else if (fs_tracer_) { + return fs_tracer_->target(); } else { - return fs_.get(); + return nullptr; } } void reset() { - fs_.reset(); fs_tracer_.reset(); io_tracer_ = nullptr; } private: - std::unique_ptr fs_; std::shared_ptr io_tracer_; std::unique_ptr fs_tracer_; }; @@ -375,12 +371,12 @@ class FSWritableFilePtr { // for tracing. It overrides methods we are interested in tracing and extends // FSRandomRWFileWrapper, which forwards all methods that are not explicitly // overridden. -class FSRandomRWFileTracingWrapper : public FSRandomRWFileWrapper { +class FSRandomRWFileTracingWrapper : public FSRandomRWFileOwnerWrapper { public: - FSRandomRWFileTracingWrapper(FSRandomRWFile* t, + FSRandomRWFileTracingWrapper(std::unique_ptr&& t, std::shared_ptr io_tracer, const std::string& file_name) - : FSRandomRWFileWrapper(t), + : FSRandomRWFileOwnerWrapper(std::move(t)), io_tracer_(io_tracer), clock_(SystemClock::Default().get()), file_name_(file_name) {} @@ -419,9 +415,8 @@ class FSRandomRWFilePtr { FSRandomRWFilePtr(std::unique_ptr&& fs, std::shared_ptr io_tracer, const std::string& file_name) - : fs_(std::move(fs)), - io_tracer_(io_tracer), - fs_tracer_(fs_.get(), io_tracer_, + : io_tracer_(io_tracer), + fs_tracer_(std::move(fs), io_tracer_, file_name.substr(file_name.find_last_of("/\\") + 1) /* pass file name */) {} @@ -429,7 +424,7 @@ class FSRandomRWFilePtr { if (io_tracer_ && io_tracer_->is_tracing_enabled()) { return const_cast(&fs_tracer_); } else { - return fs_.get(); + return fs_tracer_.target(); } } @@ -437,12 +432,11 @@ class FSRandomRWFilePtr { if (io_tracer_ && io_tracer_->is_tracing_enabled()) { return const_cast(&fs_tracer_); } else { - return fs_.get(); + return fs_tracer_.target(); } } private: - std::unique_ptr fs_; std::shared_ptr io_tracer_; FSRandomRWFileTracingWrapper fs_tracer_; }; diff --git a/file/prefetch_test.cc b/file/prefetch_test.cc index ce4ac37ae..739e0e37b 100644 --- a/file/prefetch_test.cc +++ b/file/prefetch_test.cc @@ -10,12 +10,11 @@ namespace ROCKSDB_NAMESPACE { class MockFS; -class MockRandomAccessFile : public FSRandomAccessFileWrapper { +class MockRandomAccessFile : public FSRandomAccessFileOwnerWrapper { public: MockRandomAccessFile(std::unique_ptr& file, bool support_prefetch, std::atomic_int& prefetch_count) - : FSRandomAccessFileWrapper(file.get()), - file_(std::move(file)), + : FSRandomAccessFileOwnerWrapper(std::move(file)), support_prefetch_(support_prefetch), prefetch_count_(prefetch_count) {} @@ -30,7 +29,6 @@ class MockRandomAccessFile : public FSRandomAccessFileWrapper { } private: - std::unique_ptr file_; const bool support_prefetch_; std::atomic_int& prefetch_count_; }; diff --git a/include/rocksdb/file_system.h b/include/rocksdb/file_system.h index 025908e4f..2c73f720a 100644 --- a/include/rocksdb/file_system.h +++ b/include/rocksdb/file_system.h @@ -1340,6 +1340,8 @@ class FileSystemWrapper : public FileSystem { class FSSequentialFileWrapper : public FSSequentialFile { public: + // Creates a FileWrapper around the input File object and without + // taking ownership of the object explicit FSSequentialFileWrapper(FSSequentialFile* t) : target_(t) {} FSSequentialFile* target() const { return target_; } @@ -1366,8 +1368,21 @@ class FSSequentialFileWrapper : public FSSequentialFile { FSSequentialFile* target_; }; +class FSSequentialFileOwnerWrapper : public FSSequentialFileWrapper { + public: + // Creates a FileWrapper around the input File object and takes + // ownership of the object + explicit FSSequentialFileOwnerWrapper(std::unique_ptr&& t) + : FSSequentialFileWrapper(t.get()), guard_(std::move(t)) {} + + private: + std::unique_ptr guard_; +}; + class FSRandomAccessFileWrapper : public FSRandomAccessFile { public: + // Creates a FileWrapper around the input File object and without + // taking ownership of the object explicit FSRandomAccessFileWrapper(FSRandomAccessFile* t) : target_(t) {} FSRandomAccessFile* target() const { return target_; } @@ -1398,11 +1413,26 @@ class FSRandomAccessFileWrapper : public FSRandomAccessFile { } private: + std::unique_ptr guard_; FSRandomAccessFile* target_; }; +class FSRandomAccessFileOwnerWrapper : public FSRandomAccessFileWrapper { + public: + // Creates a FileWrapper around the input File object and takes + // ownership of the object + explicit FSRandomAccessFileOwnerWrapper( + std::unique_ptr&& t) + : FSRandomAccessFileWrapper(t.get()), guard_(std::move(t)) {} + + private: + std::unique_ptr guard_; +}; + class FSWritableFileWrapper : public FSWritableFile { public: + // Creates a FileWrapper around the input File object and without + // taking ownership of the object explicit FSWritableFileWrapper(FSWritableFile* t) : target_(t) {} FSWritableFile* target() const { return target_; } @@ -1500,8 +1530,21 @@ class FSWritableFileWrapper : public FSWritableFile { FSWritableFile* target_; }; +class FSWritableFileOwnerWrapper : public FSWritableFileWrapper { + public: + // Creates a FileWrapper around the input File object and takes + // ownership of the object + explicit FSWritableFileOwnerWrapper(std::unique_ptr&& t) + : FSWritableFileWrapper(t.get()), guard_(std::move(t)) {} + + private: + std::unique_ptr guard_; +}; + class FSRandomRWFileWrapper : public FSRandomRWFile { public: + // Creates a FileWrapper around the input File object and without + // taking ownership of the object explicit FSRandomRWFileWrapper(FSRandomRWFile* t) : target_(t) {} FSRandomRWFile* target() const { return target_; } @@ -1536,8 +1579,28 @@ class FSRandomRWFileWrapper : public FSRandomRWFile { FSRandomRWFile* target_; }; +class FSRandomRWFileOwnerWrapper : public FSRandomRWFileWrapper { + public: + // Creates a FileWrapper around the input File object and takes + // ownership of the object + explicit FSRandomRWFileOwnerWrapper(std::unique_ptr&& t) + : FSRandomRWFileWrapper(t.get()), guard_(std::move(t)) {} + + private: + std::unique_ptr guard_; +}; + class FSDirectoryWrapper : public FSDirectory { public: + // Creates a FileWrapper around the input File object and takes + // ownership of the object + explicit FSDirectoryWrapper(std::unique_ptr&& t) + : guard_(std::move(t)) { + target_ = guard_.get(); + } + + // Creates a FileWrapper around the input File object and without + // taking ownership of the object explicit FSDirectoryWrapper(FSDirectory* t) : target_(t) {} IOStatus Fsync(const IOOptions& options, IODebugContext* dbg) override { @@ -1548,6 +1611,7 @@ class FSDirectoryWrapper : public FSDirectory { } private: + std::unique_ptr guard_; FSDirectory* target_; }; diff --git a/tools/simulated_hybrid_file_system.cc b/tools/simulated_hybrid_file_system.cc index 59b4654be..7cb992152 100644 --- a/tools/simulated_hybrid_file_system.cc +++ b/tools/simulated_hybrid_file_system.cc @@ -81,7 +81,7 @@ IOStatus SimulatedHybridFileSystem::NewRandomAccessFile( } IOStatus s = target()->NewRandomAccessFile(fname, file_opts, result, dbg); result->reset( - new SimulatedHybridRaf(result->release(), rate_limiter_, temperature)); + new SimulatedHybridRaf(std::move(*result), rate_limiter_, temperature)); return s; } diff --git a/tools/simulated_hybrid_file_system.h b/tools/simulated_hybrid_file_system.h index a3d16c4e7..7b101b57a 100644 --- a/tools/simulated_hybrid_file_system.h +++ b/tools/simulated_hybrid_file_system.h @@ -59,12 +59,12 @@ class SimulatedHybridFileSystem : public FileSystemWrapper { // Simulated random access file that can control IOPs and latency to simulate // specific storage media -class SimulatedHybridRaf : public FSRandomAccessFileWrapper { +class SimulatedHybridRaf : public FSRandomAccessFileOwnerWrapper { public: - SimulatedHybridRaf(FSRandomAccessFile* t, + SimulatedHybridRaf(std::unique_ptr&& t, std::shared_ptr rate_limiter, Temperature temperature) - : FSRandomAccessFileWrapper(t), + : FSRandomAccessFileOwnerWrapper(std::move(t)), rate_limiter_(rate_limiter), temperature_(temperature) {} diff --git a/utilities/fault_injection_fs.cc b/utilities/fault_injection_fs.cc index 68a039709..d0d2260c1 100644 --- a/utilities/fault_injection_fs.cc +++ b/utilities/fault_injection_fs.cc @@ -544,7 +544,7 @@ IOStatus FaultInjectionTestFS::NewSequentialFile( } IOStatus io_s = target()->NewSequentialFile(fname, file_opts, result, dbg); if (io_s.ok()) { - result->reset(new TestFSSequentialFile(result->release(), this)); + result->reset(new TestFSSequentialFile(std::move(*result), this)); } return io_s; } diff --git a/utilities/fault_injection_fs.h b/utilities/fault_injection_fs.h index 62c938ca0..3238f1e23 100644 --- a/utilities/fault_injection_fs.h +++ b/utilities/fault_injection_fs.h @@ -150,10 +150,11 @@ class TestFSRandomAccessFile : public FSRandomAccessFile { FaultInjectionTestFS* fs_; }; -class TestFSSequentialFile : public FSSequentialFileWrapper { +class TestFSSequentialFile : public FSSequentialFileOwnerWrapper { public: - explicit TestFSSequentialFile(FSSequentialFile* f, FaultInjectionTestFS* fs) - : FSSequentialFileWrapper(f), target_guard_(f), fs_(fs) {} + explicit TestFSSequentialFile(std::unique_ptr&& f, + FaultInjectionTestFS* fs) + : FSSequentialFileOwnerWrapper(std::move(f)), fs_(fs) {} IOStatus Read(size_t n, const IOOptions& options, Slice* result, char* scratch, IODebugContext* dbg) override; IOStatus PositionedRead(uint64_t offset, size_t n, const IOOptions& options, @@ -161,7 +162,6 @@ class TestFSSequentialFile : public FSSequentialFileWrapper { IODebugContext* dbg) override; private: - std::unique_ptr target_guard_; FaultInjectionTestFS* fs_; };