Skip to content

Commit

Permalink
Use SystemClock* instead of std::shared_ptr<SystemClock> in lower lev…
Browse files Browse the repository at this point in the history 
…el routines (facebook#8033)

Summary:
For performance purposes, the lower level routines were changed to use a SystemClock* instead of a std::shared_ptr<SystemClock>.  The shared ptr has some performance degradation on certain hardware classes.

For most of the system, there is no risk of the pointer being deleted/invalid because the shared_ptr will be stored elsewhere.  For example, the ImmutableDBOptions stores the Env which has a std::shared_ptr<SystemClock> in it.  The SystemClock* within the ImmutableDBOptions is essentially a "short cut" to gain access to this constant resource.

There were a few classes (PeriodicWorkScheduler?) where the "short cut" property did not hold.  In those cases, the shared pointer was preserved.

Using db_bench readrandom perf_level=3 on my EC2 box, this change performed as well or better than 6.17:

6.17: readrandom   :      28.046 micros/op 854902 ops/sec;   61.3 MB/s (355999 of 355999 found)
6.18: readrandom   :      32.615 micros/op 735306 ops/sec;   52.7 MB/s (290999 of 290999 found)
PR: readrandom   :      27.500 micros/op 871909 ops/sec;   62.5 MB/s (367999 of 367999 found)

(Note that the times for 6.18 are prior to revert of the SystemClock).

Pull Request resolved: facebook#8033

Reviewed By: pdillinger

Differential Revision: D27014563

Pulled By: mrambacher

fbshipit-source-id: ad0459eba03182e454391b5926bf5cdd45657b67
  • Loading branch information
@mrambacher @facebook-github-bot
mrambacher authored and facebook-github-bot committed Mar 15, 2021
1 parent b8f40f7 commit 3dff28c
Show file tree
Hide file tree
Showing 130 changed files with 621 additions and 580 deletions.
25 changes: 12 additions & 13 deletions db/blob/blob_file_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
namespace ROCKSDB_NAMESPACE {

BlobFileBuilder::BlobFileBuilder(
VersionSet* versions, Env* env, FileSystem* fs,
VersionSet* versions, FileSystem* fs,
const ImmutableCFOptions* immutable_cf_options,
const MutableCFOptions* mutable_cf_options, const FileOptions* file_options,
int job_id, uint32_t column_family_id,
Expand All @@ -36,14 +36,14 @@ BlobFileBuilder::BlobFileBuilder(
const std::shared_ptr<IOTracer>& io_tracer,
std::vector<std::string>* blob_file_paths,
std::vector<BlobFileAddition>* blob_file_additions)
: BlobFileBuilder([versions]() { return versions->NewFileNumber(); }, env,
fs, immutable_cf_options, mutable_cf_options,
file_options, job_id, column_family_id,
column_family_name, io_priority, write_hint, io_tracer,
blob_file_paths, blob_file_additions) {}
: BlobFileBuilder([versions]() { return versions->NewFileNumber(); }, fs,
immutable_cf_options, mutable_cf_options, file_options,
job_id, column_family_id, column_family_name, io_priority,
write_hint, io_tracer, blob_file_paths,
blob_file_additions) {}

BlobFileBuilder::BlobFileBuilder(
std::function<uint64_t()> file_number_generator, Env* env, FileSystem* fs,
std::function<uint64_t()> file_number_generator, FileSystem* fs,
const ImmutableCFOptions* immutable_cf_options,
const MutableCFOptions* mutable_cf_options, const FileOptions* file_options,
int job_id, uint32_t column_family_id,
Expand All @@ -70,15 +70,13 @@ BlobFileBuilder::BlobFileBuilder(
blob_count_(0),
blob_bytes_(0) {
assert(file_number_generator_);
assert(env);
assert(fs_);
assert(immutable_cf_options_);
assert(file_options_);
assert(blob_file_paths_);
assert(blob_file_paths_->empty());
assert(blob_file_additions_);
assert(blob_file_additions_->empty());
clock_ = env->GetSystemClock();
}

BlobFileBuilder::~BlobFileBuilder() = default;
Expand Down Expand Up @@ -185,16 +183,17 @@ Status BlobFileBuilder::OpenBlobFileIfNeeded() {
FileTypeSet tmp_set = immutable_cf_options_->checksum_handoff_file_types;
Statistics* const statistics = immutable_cf_options_->statistics;
std::unique_ptr<WritableFileWriter> file_writer(new WritableFileWriter(
std::move(file), blob_file_paths_->back(), *file_options_, clock_,
io_tracer_, statistics, immutable_cf_options_->listeners,
std::move(file), blob_file_paths_->back(), *file_options_,
immutable_cf_options_->clock, io_tracer_, statistics,
immutable_cf_options_->listeners,
immutable_cf_options_->file_checksum_gen_factory,
tmp_set.Contains(FileType::kBlobFile)));

constexpr bool do_flush = false;

std::unique_ptr<BlobLogWriter> blob_log_writer(new BlobLogWriter(
std::move(file_writer), clock_, statistics, blob_file_number,
immutable_cf_options_->use_fsync, do_flush));
std::move(file_writer), immutable_cf_options_->clock, statistics,
blob_file_number, immutable_cf_options_->use_fsync, do_flush));

constexpr bool has_ttl = false;
constexpr ExpirationRange expiration_range;
Expand Down
5 changes: 2 additions & 3 deletions db/blob/blob_file_builder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class IOTracer;

class BlobFileBuilder {
public:
BlobFileBuilder(VersionSet* versions, Env* env, FileSystem* fs,
BlobFileBuilder(VersionSet* versions, FileSystem* fs,
const ImmutableCFOptions* immutable_cf_options,
const MutableCFOptions* mutable_cf_options,
const FileOptions* file_options, int job_id,
Expand All @@ -42,7 +42,7 @@ class BlobFileBuilder {
std::vector<std::string>* blob_file_paths,
std::vector<BlobFileAddition>* blob_file_additions);

BlobFileBuilder(std::function<uint64_t()> file_number_generator, Env* env,
BlobFileBuilder(std::function<uint64_t()> file_number_generator,
FileSystem* fs,
const ImmutableCFOptions* immutable_cf_options,
const MutableCFOptions* mutable_cf_options,
Expand Down Expand Up @@ -74,7 +74,6 @@ class BlobFileBuilder {

std::function<uint64_t()> file_number_generator_;
FileSystem* fs_;
std::shared_ptr<SystemClock> clock_;
const ImmutableCFOptions* immutable_cf_options_;
uint64_t min_blob_size_;
uint64_t blob_file_size_;
Expand Down
81 changes: 43 additions & 38 deletions db/blob/blob_file_builder_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class BlobFileBuilderTest : public testing::Test {
protected:
BlobFileBuilderTest() : mock_env_(Env::Default()) {
fs_ = mock_env_.GetFileSystem().get();
clock_ = mock_env_.GetSystemClock();
clock_ = mock_env_.GetSystemClock().get();
}

void VerifyBlobFile(uint64_t blob_file_number,
Expand Down Expand Up @@ -110,7 +110,7 @@ class BlobFileBuilderTest : public testing::Test {

MockEnv mock_env_;
FileSystem* fs_;
std::shared_ptr<SystemClock> clock_;
SystemClock* clock_;
FileOptions file_options_;
};

Expand All @@ -127,6 +127,7 @@ TEST_F(BlobFileBuilderTest, BuildAndCheckOneFile) {
"BlobFileBuilderTest_BuildAndCheckOneFile"),
0);
options.enable_blob_files = true;
options.env = &mock_env_;

ImmutableCFOptions immutable_cf_options(options);
MutableCFOptions mutable_cf_options(options);
Expand All @@ -140,11 +141,11 @@ TEST_F(BlobFileBuilderTest, BuildAndCheckOneFile) {
std::vector<std::string> blob_file_paths;
std::vector<BlobFileAddition> blob_file_additions;

BlobFileBuilder builder(
TestFileNumberGenerator(), &mock_env_, fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id, column_family_id,
column_family_name, io_priority, write_hint, nullptr /*IOTracer*/,
&blob_file_paths, &blob_file_additions);
BlobFileBuilder builder(TestFileNumberGenerator(), fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id,
column_family_id, column_family_name, io_priority,
write_hint, nullptr /*IOTracer*/, &blob_file_paths,
&blob_file_additions);

std::vector<std::pair<std::string, std::string>> expected_key_value_pairs(
number_of_blobs);
Expand Down Expand Up @@ -210,6 +211,7 @@ TEST_F(BlobFileBuilderTest, BuildAndCheckMultipleFiles) {
0);
options.enable_blob_files = true;
options.blob_file_size = value_size;
options.env = &mock_env_;

ImmutableCFOptions immutable_cf_options(options);
MutableCFOptions mutable_cf_options(options);
Expand All @@ -223,11 +225,11 @@ TEST_F(BlobFileBuilderTest, BuildAndCheckMultipleFiles) {
std::vector<std::string> blob_file_paths;
std::vector<BlobFileAddition> blob_file_additions;

BlobFileBuilder builder(
TestFileNumberGenerator(), &mock_env_, fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id, column_family_id,
column_family_name, io_priority, write_hint, nullptr /*IOTracer*/,
&blob_file_paths, &blob_file_additions);
BlobFileBuilder builder(TestFileNumberGenerator(), fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id,
column_family_id, column_family_name, io_priority,
write_hint, nullptr /*IOTracer*/, &blob_file_paths,
&blob_file_additions);

std::vector<std::pair<std::string, std::string>> expected_key_value_pairs(
number_of_blobs);
Expand Down Expand Up @@ -295,6 +297,7 @@ TEST_F(BlobFileBuilderTest, InlinedValues) {
0);
options.enable_blob_files = true;
options.min_blob_size = 1024;
options.env = &mock_env_;

ImmutableCFOptions immutable_cf_options(options);
MutableCFOptions mutable_cf_options(options);
Expand All @@ -308,11 +311,11 @@ TEST_F(BlobFileBuilderTest, InlinedValues) {
std::vector<std::string> blob_file_paths;
std::vector<BlobFileAddition> blob_file_additions;

BlobFileBuilder builder(
TestFileNumberGenerator(), &mock_env_, fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id, column_family_id,
column_family_name, io_priority, write_hint, nullptr /*IOTracer*/,
&blob_file_paths, &blob_file_additions);
BlobFileBuilder builder(TestFileNumberGenerator(), fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id,
column_family_id, column_family_name, io_priority,
write_hint, nullptr /*IOTracer*/, &blob_file_paths,
&blob_file_additions);

for (size_t i = 0; i < number_of_blobs; ++i) {
const std::string key = std::to_string(i);
Expand Down Expand Up @@ -347,6 +350,7 @@ TEST_F(BlobFileBuilderTest, Compression) {
test::PerThreadDBPath(&mock_env_, "BlobFileBuilderTest_Compression"), 0);
options.enable_blob_files = true;
options.blob_compression_type = kSnappyCompression;
options.env = &mock_env_;

ImmutableCFOptions immutable_cf_options(options);
MutableCFOptions mutable_cf_options(options);
Expand All @@ -360,11 +364,11 @@ TEST_F(BlobFileBuilderTest, Compression) {
std::vector<std::string> blob_file_paths;
std::vector<BlobFileAddition> blob_file_additions;

BlobFileBuilder builder(
TestFileNumberGenerator(), &mock_env_, fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id, column_family_id,
column_family_name, io_priority, write_hint, nullptr /*IOTracer*/,
&blob_file_paths, &blob_file_additions);
BlobFileBuilder builder(TestFileNumberGenerator(), fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id,
column_family_id, column_family_name, io_priority,
write_hint, nullptr /*IOTracer*/, &blob_file_paths,
&blob_file_additions);

const std::string key("1");
const std::string uncompressed_value(value_size, 'x');
Expand Down Expand Up @@ -429,7 +433,7 @@ TEST_F(BlobFileBuilderTest, CompressionError) {
0);
options.enable_blob_files = true;
options.blob_compression_type = kSnappyCompression;

options.env = &mock_env_;
ImmutableCFOptions immutable_cf_options(options);
MutableCFOptions mutable_cf_options(options);

Expand All @@ -442,11 +446,11 @@ TEST_F(BlobFileBuilderTest, CompressionError) {
std::vector<std::string> blob_file_paths;
std::vector<BlobFileAddition> blob_file_additions;

BlobFileBuilder builder(
TestFileNumberGenerator(), &mock_env_, fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id, column_family_id,
column_family_name, io_priority, write_hint, nullptr /*IOTracer*/,
&blob_file_paths, &blob_file_additions);
BlobFileBuilder builder(TestFileNumberGenerator(), fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id,
column_family_id, column_family_name, io_priority,
write_hint, nullptr /*IOTracer*/, &blob_file_paths,
&blob_file_additions);

SyncPoint::GetInstance()->SetCallBack("CompressData:TamperWithReturnValue",
[](void* arg) {
Expand Down Expand Up @@ -506,6 +510,7 @@ TEST_F(BlobFileBuilderTest, Checksum) {
options.enable_blob_files = true;
options.file_checksum_gen_factory =
std::make_shared<DummyFileChecksumGenFactory>();
options.env = &mock_env_;

ImmutableCFOptions immutable_cf_options(options);
MutableCFOptions mutable_cf_options(options);
Expand All @@ -519,11 +524,11 @@ TEST_F(BlobFileBuilderTest, Checksum) {
std::vector<std::string> blob_file_paths;
std::vector<BlobFileAddition> blob_file_additions;

BlobFileBuilder builder(
TestFileNumberGenerator(), &mock_env_, fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id, column_family_id,
column_family_name, io_priority, write_hint, nullptr /*IOTracer*/,
&blob_file_paths, &blob_file_additions);
BlobFileBuilder builder(TestFileNumberGenerator(), fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id,
column_family_id, column_family_name, io_priority,
write_hint, nullptr /*IOTracer*/, &blob_file_paths,
&blob_file_additions);

const std::string key("1");
const std::string value("deadbeef");
Expand Down Expand Up @@ -615,11 +620,11 @@ TEST_P(BlobFileBuilderIOErrorTest, IOError) {
std::vector<std::string> blob_file_paths;
std::vector<BlobFileAddition> blob_file_additions;

BlobFileBuilder builder(
TestFileNumberGenerator(), &mock_env_, fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id, column_family_id,
column_family_name, io_priority, write_hint, nullptr /*IOTracer*/,
&blob_file_paths, &blob_file_additions);
BlobFileBuilder builder(TestFileNumberGenerator(), fs_, &immutable_cf_options,
&mutable_cf_options, &file_options_, job_id,
column_family_id, column_family_name, io_priority,
write_hint, nullptr /*IOTracer*/, &blob_file_paths,
&blob_file_additions);

SyncPoint::GetInstance()->SetCallBack(sync_point_, [this](void* arg) {
Status* const s = static_cast<Status*>(arg);
Expand Down
8 changes: 4 additions & 4 deletions db/blob/blob_file_cache_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,15 @@ void WriteBlobFile(uint32_t column_family_id,

std::unique_ptr<WritableFileWriter> file_writer(
new WritableFileWriter(std::move(file), blob_file_path, FileOptions(),
immutable_cf_options.env->GetSystemClock()));
immutable_cf_options.clock));

constexpr Statistics* statistics = nullptr;
constexpr bool use_fsync = false;
constexpr bool do_flush = false;

BlobLogWriter blob_log_writer(
std::move(file_writer), immutable_cf_options.env->GetSystemClock(),
statistics, blob_file_number, use_fsync, do_flush);
BlobLogWriter blob_log_writer(std::move(file_writer),
immutable_cf_options.clock, statistics,
blob_file_number, use_fsync, do_flush);

constexpr bool has_ttl = false;
constexpr ExpirationRange expiration_range;
Expand Down
3 changes: 1 addition & 2 deletions db/blob/blob_file_reader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,7 @@ Status BlobFileReader::OpenFile(
}

file_reader->reset(new RandomAccessFileReader(
std::move(file), blob_file_path,
immutable_cf_options.env->GetSystemClock(), io_tracer,
std::move(file), blob_file_path, immutable_cf_options.clock, io_tracer,
immutable_cf_options.statistics, BLOB_DB_BLOB_FILE_READ_MICROS,
blob_file_read_hist, immutable_cf_options.rate_limiter,
immutable_cf_options.listeners));
Expand Down
16 changes: 8 additions & 8 deletions db/blob/blob_file_reader_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,15 @@ void WriteBlobFile(const ImmutableCFOptions& immutable_cf_options,

std::unique_ptr<WritableFileWriter> file_writer(
new WritableFileWriter(std::move(file), blob_file_path, FileOptions(),
immutable_cf_options.env->GetSystemClock()));
immutable_cf_options.clock));

constexpr Statistics* statistics = nullptr;
constexpr bool use_fsync = false;
constexpr bool do_flush = false;

BlobLogWriter blob_log_writer(
std::move(file_writer), immutable_cf_options.env->GetSystemClock(),
statistics, blob_file_number, use_fsync, do_flush);
BlobLogWriter blob_log_writer(std::move(file_writer),
immutable_cf_options.clock, statistics,
blob_file_number, use_fsync, do_flush);

BlobLogHeader header(column_family_id, compression_type, has_ttl,
expiration_range_header);
Expand Down Expand Up @@ -280,15 +280,15 @@ TEST_F(BlobFileReaderTest, Malformed) {

std::unique_ptr<WritableFileWriter> file_writer(
new WritableFileWriter(std::move(file), blob_file_path, FileOptions(),
immutable_cf_options.env->GetSystemClock()));
immutable_cf_options.clock));

constexpr Statistics* statistics = nullptr;
constexpr bool use_fsync = false;
constexpr bool do_flush = false;

BlobLogWriter blob_log_writer(
std::move(file_writer), immutable_cf_options.env->GetSystemClock(),
statistics, blob_file_number, use_fsync, do_flush);
BlobLogWriter blob_log_writer(std::move(file_writer),
immutable_cf_options.clock, statistics,
blob_file_number, use_fsync, do_flush);

BlobLogHeader header(column_family_id, kNoCompression, has_ttl,
expiration_range);
Expand Down
4 changes: 2 additions & 2 deletions db/blob/blob_log_sequential_reader.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
namespace ROCKSDB_NAMESPACE {

BlobLogSequentialReader::BlobLogSequentialReader(
std::unique_ptr<RandomAccessFileReader>&& file_reader,
const std::shared_ptr<SystemClock>& clock, Statistics* statistics)
std::unique_ptr<RandomAccessFileReader>&& file_reader, SystemClock* clock,
Statistics* statistics)
: file_(std::move(file_reader)),
clock_(clock),
statistics_(statistics),
Expand Down
5 changes: 2 additions & 3 deletions db/blob/blob_log_sequential_reader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ class BlobLogSequentialReader {

// Create a reader that will return log records from "*file_reader".
BlobLogSequentialReader(std::unique_ptr<RandomAccessFileReader>&& file_reader,
const std::shared_ptr<SystemClock>& clock,
Statistics* statistics);
SystemClock* clock, Statistics* statistics);

// No copying allowed
BlobLogSequentialReader(const BlobLogSequentialReader&) = delete;
Expand Down Expand Up @@ -65,7 +64,7 @@ class BlobLogSequentialReader {
Status ReadSlice(uint64_t size, Slice* slice, char* buf);

const std::unique_ptr<RandomAccessFileReader> file_;
std::shared_ptr<SystemClock> clock_;
SystemClock* clock_;

Statistics* statistics_;

Expand Down
6 changes: 3 additions & 3 deletions db/blob/blob_log_writer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
namespace ROCKSDB_NAMESPACE {

BlobLogWriter::BlobLogWriter(std::unique_ptr<WritableFileWriter>&& dest,
const std::shared_ptr<SystemClock>& clock,
Statistics* statistics, uint64_t log_number,
bool use_fs, bool do_flush, uint64_t boffset)
SystemClock* clock, Statistics* statistics,
uint64_t log_number, bool use_fs, bool do_flush,
uint64_t boffset)
: dest_(std::move(dest)),
clock_(clock),
statistics_(statistics),
Expand Down
Loading

0 comments on commit 3dff28c

Please sign in to comment.