Move OSV::stale back to StoredValue

As part of MB-23795 it was necessary to move the OrderedStoredValue
{stale} flag out of the packed bitfields in StoredValue so it didn't
share its byte with any other data not guarded by writeLock. This was
to prevent any data races, as C++/x86_64 only exposes byte-level
addressing and if it remained in the bitfield we'd see races when
other elements in the bitfield (e.g. deleted) were changed.

While fixing the correctness issue, this had the consequence of
bloating the size of OrderedStoredValue by 8 bytes; as OSV was
previously an exact multiple of 8 bytes in size, and adding one more
byte for the bitfield forced the object size to increase by 8 bytes to
maintain ABI alignment rules.

However, we actually /do/ have a spare byte in StoredValue after the
bitfields. If we move the stale flag back to SV, but in its own byte
then we recover the size bloat of OSV - we are back to the original SV
and OSV sizes before MB-23795.

Change-Id: I15d299dcdd0107915c8b50c717305e2ecb9960a4
Reviewed-on: http://review.couchbase.org/76774
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Manu Dhundi <manu@couchbase.com>

diff --git a/src/stored-value.h b/src/stored-value.h
index 33636ca..e8515bf 100644 (file)
--- a/src/stored-value.h
+++ b/src/stored-value.h
@@ -642,7 +642,8 @@ protected:
           deleted(itm.isDeleted()),
           newCacheItem(true),
           isOrdered(isOrdered),
-          nru(itm.getNRUValue()) {
+          nru(itm.getNRUValue()),
+          stale(false) {
         // Placement-new the key which lives in memory directly after this
         // object.
         new (key()) SerialisedDocKey(itm.getKey());
@@ -698,7 +699,8 @@ protected:
           deleted(other.deleted),
           newCacheItem(other.newCacheItem),
           isOrdered(other.isOrdered),
-          nru(other.nru) {
+          nru(other.nru),
+          stale(false) {
         // Placement-new the key which lives in memory directly after this
         // object.
         StoredDocKey sKey(other.getKey());
@@ -745,6 +747,20 @@ protected:
     bool               newCacheItem : 1;
     const bool isOrdered : 1; //!< Is this an instance of OrderedStoredValue?
     uint8_t            nru       :  2; //!< True if referenced since last sweep
+    bool unused : 2; // Unused bits in first byte of bitfields.
+
+    // Indicates if a newer instance of the item is added. Logically part of
+    // OSV, but is physically located in SV as there are spare bytes here.
+    // Guarded by the SequenceList's writeLock.
+    // NOTE: As this is guarded by a different lock to the rest of the SV,
+    // it *must* be in a different byte than any other data not guarded by
+    // writeLock (Hence why this isn't in the same byte as _isDirty, deleted,
+    // newCacheItem etc). To achieve this std::atomic is used to ensure accesses
+    // are not "optimized" and merged with the previous byte. The thread-safety
+    // of std::atomic is not actually used/needed; we just need the no-merge
+    // guarantee.
+    // Note (2): Only 1 bit of this is currently used; rest is "spare".
+    std::atomic<bool> stale;
 
     static void increaseMetaDataSize(HashTable &ht, EPStats &st, size_t by);
     static void reduceMetaDataSize(HashTable &ht, EPStats &st, size_t by);
@@ -830,8 +846,7 @@ private:
                        UniquePtr n,
                        EPStats& stats,
                        HashTable& ht)
-        : StoredValue(itm, std::move(n), stats, ht, /*isOrdered*/ true),
-          stale(false) {
+        : StoredValue(itm, std::move(n), stats, ht, /*isOrdered*/ true) {
     }
 
     // Copy Constructor. Private, as needs to be carefully created via
@@ -844,7 +859,7 @@ private:
                        UniquePtr n,
                        EPStats& stats,
                        HashTable& ht)
-        : StoredValue(other, std::move(n), stats, ht), stale(false) {
+        : StoredValue(other, std::move(n), stats, ht) {
     }
 
     /* Do not allow assignment */
@@ -856,10 +871,6 @@ private:
     // Grant friendship to base class so it can perform flag dispatch to our
     // overridden protected methods.
     friend class StoredValue;
-
-    // indicates if a newer instance of the item is added. Guarded by the
-    // SequenceList's writeLock.
-    bool stale : 1;
 };
 
 SerialisedDocKey* StoredValue::key() {

diff --git a/tests/module_tests/stored_value_test.cc b/tests/module_tests/stored_value_test.cc
index 8fd71d9..78f10a2 100644 (file)
--- a/tests/module_tests/stored_value_test.cc
+++ b/tests/module_tests/stored_value_test.cc
@@ -139,17 +139,10 @@ TEST(StoredValueTest, expectedSize) {
 }
 
 TEST(OrderedStoredValueTest, expectedSize) {
-    // TODO-PERF: Ideally should be 72 - this is 63 bits larger than
-    // actually used due to moving {stale} from StoredValue's packed
-    // bitfields to OrderedStoredValue to prevent a race on a bitfield
-    // (TSan reports race between {stale} and {dirty} for example if they
-    // reside in the same byte).
-    // Better solution would be to pack this into the boost_list hook,
-    // or introduce per-OSV microlock to guard the whole object.
-    EXPECT_EQ(80, sizeof(OrderedStoredValue))
+    EXPECT_EQ(72, sizeof(OrderedStoredValue))
             << "Unexpected change in OrderedStoredValue fixed size";
     auto item = make_item(0, makeStoredDocKey("k"), "v");
-    EXPECT_EQ(83, OrderedStoredValue::getRequiredStorage(item))
+    EXPECT_EQ(75, OrderedStoredValue::getRequiredStorage(item))
             << "Unexpected change in OrderedStoredValue storage size for item: "
             << item;
 }