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

@@ -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
+  // Phase 0. The verify the initial value of all these properties are the same
-  // the same as we have no mem-tables.
+  // 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"
+  // Phase 1. Simply issue Put() and expect "cur-size-all-mem-tables" equals to
-  // equals to "size-all-mem-tables"
+  // "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
+  // Phase 2. Keep issuing Put() but also create new iterators. This time we
-  // we expect "size-all-mem-tables" > "cur-size-all-mem-tables".
+  // 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"
+  // Phase 3. Delete iterators and expect "size-all-mem-tables" shrinks
-  // shrinks whenever we release an iterator.
+  // whenever we release an iterator.
   for (auto* iter : iters) {
     delete iter;
-    if (iters.size() != 0) {
+    dbfull()->GetIntProperty("rocksdb.size-all-mem-tables", &all_mem);
-      dbfull()->GetIntProperty("rocksdb.size-all-mem-tables", &all_mem);
+    // Expect the size shrinking
-      // Expect the size shrinking
+    ASSERT_LT(all_mem, prev_all_mem);
-      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
+  // now we expect "cur-size-all-mem-tables" and "size-all-mem-tables" are the
-  // "size-all-mem-tables" are the same again after we
+  // same again after we released all iterators.
-  // 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.
+  // Phase 4. Perform flush, and expect all these three counters to be the same.
-  Flush();
   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);