Skip to content

Commit

Permalink
Change the File System File Wrappers to std::unique_ptr (facebook#8618)
Browse files Browse the repository at this point in the history 
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 facebook#8606

Pull Request resolved: facebook#8618

Reviewed By: pdillinger

Differential Revision: D30136064

Pulled By: mrambacher

fbshipit-source-id: d0bf471df8818dbb1770a86335fe98f761cca193
  • Loading branch information
@mrambacher @facebook-github-bot
mrambacher authored and facebook-github-bot committed Sep 13, 2021
1 parent 2a2b3e0 commit dafa584
Show file tree
Hide file tree
Showing 8 changed files with 107 additions and 53 deletions.
6 changes: 2 additions & 4 deletions db/db_basic_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<FSRandomAccessFile>& 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,
Expand Down
66 changes: 30 additions & 36 deletions env/file_system_tracer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<FSSequentialFile>&& t,
std::shared_ptr<IOTracer> 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) {}
Expand Down Expand Up @@ -169,30 +169,28 @@ class FSSequentialFilePtr {
FSSequentialFilePtr(std::unique_ptr<FSSequentialFile>&& fs,
const std::shared_ptr<IOTracer>& 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 */) {}

FSSequentialFile* operator->() const {
if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
return const_cast<FSSequentialFileTracingWrapper*>(&fs_tracer_);
} else {
return fs_.get();
return fs_tracer_.target();
}
}

FSSequentialFile* get() const {
if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
return const_cast<FSSequentialFileTracingWrapper*>(&fs_tracer_);
} else {
return fs_.get();
return fs_tracer_.target();
}
}

private:
std::unique_ptr<FSSequentialFile> fs_;
std::shared_ptr<IOTracer> io_tracer_;
FSSequentialFileTracingWrapper fs_tracer_;
};
Expand All @@ -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<FSRandomAccessFile>&& t,
std::shared_ptr<IOTracer> 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) {}
Expand Down Expand Up @@ -244,30 +242,28 @@ class FSRandomAccessFilePtr {
FSRandomAccessFilePtr(std::unique_ptr<FSRandomAccessFile>&& fs,
const std::shared_ptr<IOTracer>& 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 */) {}

FSRandomAccessFile* operator->() const {
if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
return const_cast<FSRandomAccessFileTracingWrapper*>(&fs_tracer_);
} else {
return fs_.get();
return fs_tracer_.target();
}
}

FSRandomAccessFile* get() const {
if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
return const_cast<FSRandomAccessFileTracingWrapper*>(&fs_tracer_);
} else {
return fs_.get();
return fs_tracer_.target();
}
}

private:
std::unique_ptr<FSRandomAccessFile> fs_;
std::shared_ptr<IOTracer> io_tracer_;
FSRandomAccessFileTracingWrapper fs_tracer_;
};
Expand All @@ -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<FSWritableFile>&& t,
std::shared_ptr<IOTracer> 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) {}
Expand Down Expand Up @@ -334,9 +330,9 @@ class FSWritableFilePtr {
FSWritableFilePtr(std::unique_ptr<FSWritableFile>&& fs,
const std::shared_ptr<IOTracer>& 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 */));
}
Expand All @@ -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<FSWritableFile> fs_;
std::shared_ptr<IOTracer> io_tracer_;
std::unique_ptr<FSWritableFileTracingWrapper> fs_tracer_;
};
Expand All @@ -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<FSRandomRWFile>&& t,
std::shared_ptr<IOTracer> 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) {}
Expand Down Expand Up @@ -419,30 +415,28 @@ class FSRandomRWFilePtr {
FSRandomRWFilePtr(std::unique_ptr<FSRandomRWFile>&& fs,
std::shared_ptr<IOTracer> 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 */) {}

FSRandomRWFile* operator->() const {
if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
return const_cast<FSRandomRWFileTracingWrapper*>(&fs_tracer_);
} else {
return fs_.get();
return fs_tracer_.target();
}
}

FSRandomRWFile* get() const {
if (io_tracer_ && io_tracer_->is_tracing_enabled()) {
return const_cast<FSRandomRWFileTracingWrapper*>(&fs_tracer_);
} else {
return fs_.get();
return fs_tracer_.target();
}
}

private:
std::unique_ptr<FSRandomRWFile> fs_;
std::shared_ptr<IOTracer> io_tracer_;
FSRandomRWFileTracingWrapper fs_tracer_;
};
Expand Down
6 changes: 2 additions & 4 deletions file/prefetch_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@ namespace ROCKSDB_NAMESPACE {

class MockFS;

class MockRandomAccessFile : public FSRandomAccessFileWrapper {
class MockRandomAccessFile : public FSRandomAccessFileOwnerWrapper {
public:
MockRandomAccessFile(std::unique_ptr<FSRandomAccessFile>& 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) {}

Expand All @@ -30,7 +29,6 @@ class MockRandomAccessFile : public FSRandomAccessFileWrapper {
}

private:
std::unique_ptr<FSRandomAccessFile> file_;
const bool support_prefetch_;
std::atomic_int& prefetch_count_;
};
Expand Down
64 changes: 64 additions & 0 deletions include/rocksdb/file_system.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_; }
Expand All @@ -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<FSSequentialFile>&& t)
: FSSequentialFileWrapper(t.get()), guard_(std::move(t)) {}

private:
std::unique_ptr<FSSequentialFile> 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_; }
Expand Down Expand Up @@ -1398,11 +1413,26 @@ class FSRandomAccessFileWrapper : public FSRandomAccessFile {
}

private:
std::unique_ptr<FSRandomAccessFile> 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<FSRandomAccessFile>&& t)
: FSRandomAccessFileWrapper(t.get()), guard_(std::move(t)) {}

private:
std::unique_ptr<FSRandomAccessFile> 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_; }
Expand Down Expand Up @@ -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<FSWritableFile>&& t)
: FSWritableFileWrapper(t.get()), guard_(std::move(t)) {}

private:
std::unique_ptr<FSWritableFile> 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_; }
Expand Down Expand Up @@ -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<FSRandomRWFile>&& t)
: FSRandomRWFileWrapper(t.get()), guard_(std::move(t)) {}

private:
std::unique_ptr<FSRandomRWFile> 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<FSDirectory>&& 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 {
Expand All @@ -1548,6 +1611,7 @@ class FSDirectoryWrapper : public FSDirectory {
}

private:
std::unique_ptr<FSDirectory> guard_;
FSDirectory* target_;
};

Expand Down
2 changes: 1 addition & 1 deletion tools/simulated_hybrid_file_system.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
6 changes: 3 additions & 3 deletions tools/simulated_hybrid_file_system.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<FSRandomAccessFile>&& t,
std::shared_ptr<RateLimiter> rate_limiter,
Temperature temperature)
: FSRandomAccessFileWrapper(t),
: FSRandomAccessFileOwnerWrapper(std::move(t)),
rate_limiter_(rate_limiter),
temperature_(temperature) {}

Expand Down
Loading

0 comments on commit dafa584

Please sign in to comment.