From 6d70deff8139f8596cdd4b986da8bb4669714fb6 Mon Sep 17 00:00:00 2001 From: Si Ke Date: Mon, 13 Dec 2021 17:44:38 +0000 Subject: [PATCH 1/8] Fixed compiling errors --- db/db_test.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index c3f1954c6ae..6a8c339579b 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -6799,7 +6799,7 @@ TEST_F(DBTest, LargeBlockSizeTest) { CreateAndReopenWithCF({"pikachu"}, options); ASSERT_OK(Put(0, "foo", "bar")); BlockBasedTableOptions table_options; - table_options.block_size = 8LL * 1024 * 1024 * 1024LL; + table_options.block_size = (size_t)(8LL * 1024 * 1024 * 1024LL); options.table_factory.reset(NewBlockBasedTableFactory(table_options)); ASSERT_NOK(TryReopenWithColumnFamilies({"default", "pikachu"}, options)); } @@ -6942,8 +6942,9 @@ TEST_F(DBTest, MemoryUsageWithMaxWriteBufferSizeToMaintain) { if ((size_all_mem_table > cur_active_mem) && (cur_active_mem >= static_cast(options.max_write_buffer_size_to_maintain)) && - (size_all_mem_table > options.max_write_buffer_size_to_maintain + - options.write_buffer_size)) { + (size_all_mem_table > + static_cast(options.max_write_buffer_size_to_maintain) + + options.write_buffer_size)) { ASSERT_FALSE(memory_limit_exceeded); memory_limit_exceeded = true; } else { From 18209e89fcb520201bd6befaa4a333a11bc16a07 Mon Sep 17 00:00:00 2001 From: Si Ke Date: Mon, 13 Dec 2021 18:11:55 +0000 Subject: [PATCH 2/8] Fixed bugs for 32-bit db_test --- env/fs_posix.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/env/fs_posix.cc b/env/fs_posix.cc index 52264ce7c22..887555af34b 100644 --- a/env/fs_posix.cc +++ b/env/fs_posix.cc @@ -238,7 +238,11 @@ class PosixFileSystem : public FileSystem { } SetFD_CLOEXEC(fd, &options); - if (options.use_mmap_reads && sizeof(void*) >= 8) { + // Remove 64 bit OS checking. Both 64 and 32 bit OSes use mmap to read files + // There is one issue for 32 bit OS if PosixRandomAccessFile is used to read + // files - Here is the comments + // https://github.com/facebook/rocksdb/issues/9271#issuecomment-991230315 + if (options.use_mmap_reads) { // Use of mmap for random reads has been removed because it // kills performance when storage is fast. // Use mmap when virtual address-space is plentiful. From 1e3137a93024ee8c0575487c5e5de2f39f12e333 Mon Sep 17 00:00:00 2001 From: Si Ke Date: Mon, 13 Dec 2021 21:55:50 +0000 Subject: [PATCH 3/8] Fixed the problem for multithreaded test cases --- db/db_test.cc | 13 +++++++++++-- env/io_posix.cc | 2 +- util/regex.cc | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index 6a8c339579b..5f95980856a 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -2738,7 +2738,10 @@ TEST_P(MultiThreadedDBTest, MultiThreaded) { thread[id].state = &mt; thread[id].id = id; thread[id].multiget_batched = multiget_batched_; - env_->StartThread(MTThreadBody, &thread[id]); + // Cannot use env_->StartThread because the thread needs to be detached. + // There is no function in env_ to detach the thread. + // std::thread can be used in multiple OSes + std::thread(MTThreadBody, &thread[id]).detach(); } // Let them run for a while @@ -6801,7 +6804,13 @@ TEST_F(DBTest, LargeBlockSizeTest) { BlockBasedTableOptions table_options; table_options.block_size = (size_t)(8LL * 1024 * 1024 * 1024LL); options.table_factory.reset(NewBlockBasedTableFactory(table_options)); - ASSERT_NOK(TryReopenWithColumnFamilies({"default", "pikachu"}, options)); + // size_t in 32 bit OS is defined as unsigned integer + // It cannot make large block size in 32 bit OS + if (sizeof(table_options.block_size) >= 8) { + ASSERT_NOK(TryReopenWithColumnFamilies({"default", "pikachu"}, options)); + } else { + ASSERT_OK(TryReopenWithColumnFamilies({"default", "pikachu"}, options)); + } } #ifndef ROCKSDB_LITE diff --git a/env/io_posix.cc b/env/io_posix.cc index 8b09cce572f..d77d38fc654 100644 --- a/env/io_posix.cc +++ b/env/io_posix.cc @@ -1543,7 +1543,7 @@ PosixDirectory::PosixDirectory(int fd) : fd_(fd) { #ifdef OS_LINUX struct statfs buf; int ret = fstatfs(fd, &buf); - is_btrfs_ = (ret == 0 && buf.f_type == BTRFS_SUPER_MAGIC); + is_btrfs_ = (ret == 0 && buf.f_type == (int)BTRFS_SUPER_MAGIC); #endif } diff --git a/util/regex.cc b/util/regex.cc index 7aa4c52ae2f..d8c3ae8cfca 100644 --- a/util/regex.cc +++ b/util/regex.cc @@ -26,7 +26,7 @@ bool Regex::Matches(const std::string &str) const { return std::regex_match(str, *impl_); } else { // Should not call Matches on unset Regex - assert(false); + // assert(false); return false; } } From 1a9afac0ef035237f30a87c7400dfcc2e30f78aa Mon Sep 17 00:00:00 2001 From: Si Ke Date: Thu, 16 Dec 2021 21:25:23 +0000 Subject: [PATCH 4/8] Disable one test on 32 bit platform --- db/db_test.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index 5f45f3a6143..59caa2a4d6c 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -4914,8 +4914,12 @@ TEST_F(DBTest, DynamicLevelCompressionPerLevel) { ASSERT_OK(dbfull()->TEST_WaitForCompact()); ASSERT_EQ(NumTableFilesAtLevel(1), 0); ASSERT_EQ(NumTableFilesAtLevel(2), 0); - ASSERT_LT(SizeAtLevel(0) + SizeAtLevel(3) + SizeAtLevel(4), - 120U * 4000U + 50U * 24); + // The default compaction method snappy does not work on Alpine 32 platform + // This test case has been disabled on 32 bit platform + if (sizeof(void*) >= 8) { + ASSERT_LT(SizeAtLevel(0) + SizeAtLevel(3) + SizeAtLevel(4), + 120U * 4000U + 50U * 24); + } // Make sure data in files in L3 is not compacted by removing all files // in L4 and calculate number of rows ASSERT_OK(dbfull()->SetOptions({ From 6e6c4d2f8621233064fdd72d33e577fdd64a5c2f Mon Sep 17 00:00:00 2001 From: Si Ke Date: Wed, 22 Dec 2021 05:42:34 +0000 Subject: [PATCH 5/8] Modified code based on the code reviewer comments --- db/db_test.cc | 50 ++++++++----------- env/fs_posix.cc | 4 -- include/rocksdb/table.h | 2 +- .../block_based/block_based_table_builder.cc | 3 +- .../block_based/block_based_table_factory.cc | 2 +- 5 files changed, 25 insertions(+), 36 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index 59caa2a4d6c..7b1ff883fe6 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -2576,9 +2576,7 @@ static const int kNumKeys = 1000; struct MTState { DBTest* test; - std::atomic stop; std::atomic counter[kNumThreads]; - std::atomic thread_done[kNumThreads]; }; struct MTThread { @@ -2587,15 +2585,23 @@ struct MTThread { bool multiget_batched; }; +// Based on Peter Dillinger's suggestion, the threads have been changed from +// detached threads to joinable threads static void MTThreadBody(void* arg) { MTThread* t = reinterpret_cast(arg); int id = t->id; DB* db = t->state->test->db_; int counter = 0; + auto start = std::chrono::high_resolution_clock::now(); + auto elapsed = std::chrono::high_resolution_clock::now() - start; + long long runForMicroseconds(kTestSeconds * 1000000); + fprintf(stderr, "... starting thread %d\n", id); Random rnd(1000 + id); char valbuf[1500]; - while (t->state->stop.load(std::memory_order_acquire) == false) { + long long microseconds = + std::chrono::duration_cast(elapsed).count(); + while (microseconds < runForMicroseconds) { t->state->counter[id].store(counter, std::memory_order_release); int key = rnd.Uniform(kNumKeys); @@ -2691,8 +2697,10 @@ static void MTThreadBody(void* arg) { } } counter++; + elapsed = std::chrono::high_resolution_clock::now() - start; + microseconds = + std::chrono::duration_cast(elapsed).count(); } - t->state->thread_done[id].store(true, std::memory_order_release); fprintf(stderr, "... stopping thread %d after %d ops\n", id, int(counter)); } @@ -2731,10 +2739,8 @@ TEST_P(MultiThreadedDBTest, MultiThreaded) { // Initialize state MTState mt; mt.test = this; - mt.stop.store(false, std::memory_order_release); for (int id = 0; id < kNumThreads; id++) { mt.counter[id].store(0, std::memory_order_release); - mt.thread_done[id].store(false, std::memory_order_release); } // Start threads @@ -2743,22 +2749,12 @@ TEST_P(MultiThreadedDBTest, MultiThreaded) { thread[id].state = &mt; thread[id].id = id; thread[id].multiget_batched = multiget_batched_; - // Cannot use env_->StartThread because the thread needs to be detached. - // There is no function in env_ to detach the thread. - // std::thread can be used in multiple OSes - std::thread(MTThreadBody, &thread[id]).detach(); + env_->StartThread(MTThreadBody, &thread[id]); } - // Let them run for a while - env_->SleepForMicroseconds(kTestSeconds * 1000000); - - // Stop the threads and wait for them to finish - mt.stop.store(true, std::memory_order_release); - for (int id = 0; id < kNumThreads; id++) { - while (mt.thread_done[id].load(std::memory_order_acquire) == false) { - env_->SleepForMicroseconds(100000); - } - } + // Based on Peter Dillinger's suggestion, the threads have been changed from + // detached threads to joinable threads + env_->WaitForJoin(); } INSTANTIATE_TEST_CASE_P( @@ -4914,8 +4910,10 @@ TEST_F(DBTest, DynamicLevelCompressionPerLevel) { ASSERT_OK(dbfull()->TEST_WaitForCompact()); ASSERT_EQ(NumTableFilesAtLevel(1), 0); ASSERT_EQ(NumTableFilesAtLevel(2), 0); + // The default compaction method snappy does not work on Alpine 32 platform - // This test case has been disabled on 32 bit platform + // That means there is no compression + // The following test case has been disabled on 32 bit platform if (sizeof(void*) >= 8) { ASSERT_LT(SizeAtLevel(0) + SizeAtLevel(3) + SizeAtLevel(4), 120U * 4000U + 50U * 24); @@ -6843,15 +6841,9 @@ TEST_F(DBTest, LargeBlockSizeTest) { CreateAndReopenWithCF({"pikachu"}, options); ASSERT_OK(Put(0, "foo", "bar")); BlockBasedTableOptions table_options; - table_options.block_size = (size_t)(8LL * 1024 * 1024 * 1024LL); + table_options.block_size = 8LL * 1024 * 1024 * 1024LL; options.table_factory.reset(NewBlockBasedTableFactory(table_options)); - // size_t in 32 bit OS is defined as unsigned integer - // It cannot make large block size in 32 bit OS - if (sizeof(table_options.block_size) >= 8) { - ASSERT_NOK(TryReopenWithColumnFamilies({"default", "pikachu"}, options)); - } else { - ASSERT_OK(TryReopenWithColumnFamilies({"default", "pikachu"}, options)); - } + ASSERT_NOK(TryReopenWithColumnFamilies({"default", "pikachu"}, options)); } #ifndef ROCKSDB_LITE diff --git a/env/fs_posix.cc b/env/fs_posix.cc index 887555af34b..f35a2b9e1cf 100644 --- a/env/fs_posix.cc +++ b/env/fs_posix.cc @@ -238,10 +238,6 @@ class PosixFileSystem : public FileSystem { } SetFD_CLOEXEC(fd, &options); - // Remove 64 bit OS checking. Both 64 and 32 bit OSes use mmap to read files - // There is one issue for 32 bit OS if PosixRandomAccessFile is used to read - // files - Here is the comments - // https://github.com/facebook/rocksdb/issues/9271#issuecomment-991230315 if (options.use_mmap_reads) { // Use of mmap for random reads has been removed because it // kills performance when storage is fast. diff --git a/include/rocksdb/table.h b/include/rocksdb/table.h index 3590b7f42f1..1e48eca72c3 100644 --- a/include/rocksdb/table.h +++ b/include/rocksdb/table.h @@ -259,7 +259,7 @@ struct BlockBasedTableOptions { // block size specified here corresponds to uncompressed data. The // actual size of the unit read from disk may be smaller if // compression is enabled. This parameter can be changed dynamically. - size_t block_size = 4 * 1024; + uint64_t block_size = 4 * 1024; // This is used to close a block before it reaches the configured // 'block_size'. If the percentage of free space in the current block is less diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc index 62f0b3bb408..492f9f57091 100644 --- a/table/block_based/block_based_table_builder.cc +++ b/table/block_based/block_based_table_builder.cc @@ -408,7 +408,8 @@ struct BlockBasedTableBuilder::Rep { file(f), offset(0), alignment(table_options.block_align - ? std::min(table_options.block_size, kDefaultPageSize) + ? std::min(static_cast(table_options.block_size), + kDefaultPageSize) : 0), data_block(table_options.block_restart_interval, table_options.use_delta_encoding, diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc index 08cd06b6cbe..5b1bd2e6861 100644 --- a/table/block_based/block_based_table_factory.cc +++ b/table/block_based/block_based_table_factory.cc @@ -778,7 +778,7 @@ std::string BlockBasedTableFactory::GetPrintableOptions() const { ret.append(buffer); ret.append(table_options_.persistent_cache->GetPrintableOptions()); } - snprintf(buffer, kBufferSize, " block_size: %" ROCKSDB_PRIszt "\n", + snprintf(buffer, kBufferSize, " block_size: %" PRIu64 "\n", table_options_.block_size); ret.append(buffer); snprintf(buffer, kBufferSize, " block_size_deviation: %d\n", From fe182924efa74042b0fc75c8c6f10c6bffbc156e Mon Sep 17 00:00:00 2001 From: Si Ke Date: Wed, 22 Dec 2021 14:17:50 +0000 Subject: [PATCH 6/8] Uncommented the code according to the PR #9264 --- util/regex.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/regex.cc b/util/regex.cc index d8c3ae8cfca..7aa4c52ae2f 100644 --- a/util/regex.cc +++ b/util/regex.cc @@ -26,7 +26,7 @@ bool Regex::Matches(const std::string &str) const { return std::regex_match(str, *impl_); } else { // Should not call Matches on unset Regex - // assert(false); + assert(false); return false; } } From bd5584643e5bcd3078d419015507221cab1f0d5a Mon Sep 17 00:00:00 2001 From: Si Ke Date: Tue, 4 Jan 2022 22:01:46 +0000 Subject: [PATCH 7/8] Removed 32 bit checking and some comments --- db/db_test.cc | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index 7b1ff883fe6..8901dd52d6b 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -2585,23 +2585,18 @@ struct MTThread { bool multiget_batched; }; -// Based on Peter Dillinger's suggestion, the threads have been changed from -// detached threads to joinable threads static void MTThreadBody(void* arg) { MTThread* t = reinterpret_cast(arg); int id = t->id; DB* db = t->state->test->db_; int counter = 0; - auto start = std::chrono::high_resolution_clock::now(); - auto elapsed = std::chrono::high_resolution_clock::now() - start; - long long runForMicroseconds(kTestSeconds * 1000000); + std::shared_ptr clock = SystemClock::Default(); + auto end_micros = clock->NowMicros() + kTestSeconds * 1000000U; fprintf(stderr, "... starting thread %d\n", id); Random rnd(1000 + id); char valbuf[1500]; - long long microseconds = - std::chrono::duration_cast(elapsed).count(); - while (microseconds < runForMicroseconds) { + while (clock->NowMicros() < end_micros) { t->state->counter[id].store(counter, std::memory_order_release); int key = rnd.Uniform(kNumKeys); @@ -2697,9 +2692,6 @@ static void MTThreadBody(void* arg) { } } counter++; - elapsed = std::chrono::high_resolution_clock::now() - start; - microseconds = - std::chrono::duration_cast(elapsed).count(); } fprintf(stderr, "... stopping thread %d after %d ops\n", id, int(counter)); } @@ -2752,8 +2744,6 @@ TEST_P(MultiThreadedDBTest, MultiThreaded) { env_->StartThread(MTThreadBody, &thread[id]); } - // Based on Peter Dillinger's suggestion, the threads have been changed from - // detached threads to joinable threads env_->WaitForJoin(); } @@ -4914,10 +4904,8 @@ TEST_F(DBTest, DynamicLevelCompressionPerLevel) { // The default compaction method snappy does not work on Alpine 32 platform // That means there is no compression // The following test case has been disabled on 32 bit platform - if (sizeof(void*) >= 8) { - ASSERT_LT(SizeAtLevel(0) + SizeAtLevel(3) + SizeAtLevel(4), - 120U * 4000U + 50U * 24); - } + ASSERT_LT(SizeAtLevel(0) + SizeAtLevel(3) + SizeAtLevel(4), + 120U * 4000U + 50U * 24); // Make sure data in files in L3 is not compacted by removing all files // in L4 and calculate number of rows ASSERT_OK(dbfull()->SetOptions({ From 550be57a0a708410a9dda9383426eecbd631a94d Mon Sep 17 00:00:00 2001 From: Si Ke Date: Fri, 14 Jan 2022 02:06:12 +0000 Subject: [PATCH 8/8] Removed the comments about snappy --- db/db_test.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index 8901dd52d6b..fa1aa54cfa7 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -4901,9 +4901,6 @@ TEST_F(DBTest, DynamicLevelCompressionPerLevel) { ASSERT_EQ(NumTableFilesAtLevel(1), 0); ASSERT_EQ(NumTableFilesAtLevel(2), 0); - // The default compaction method snappy does not work on Alpine 32 platform - // That means there is no compression - // The following test case has been disabled on 32 bit platform ASSERT_LT(SizeAtLevel(0) + SizeAtLevel(3) + SizeAtLevel(4), 120U * 4000U + 50U * 24); // Make sure data in files in L3 is not compacted by removing all files