MB-23263: DefragmenterVisitor: Check blob refcount 58/76958/10
authorolivermd <oliver.downard@couchbase.com>
Wed, 19 Apr 2017 08:55:34 +0000 (09:55 +0100)
committerDave Rigby <daver@couchbase.com>
Wed, 19 Apr 2017 14:41:14 +0000 (14:41 +0000)
This patch adds the facility to check the refcount value of a
SingleThreadedRCPTR and uses this to ensure that the refcount of the
blob is less than 2 before the defragmenter reallocates it. This is to
ensure that we do not end up just creating a copy of the blob thus
increasing memory usage which would be the opposite of what you would
expect from the defragmenter.

It's worth noting that it appears as though something could create
another reference to the blob without holding the hashtable lock. This
could lead to a soft data race on the refcount. This means that we
cannot give a hard guarantee that the defragmenter doesn't duplicate the
blob, just that it will in most cases. The case where it won't will be
where the other thing creating the reference doesn't hold the hash
bucket lock and happens to create a reference after refcount is read but
before the blob is reallocated, which seems rare.

Change-Id: I3a8b8812ac039445451952384cb5b40ce8b433cb
Reviewed-on: http://review.couchbase.org/76958
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>
src/atomic.h
src/defragmenter_visitor.cc
tests/module_tests/defragmenter_test.cc

index 89e1438..8945b1b 100644 (file)
@@ -273,6 +273,10 @@ public:
         swap(other.gimme());
     }
 
+    int refCount() const {
+        return static_cast<RCValue*>(value)->_rc_refcount.load();
+    }
+
     // safe for the lifetime of this instance
     T *get() const {
         return value;
index 1f06a3b..fa2cbcc 100644 (file)
@@ -94,8 +94,13 @@ bool DefragmentVisitor::visit(StoredValue& v) {
     // supports, so it can be successfully reallocated to a run with other
     // objects of the same size.
     if (value_len > 0 && value_len <= max_size_class) {
-        // If sufficiently old reallocate, otherwise increment it's age.
-        if (v.getValue()->getAge() >= age_threshold) {
+        // If sufficiently old and if it looks like nothing else holds a
+        // reference to the blob reallocate, otherwise increment it's age.
+        // It may be possible to add a reference to the blob without holding
+        // any locks, therefore the check is somewhat of an estimate which
+        // should be good enough.
+        if (v.getValue()->getAge() >= age_threshold &&
+            v.getValue().refCount() < 2) {
             v.reallocate();
             defrag_count++;
         } else {
index c905efa..2217c4a 100644 (file)
@@ -20,7 +20,6 @@
 #include "daemon/alloc_hooks.h"
 #include "defragmenter_visitor.h"
 
-#include <gtest/gtest.h>
 #include <valgrind/valgrind.h>
 
 
@@ -249,6 +248,85 @@ TEST_P(DefragmenterTest, DISABLED_MappedMemory) {
         << "and moved " << visitor.getDefragCount() << " items!";
 }
 
+// Check that the defragmenter doesn't increase the memory used. The specific
+// case we are testing here is what happens when a reference to the blobs is
+// also held by the Checkpoint Manager. See MB-23263.
+/* The Defragmenter (and hence it's unit tests) depend on using jemalloc as the
+ * memory allocator.
+ */
+#if defined(HAVE_JEMALLOC)
+TEST_P(DefragmenterTest, RefCountMemUsage) {
+#else
+TEST_P(DefragmenterTest, DISABLED_RefCountMemUsage) {
+#endif
+
+    /*
+     * Similarly to the MappedMemory test, this test appears to cause issues
+     * with Valgrind. So it is disabled under Valgrind for now. I've included
+     * the same explanation below for completeness.
+     *
+      MB-22016:
+      Disable this test for valgrind as it currently triggers some problems
+      within jemalloc that will get detected much further down the set of
+      unit-tests.
+
+      The problem appears to be the toggling of thread cache, I suspect that
+      jemalloc is writing some data when the thread cache is off (during the
+      defrag) and then accessing that data differently with thread cache on.
+
+      The will link to an issue on jemalloc to see if there is anything to be
+      changed.
+    */
+    if (RUNNING_ON_VALGRIND) {
+        printf("DefragmenterTest.RefCountMemUsage is currently disabled for"
+               " valgrind\n");
+        return;
+    }
+
+    // Sanity check - need memory tracker to be able to check our memory usage.
+    ASSERT_TRUE(MemoryTracker::trackingMemoryAllocations())
+            << "Memory tracker not enabled - cannot continue";
+
+    // 1. Create a small number of documents to record the checkpoint manager
+    // overhead with
+    const size_t size = 3500;
+    const size_t num_docs = 50000;
+    setDocs(size, num_docs);
+
+    // 2. Determine how many documents are in each page, and then remove all but
+    //    one from each page.
+    size_t num_remaining = num_docs;
+    fragment(num_docs, num_remaining);
+
+    // Release free memory back to OS to minimize our footprint after
+    // removing the documents above.
+    AllocHooks::release_free_memory();
+
+    size_t mem_used_before_defrag = mem_used.load();
+
+    // The refcounts of all blobs should at least 2 at this point as the
+    // CheckpointManager will also be holding references to them.
+
+    // 3. Enable defragmenter and trigger defragmentation but let the
+    // defragmenter drop out of scope afterwards to avoid interfering
+    // with the memory measurements.
+    {
+        AllocHooks::enable_thread_cache(false);
+
+        DefragmentVisitor visitor(0);
+        visitor.visit(vbucket->getId(), vbucket->ht);
+
+        AllocHooks::enable_thread_cache(true);
+        AllocHooks::release_free_memory();
+        ASSERT_EQ(visitor.getVisitedCount(), num_remaining);
+        ASSERT_EQ(visitor.getDefragCount(), 0);
+    }
+
+    size_t mem_used_after_defrag = mem_used.load();
+
+    EXPECT_LE(mem_used_after_defrag, mem_used_before_defrag);
+}
+
 INSTANTIATE_TEST_CASE_P(
         FullAndValueEviction,
         DefragmenterTest,