MB-27554: [BP] Centralize HashTable count statistics

Originally 4f6873e

HashTable maintains a number of statistics about how many StoredValues
it contains which match some criteria - for example number of
non-resident, number with a given datatype, etc.

Ensuring the count of these statistics is accurate is currenly
error-prone - each of the different operations which may result in the
count changing needs to take care to update the relevent stat(s).

To make this more robust, refactor the counting of these statistics so
it is centrallised in two symmetric methods - statsPrologue() and
statsEpilogue(). statsPrologue should be called before making any
changes which may affect any of the stats (i.e. whenever a StoredValue
is changed), and statsEpilogue should be called afterwards.

By moving the all stats accounting to two methods (which are vitually
identical) we should minimise any accounting issues.

Change-Id: I33bb436a7a7d80d1c8569ce825ba215a0f47f14f
Reviewed-on: http://review.couchbase.org/88377
Well-Formed: Build Bot <[email protected]>
Tested-by: Build Bot <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: daverigby

engines/ep/src/hash_table.cc

@@ -277,34 +277,13 @@ MutationStatus HashTable::unlocked_updateStoredValue(
 
     MutationStatus status =
             v.isDirty() ? MutationStatus::WasDirty : MutationStatus::WasClean;
-    if (!v.isResident() && !v.isDeleted() && !v.isTempItem()) {
-        decrNumNonResidentItems();
-    }
-
-    if (v.isTempItem()) {
-        --numTempItems;
-        ++numItems;
-        ++numTotalItems;
-    }
-
-    if (v.isDeleted() && !itm.isDeleted()) {
-        --numDeletedItems;
-    }
-    if (!v.isDeleted() && itm.isDeleted()) {
-        ++numDeletedItems;
-    }
 
-    // Update datatype counts (which only count non-temp, non-deleted
-    // documents).
-    if (!v.isDeleted() && !v.isTempItem()) {
-        --datatypeCounts[v.getDatatype()];
-    }
-    if (!itm.isDeleted()) {
-        ++datatypeCounts[itm.getDataType()];
-    }
+    statsPrologue(v);
 
     /* setValue() will mark v as undeleted if required */
-    setValue(itm, v);
+    v.setValue(itm);
+
+    statsEpilogue(v);
 
     return status;
 }
@@ -325,23 +304,55 @@ StoredValue* HashTable::unlocked_addNewStoredValue(const HashBucketLock& hbl,
 
     // Create a new StoredValue and link it into the head of the bucket chain.
     auto v = (*valFact)(itm, std::move(values[hbl.getBucketNum()]));
-    increaseMetaDataSize(stats, v->metaDataSize());
-    increaseCacheSize(v->size());
 
-    if (v->isTempItem()) {
+    statsEpilogue(*v);
+
+    values[hbl.getBucketNum()] = std::move(v);
+    return values[hbl.getBucketNum()].get();
+}
+
+void HashTable::statsPrologue(const StoredValue& v) {
+    // Decrease all statistics which sv matches.
+    reduceMetaDataSize(stats, v.metaDataSize());
+    reduceCacheSize(v.size());
+
+    if (!v.isResident() && !v.isDeleted() && !v.isTempItem()) {
+        decrNumNonResidentItems();
+    }
+
+    if (v.isTempItem()) {
+        --numTempItems;
+    } else {
+        --numItems;
+        --numTotalItems;
+        if (v.isDeleted()) {
+            --numDeletedItems;
+        } else {
+            --datatypeCounts[v.getDatatype()];
+        }
+    }
+}
+
+void HashTable::statsEpilogue(const StoredValue& v) {
+    // After performing updates to sv; increase all statistics which sv matches.
+    increaseMetaDataSize(stats, v.metaDataSize());
+    increaseCacheSize(v.size());
+
+    if (!v.isResident() && !v.isDeleted() && !v.isTempItem()) {
+        ++numNonResidentItems;
+    }
+
+    if (v.isTempItem()) {
         ++numTempItems;
     } else {
         ++numItems;
         ++numTotalItems;
+        if (v.isDeleted()) {
+            ++numDeletedItems;
+        } else {
+            ++datatypeCounts[v.getDatatype()];
+        }
     }
-    if (v->isDeleted()) {
-        ++numDeletedItems;
-    } else {
-        ++datatypeCounts[v->getDatatype()];
-    }
-    values[hbl.getBucketNum()] = std::move(v);
-
-    return values[hbl.getBucketNum()].get();
 }
 
 std::pair<StoredValue*, StoredValue::UniquePtr>
@@ -365,53 +376,26 @@ HashTable::unlocked_replaceByCopy(const HashBucketLock& hbl,
     /* Copy the StoredValue and link it into the head of the bucket chain. */
     auto newSv = valFact->copyStoredValue(
             vToCopy, std::move(values[hbl.getBucketNum()]));
-    increaseMetaDataSize(stats, newSv->metaDataSize());
-    increaseCacheSize(newSv->size());
 
-    if (newSv->isTempItem()) {
-        ++numTempItems;
-    } else {
-        ++numItems;
-        ++numTotalItems;
-    }
-    if (newSv->isDeleted()) {
-        ++numDeletedItems;
-    } else {
-        ++datatypeCounts[newSv->getDatatype()];
-    }
-    values[hbl.getBucketNum()] = std::move(newSv);
+    // Adding a new item into the HashTable; update stats.
+    statsEpilogue(*newSv);
 
+    values[hbl.getBucketNum()] = std::move(newSv);
     return {values[hbl.getBucketNum()].get(), std::move(releasedSv)};
 }
 
 void HashTable::unlocked_softDelete(const std::unique_lock<std::mutex>& htLock,
                                     StoredValue& v,
                                     bool onlyMarkDeleted) {
-    const bool alreadyDeleted = v.isDeleted();
-    if (!v.isResident() && !v.isDeleted() && !v.isTempItem()) {
-        decrNumNonResidentItems();
-    }
-
-    if (!alreadyDeleted) {
-        --datatypeCounts[v.getDatatype()];
-    }
+    statsPrologue(v);
 
     if (onlyMarkDeleted) {
         v.markDeleted();
     } else {
-        if (v.isTempItem()) {
-            --numTempItems;
-            ++numItems;
-            ++numTotalItems;
-        }
-        size_t len = v.valuelen();
-        if (v.del()) {
-            reduceCacheSize(len);
-        }
-    }
-    if (!alreadyDeleted) {
-        ++numDeletedItems;
+        v.del();
     }
+
+    statsEpilogue(v);
 }
 
 StoredValue* HashTable::unlocked_find(const DocKey& key,
@@ -464,20 +448,9 @@ StoredValue::UniquePtr HashTable::unlocked_release(
                 "not found in HashTable; possibly HashTable leak");
     }
 
-    // Update statistics now the item has been removed.
-    reduceCacheSize(released->size());
-    reduceMetaDataSize(stats, released->metaDataSize());
-    if (released->isTempItem()) {
-        --numTempItems;
-    } else {
-        decrNumItems();
-        decrNumTotalItems();
-        if (released->isDeleted()) {
-            --numDeletedItems;
-        } else {
-            --datatypeCounts[released->getDatatype()];
-        }
-    }
+    // Update statistics for the item which is now gone.
+    statsPrologue(*released);
+
     return released;
 }
 
@@ -829,12 +802,6 @@ void HashTable::reduceMetaDataSize(EPStats &st, size_t by) {
     st.currentSize.fetch_sub(by);
 }
 
-void HashTable::setValue(const Item& itm, StoredValue& v) {
-    reduceCacheSize(v.size());
-    v.setValue(itm);
-    increaseCacheSize(v.size());
-}
-
 std::ostream& operator<<(std::ostream& os, const HashTable& ht) {
     os << "HashTable[" << &ht << "] with"
        << " numItems:" << ht.getNumItems()

engines/ep/src/hash_table.h

@@ -614,18 +614,35 @@ class HashTable {
 
 private:
     /**
-     * Set item into StoredValue.
-     * WARNING: Correct use of this method requires care, it directly updates
-     * the state of a StoredValue without updating the related HashTable
-     * counters, callers are responsible for ensuring the various counters
-     * (numDeletedItems, numNonResident, etc) are correct after calling this
-     * method.
-     *
-     * @param itm the Item to store
-     * @param   v the stored value in which itm needs
-     *            to be stored
-     */
-    void setValue(const Item& itm, StoredValue& v);
+     * Update HashTable statistics before modifying a StoredValue.
+     *
+     * This function should be called before modifying *any* StoredValue object,
+     * if modifying it may affect any of the HashTable counts.
+     * For example, before we remove a StoredValue we must decrement any counts
+     * which it matches; or before we change its datatype we must decrement the
+     * count of the old datatype.
+     *
+     * It is typically paired with statsPrologue if modifying an existing SV.
+     * It will be used by itself if removing a SV (as there will be no SV
+     * after to call statsEpilogue() with).
+     *
+     * See also: statsEpilogue().
+     * @param sv StoredValue which is about to be modified.
+     */
+    void statsPrologue(const StoredValue& sv);
+
+    /**
+     * Update HashTable statistics after modifying a StoredValue.
+     *
+     * This function should be called after modifying *any* StoredValue object,
+     * if modifying it may affect any of the HashTable counts.
+     * For example, if the datatype of a StoredValue may have changed; then
+     * datatypeCounts needs to be updated.
+     *
+     * See also: statsPrologue().
+     * @param sv StoredValue which has just been modified.
+     */
+    void statsEpilogue(const StoredValue& sv);
 
     // The container for actually holding the StoredValues.
     using table_type = std::vector<StoredValue::UniquePtr>;

engines/ep/src/stored-value.cc

@@ -26,6 +26,7 @@
 
 #include <platform/cb_malloc.h>
 
+const int64_t StoredValue::state_pending_seqno = -2;
 const int64_t StoredValue::state_deleted_key = -3;
 const int64_t StoredValue::state_non_existent_key = -4;
 const int64_t StoredValue::state_temp_init = -5;
@@ -280,6 +281,7 @@ bool StoredValue::deleteImpl() {
 
     resetValue();
     setDatatype(PROTOCOL_BINARY_RAW_BYTES);
+    setPendingSeqno();
 
     setDeletedPriv(true);
     markDirty();

engines/ep/src/stored-value.h

@@ -364,6 +364,14 @@ class StoredValue {
         bySeqno = state_non_existent_key;
     }
 
+    /**
+     * Marks that the item's sequence number is pending (valid, non-temp; but
+     * final value not yet known.
+     */
+    void setPendingSeqno() {
+        bySeqno = state_pending_seqno;
+    }
+
     /**
      * Is this a temporary item created for processing a get-meta request?
      */
@@ -539,6 +547,15 @@ class StoredValue {
      * objects.
      */
 
+    /**
+     * Represents the state when a StoredValue is in the process of having its
+     * seqno updated (and so _will be_ non-temporary; but we don't yet have the
+     * new sequence number. This is unfortunately required due to limitations
+     * in sequence number generation; where we need delete an item in the
+     * HashTable before we know what its sequence number is going to be.
+     */
+    static const int64_t state_pending_seqno;
+
     /// Represents an item that's deleted from memory but present on disk.
     static const int64_t state_deleted_key;
 

engines/ep/tests/module_tests/ephemeral_vb_test.cc

@@ -210,6 +210,7 @@ TEST_F(EphemeralVBucketTest, AddTempItemAndUpdate) {
 
     /* Add temp item */
     EXPECT_EQ(AddStatus::BgFetch, addOneTemp(k));
+    ASSERT_EQ(0, vbucket->getNumItems());
 
     /* Update the temp item (make it non-temp) */
     Item i(k, 0, /*expiry*/ 0, k.data(), k.size());
@@ -232,6 +233,7 @@ TEST_F(EphemeralVBucketTest, AddTempItemAndSoftDelete) {
 
     /* Add temp item */
     EXPECT_EQ(AddStatus::BgFetch, addOneTemp(k));
+    ASSERT_EQ(1, vbucket->getNumTempItems());
 
     /* SoftDelete the temp item (make it non-temp) */
     softDeleteOne(k, MutationStatus::WasClean);