From e853191c17eb85f527d0e495a612fd0679b241ea Mon Sep 17 00:00:00 2001 From: Andres Noetzli Date: Thu, 27 Aug 2015 16:17:08 -0700 Subject: [PATCH] Fix DBTest.ApproximateMemoryUsage Summary: This patch fixes two issues in DBTest.ApproximateMemoryUsage: - It was possible that a flush happened between getting the two properties in Phase 1, resulting in different numbers for the properties and failing the assertion. This is fixed by waiting for the flush to finish before getting the properties. - There was a similar issue in Phase 2 and additionally there was an issue that rocksdb.size-all-mem-tables was not monotonically increasing because it was possible that a flush happened just after getting the properties and then another flush just before getting the properties in the next round. In this situation, the reported memory usage decreased. This is fixed by forcing a flush before getting the properties. Note: during testing, I found that kFlushesPerRound does not seem very accurate. I added a TODO for this and it would be great to get some input on what to do there. Test Plan: The first issue can be made more likely to trigger by inserting a `usleep(10000);` between the calls to GetIntProperty() in Phase 1. The second issue can be made more likely to trigger by inserting a `if (r != 0) usleep(10000);` before the calls to GetIntProperty() and a `usleep(10000);` after the calls. Then execute make db_test && ./db_test --gtest_filter=DBTest.ApproximateMemoryUsage Reviewers: rven, yhchiang, igor, sdong, anthony Reviewed By: anthony Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D45675 --- db/db_test.cc | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/db/db_test.cc b/db/db_test.cc index 4c06fa9be..aa59d76fd 100644 --- a/db/db_test.cc +++ b/db/db_test.cc @@ -2386,6 +2386,8 @@ TEST_F(DBTest, GetProperty) { TEST_F(DBTest, ApproximateMemoryUsage) { const int kNumRounds = 10; + // TODO(noetzli) kFlushesPerRound does not really correlate with how many + // flushes happen. const int kFlushesPerRound = 10; const int kWritesPerFlush = 10; const int kKeySize = 100; @@ -2407,22 +2409,24 @@ TEST_F(DBTest, ApproximateMemoryUsage) { uint64_t all_mem; uint64_t prev_all_mem; - // Phase 0. The verify the initial value of all these properties are - // the same as we have no mem-tables. + // Phase 0. The verify the initial value of all these properties are the same + // as we have no mem-tables. dbfull()->GetIntProperty("rocksdb.cur-size-active-mem-table", &active_mem); dbfull()->GetIntProperty("rocksdb.cur-size-all-mem-tables", &unflushed_mem); dbfull()->GetIntProperty("rocksdb.size-all-mem-tables", &all_mem); ASSERT_EQ(all_mem, active_mem); ASSERT_EQ(all_mem, unflushed_mem); - // Phase 1. Simply issue Put() and expect "cur-size-all-mem-tables" - // equals to "size-all-mem-tables" + // Phase 1. Simply issue Put() and expect "cur-size-all-mem-tables" equals to + // "size-all-mem-tables" for (int r = 0; r < kNumRounds; ++r) { for (int f = 0; f < kFlushesPerRound; ++f) { for (int w = 0; w < kWritesPerFlush; ++w) { Put(RandomString(&rnd, kKeySize), RandomString(&rnd, kValueSize)); } } + // Make sure that there is no flush between getting the two properties. + dbfull()->TEST_WaitForFlushMemTable(); dbfull()->GetIntProperty("rocksdb.cur-size-all-mem-tables", &unflushed_mem); dbfull()->GetIntProperty("rocksdb.size-all-mem-tables", &all_mem); // in no iterator case, these two number should be the same. @@ -2430,8 +2434,8 @@ TEST_F(DBTest, ApproximateMemoryUsage) { } prev_all_mem = all_mem; - // Phase 2. Keep issuing Put() but also create new iterator. This time - // we expect "size-all-mem-tables" > "cur-size-all-mem-tables". + // Phase 2. Keep issuing Put() but also create new iterators. This time we + // expect "size-all-mem-tables" > "cur-size-all-mem-tables". for (int r = 0; r < kNumRounds; ++r) { iters.push_back(db_->NewIterator(ReadOptions())); for (int f = 0; f < kFlushesPerRound; ++f) { @@ -2439,6 +2443,10 @@ TEST_F(DBTest, ApproximateMemoryUsage) { Put(RandomString(&rnd, kKeySize), RandomString(&rnd, kValueSize)); } } + // Force flush to prevent flush from happening between getting the + // properties or after getting the properties and before the new round. + Flush(); + // In the second round, add iterators. dbfull()->GetIntProperty("rocksdb.cur-size-active-mem-table", &active_mem); dbfull()->GetIntProperty("rocksdb.cur-size-all-mem-tables", &unflushed_mem); @@ -2449,35 +2457,32 @@ TEST_F(DBTest, ApproximateMemoryUsage) { prev_all_mem = all_mem; } - // Phase 3. Delete iterators and expect "size-all-mem-tables" - // shrinks whenever we release an iterator. + // Phase 3. Delete iterators and expect "size-all-mem-tables" shrinks + // whenever we release an iterator. for (auto* iter : iters) { delete iter; - if (iters.size() != 0) { - dbfull()->GetIntProperty("rocksdb.size-all-mem-tables", &all_mem); - // Expect the size shrinking - ASSERT_LT(all_mem, prev_all_mem); - } + dbfull()->GetIntProperty("rocksdb.size-all-mem-tables", &all_mem); + // Expect the size shrinking + ASSERT_LT(all_mem, prev_all_mem); prev_all_mem = all_mem; } + dbfull()->GetIntProperty("rocksdb.cur-size-active-mem-table", &active_mem); dbfull()->GetIntProperty("rocksdb.cur-size-all-mem-tables", &unflushed_mem); dbfull()->GetIntProperty("rocksdb.size-all-mem-tables", &all_mem); - // now we expect "cur-size-all-mem-tables" and - // "size-all-mem-tables" are the same again after we - // released all iterators. + // now we expect "cur-size-all-mem-tables" and "size-all-mem-tables" are the + // same again after we released all iterators. ASSERT_EQ(all_mem, unflushed_mem); ASSERT_GE(all_mem, active_mem); - // Phase 4. Perform flush, and expect all these three counters are the same. - Flush(); + // Phase 4. Perform flush, and expect all these three counters to be the same. dbfull()->GetIntProperty("rocksdb.cur-size-active-mem-table", &active_mem); dbfull()->GetIntProperty("rocksdb.cur-size-all-mem-tables", &unflushed_mem); dbfull()->GetIntProperty("rocksdb.size-all-mem-tables", &all_mem); ASSERT_EQ(active_mem, unflushed_mem); ASSERT_EQ(unflushed_mem, all_mem); - // Phase 5. Reopen, and expect all these three counters are the same again. + // Phase 5. Reopen, and expect all these three counters to be the same again. Reopen(options); dbfull()->GetIntProperty("rocksdb.cur-size-active-mem-table", &active_mem); dbfull()->GetIntProperty("rocksdb.cur-size-all-mem-tables", &unflushed_mem); -- GitLab