MB-59746 [2/2]: Optimise space needed to store locked_CAS

daverigby · daverigby · commit ea3be68838ff · 2023-11-29T11:45:45.000Z
Optimise the representation of the 'cas' and 'locked_cas' fields, so they always consume 8B in StoredValue, at the cost of some complexity. We need to store one CAS value when the SV is unlocked (common), and two CAS values when locked (rare). We want to dto this without having to unncessarily inflate the size of _all_ StoredValue objects, which the current basic impl does (by always storing both cas and locked_cas). Logically what we want is a the 'cas' member to be variant which is either one or two CAS values: std::variant<uint64_t, CasPair> However this has sizeof(max_sizeof(T...)) + 1 = 17Bytes on 64bit systems. To achive this funcionality using only 8B, we use two optimisations: 1. Use an external variable (casIsSeparate) as the tag of the variant (saves 1 Byte). 2. For the CasPair variant, allocate the CasPair on the heap and just store a ptr to it in the variant (save 8 Bytes). (1) Somewhat complicates the code, as we need to manage the tag manually - but that's the price we pay for avoiding the extra byte std::variant would have.. (2) comes at the cost of an additional 16B heap allocation for locked documents (in addition to the fixed 8B footprint of this field), but given locked documents are very rare compared to unlocked ones, this should overall be cheaper than spending 16B on every SV. Change-Id: I8b95a743ef041af3d5566d13087c60213811e359 Reviewed-on: https://review.couchbase.org/c/kv_engine/+/201511 Reviewed-by: Vesko Karaganev <vesko.karaganev@couchbase.com> Tested-by: Dave Rigby <daver@couchbase.com> Well-Formed: Restriction Checker
diff --git a/engines/ep/src/stored-value.cc b/engines/ep/src/stored-value.cc
@@ -36,15 +36,15 @@ StoredValue::StoredValue(const Item& itm,
                          bool isOrdered)
     : value(itm.getValue()),
       chain_next_or_replacement(std::move(n)),
-      cas(itm.getCas()),
       bySeqno(itm.getBySeqno()),
       exptime(itm.getExptime()),
       flags(itm.getFlags()),
       revSeqno(itm.getRevSeqno()),
       datatype(itm.getDataType()),
       ordered(isOrdered),
       deletionSource(0),
-      committed(static_cast<uint8_t>(CommittedState::CommittedViaMutation)) {
+      committed(static_cast<uint8_t>(CommittedState::CommittedViaMutation)),
+      casIsSeparate(0) {
     // Initialise bit fields
     setDeletedPriv(itm.isDeleted());
     setResident(!isTempItem());
@@ -53,6 +53,8 @@ StoredValue::StoredValue(const Item& itm,
     setAge(0);
     // dirty initialised below
 
+    setCas(itm.getCas());
+
     // Placement-new the key which lives in memory directly after this
     // object.
     new (key()) SerialisedDocKey(itm.getKey());
@@ -76,26 +78,30 @@ StoredValue::StoredValue(const Item& itm,
 
 StoredValue::~StoredValue() {
     ObjectRegistry::onDeleteStoredValue(this);
+    // If still has a locked CAS, must clear this to delete the CasPair before
+    // destroying StoredValue.
+    clearLockedCas();
 }
 
 StoredValue::StoredValue(const StoredValue& other, UniquePtr n, EPStats& stats)
     : value(other.value), // Implicitly also copies the frequency counter
       chain_next_or_replacement(std::move(n)),
-      cas(other.cas),
       bySeqno(other.bySeqno),
       lock_expiry_or_delete_or_complete_time(
               other.lock_expiry_or_delete_or_complete_time),
       exptime(other.exptime),
       flags(other.flags),
       revSeqno(other.revSeqno),
       datatype(other.datatype),
-      ordered(other.ordered) {
+      ordered(other.ordered),
+      casIsSeparate(0) {
     setDirty(other.isDirty());
     setDeletedPriv(other.isDeleted());
     setResident(other.isResident());
     setStale(false);
     setCommitted(other.getCommitted());
     setAge(0);
+    setCas(other.getCas());
     // Placement-new the key which lives in memory directly after this
     // object.
     StoredDocKey sKey(other.getKey());
@@ -122,7 +128,7 @@ void StoredValue::ejectValue() {
 
 void StoredValue::restoreValue(const Item& itm) {
     if (isTempInitialItem() || isTempDeletedItem()) {
-        cas = itm.getCas();
+        setCas(itm.getCas());
         flags = itm.getFlags();
         exptime = itm.getExptime();
         revSeqno = itm.getRevSeqno();
@@ -144,7 +150,7 @@ void StoredValue::restoreValue(const Item& itm) {
 }
 
 void StoredValue::restoreMeta(const Item& itm) {
-    cas = itm.getCas();
+    setCas(itm.getCas());
     flags = itm.getFlags();
     datatype = itm.getDataType();
     exptime = itm.getExptime();
@@ -158,6 +164,24 @@ void StoredValue::restoreMeta(const Item& itm) {
     setCommitted(itm.getCommitted());
 }
 
+void StoredValue::lock(rel_time_t expiry, uint64_t lockedCas) {
+    if (isDeleted()) {
+        // Cannot lock Deleted items.
+        throw std::logic_error("StoredValue::lock: Called on Deleted item");
+    }
+    setLockedCas(lockedCas);
+    lock_expiry_or_delete_or_complete_time.lock_expiry = expiry;
+}
+
+void StoredValue::unlock() {
+    if (isDeleted()) {
+        // Deleted items are not locked - just skip.
+        return;
+    }
+    clearLockedCas();
+    lock_expiry_or_delete_or_complete_time.lock_expiry = 0;
+}
+
 size_t StoredValue::uncompressedValuelen() const {
     if (!value) {
         return 0;
@@ -266,7 +290,7 @@ bool StoredValue::operator==(const StoredValue& other) const {
         auto& otherOsv = static_cast<const OrderedStoredValue&>(other);
         orderedEqual = osv.prepareSeqno == otherOsv.prepareSeqno;
     }
-    return (cas == other.cas && revSeqno == other.revSeqno &&
+    return (getCas() == other.getCas() && revSeqno == other.revSeqno &&
             bySeqno == other.bySeqno &&
             lock_expiry_or_delete_or_complete_time.lock_expiry ==
                     other.lock_expiry_or_delete_or_complete_time.lock_expiry &&
@@ -335,7 +359,7 @@ void StoredValue::setValueImpl(const Item& itm) {
     flags = itm.getFlags();
     datatype = itm.getDataType();
     bySeqno = itm.getBySeqno();
-    cas = itm.getCas();
+    setCas(itm.getCas());
     lock_expiry_or_delete_or_complete_time.lock_expiry = 0;
     exptime = itm.getExptime();
     revSeqno = itm.getRevSeqno();
@@ -355,6 +379,57 @@ void StoredValue::setValueImpl(const Item& itm) {
     setCommitted(itm.getCommitted());
 }
 
+StoredValue::CasPair StoredValue::getCasPair() const {
+    switch (getCasEncoding()) {
+    case CasEncoding::InlineSingle:
+        // `cas' encodes the original CAS value, locked CAS is empty.
+        CasPair pair;
+        pair.originalCAS = cas.single;
+        return pair;
+    case CasEncoding::SeparateDouble:
+        // 'cas' points to a CasPair holding two CAS values:
+        return *cas.pair;
+    }
+    folly::assume_unreachable();
+}
+
+void StoredValue::setLockedCas(uint64_t lockedCas) {
+    // Switch to SeparateDouble encoding (if not already)
+    switch (getCasEncoding()) {
+    case CasEncoding::InlineSingle: {
+        // Create separate CasPair.
+        const auto originalCas = cas.single;
+        cas.pair = new CasPair{originalCas, lockedCas};
+        casIsSeparate = 1;
+        return;
+    }
+    case CasEncoding::SeparateDouble:
+        // Format already correct, just update lockedCas.
+        cas.pair->lockedCAS = lockedCas;
+        return;
+    }
+    folly::assume_unreachable();
+}
+
+void StoredValue::clearLockedCas() {
+    // Switch to InlineSingle encoding (if not already)
+    switch (getCasEncoding()) {
+    case CasEncoding::InlineSingle:
+        // Format already correct (double unlock?) -
+        // for robustness allow this.
+        return;
+    case CasEncoding::SeparateDouble: {
+        // Free separate pair, put originalCas back.
+        const auto originalCas = cas.pair->originalCAS;
+        delete cas.pair;
+        cas.single = originalCas;
+        casIsSeparate = 0;
+        return;
+    }
+    }
+    folly::assume_unreachable();
+}
+
 bool StoredValue::compressValue() {
     if (!mcbp::datatype::is_snappy(datatype)) {
         // Attempt compression only if datatype indicates
@@ -395,7 +470,7 @@ std::optional<item_info> StoredValue::getItemInfo(uint64_t vbuuid) const {
     }
 
     item_info info;
-    info.cas = cas;
+    info.cas = getCas();
     info.vbucket_uuid = vbuuid;
     info.seqno = bySeqno;
     info.exptime = exptime;
@@ -446,7 +521,7 @@ void to_json(nlohmann::json& json, const StoredValue& sv) {
         json["prepareSeqno"] = static_cast<uint64_t>(osv.prepareSeqno);
     }
 
-    json["cas"] = sv.cas;
+    json["cas"] = sv.getCas();
     json["rev"] = static_cast<uint64_t>(sv.revSeqno);
     json["seqno"] = sv.bySeqno.load();
     json["l/e/d/c time"] =
@@ -574,8 +649,8 @@ std::ostream& operator<<(std::ostream& os, const StoredValue& sv) {
     os << std::dec << "seq:" << uint64_t(sv.getBySeqno())
        << " rev:" << sv.getRevSeqno();
     os << " cas:" << sv.getCas();
-    if (sv.isLocked(now)) {
-        os << " locked_cas:" << sv.locked_cas;
+    if (sv.getCasEncoding() == StoredValue::CasEncoding::SeparateDouble) {
+        os << " locked_cas:" << sv.getCasPair().lockedCAS;
     }
     os << " key:\"" << sv.getKey() << "\"";
     if (sv.isOrdered() && sv.isDeleted()) {
diff --git a/engines/ep/src/stored-value.h b/engines/ep/src/stored-value.h
@@ -451,7 +451,7 @@ class StoredValue {
      * @return the cas ID
      */
     uint64_t getCas() const {
-        return cas;
+        return getCasPair().originalCAS;
     }
 
     /**
@@ -462,44 +462,37 @@ class StoredValue {
      * @param curtime The current time, used to check if lock has expired
      */
     uint64_t getCasForWrite(rel_time_t curtime) const {
+        auto pair = getCasPair();
         if (isLocked(curtime)) {
-            return locked_cas;
+            return pair.lockedCAS;
         }
-        return cas;
+        return pair.originalCAS;
     }
 
     /**
      * Set a new CAS ID.
      */
     void setCas(uint64_t c) {
-        cas = c;
+        switch (getCasEncoding()) {
+        case CasEncoding::InlineSingle:
+            cas.single = c;
+            break;
+        case CasEncoding::SeparateDouble:
+            cas.pair->originalCAS = c;
+            break;
+        }
     }
 
     /**
      * Lock this item until the given time, setting the locked CAS to
      * the specified value.
      */
-    void lock(rel_time_t expiry, uint64_t cas) {
-        if (isDeleted()) {
-            // Cannot lock Deleted items.
-            throw std::logic_error(
-                    "StoredValue::lock: Called on Deleted item");
-        }
-        lock_expiry_or_delete_or_complete_time.lock_expiry = expiry;
-        locked_cas = cas;
-    }
+    void lock(rel_time_t expiry, uint64_t cas);
 
     /**
      * Unlock this item.
      */
-    void unlock() {
-        if (isDeleted()) {
-            // Deleted items are not locked - just skip.
-            return;
-        }
-        lock_expiry_or_delete_or_complete_time.lock_expiry = 0;
-        locked_cas = 0;
-    }
+    void unlock();
 
     /**
      * True if this item has an ID.
@@ -1016,6 +1009,58 @@ class StoredValue {
         deletionSource = static_cast<uint8_t>(delSource);
     }
 
+    /**
+     * How are the CAS value(s) currently encoded?
+     */
+    enum class CasEncoding {
+        // Single CAS stored inline in the StoredValue.
+        InlineSingle,
+        // Double CAS (original and locked) stored separarely in a
+        // heap-allocated CasPair, StoredValue holds a ptr to them.
+        SeparateDouble,
+    };
+
+    /**
+     * When a document is locked, holds both the CAS of the last mutation
+     * (the pre-lock, original CAS), and the CAS generated by
+     * VBucket::getLocked() which is needed to mutate the document while locked
+     * or unlock it.
+     *
+     * This type is used only for documents which have been locked, and is
+     * heap-allocated and pointed-to by the CasPair* variant of
+     * StoredValue::cas.
+     */
+    struct CasPair {
+        uint64_t originalCAS;
+        uint64_t lockedCAS;
+    };
+
+    /// Returns the current encoding of the CAS value(s).
+    CasEncoding getCasEncoding() const {
+        return casIsSeparate ? CasEncoding::SeparateDouble
+                             : CasEncoding::InlineSingle;
+    }
+
+    /**
+     * Returns both CAS values associated with the StoredValue; the
+     * original CAS and the locked CAS.
+     * If the document doesn't currently have two CAS values (i.e. was not
+     * locked) then lockedCAS will be zero.
+     */
+    CasPair getCasPair() const;
+
+    /**
+     * Sets the locked CAS to the specified value. This changes the
+     * CAS encoding to separate double if not already.
+     */
+    void setLockedCas(uint64_t lockedCas);
+
+    /**
+     * Clears (discards) the locked CAS. This changes the CAS encoding
+     * to inline single if not already.
+     */
+    void clearLockedCas();
+
     friend class StoredValueFactory;
 
     /**
@@ -1074,11 +1119,40 @@ class StoredValue {
     // @todo: Re-factoring of the UniquePtr management is needed to safely use
     // the tag.
     UniquePtr chain_next_or_replacement; // 8 bytes (2-byte tag, 6 byte address)
-    uint64_t           cas;            //!< CAS identifier.
-    // The CAS to use to modify/unlock a locked document. Stored separately from
-    // 'cas' as we must preserve the original CAS in the event the locked
-    // document is unlocked without being modified.
-    uint64_t locked_cas;
+
+    // Space-optimised representation of one or two CAS values. Always consumes
+    // 8B in StoredValue, at the cost of some complexity.
+    //
+    // We need to store one CAS value when the SV is unlocked (common), and two
+    // CAS values when locked (rare). We want to do this without having to
+    // unncessarily inflate the size of _all_ StoredValue objects, which would
+    // occur with a basic implementation along the lines of:
+    //
+    //     std::variant<uint64_t, CasPair>
+    //
+    // which has sizeof(max_sizeof(T...)) + 1 = 17Bytes on 64bit systems.
+    // To achive this funcionality using only 8B, we use two optimisations:
+    // 1. Use an external variable (casIsSeparate) as the tag of the variant
+    //    (saves 1 Byte).
+    // 2. For the CasPair variant, allocate the CasPair on the heap and just
+    //    store a ptr to it in the variant (save 8 Bytes).
+    //
+    // (1) Somewhat complicates the code, as we need to manage the tag manually
+    //  - but that's the price we pay for avoiding the extra byte std::variant
+    // would have..
+    // (2) comes at the cost of an additional 16B heap allocation for locked
+    // documents (in addition to the fixed 8B footprint of this field), but
+    // given locked documents are very rare compared to unlocked ones, this
+    // should overall be cheaper than spending 16B on every SV.
+    union Cas {
+        uint64_t single;
+        CasPair* pair;
+
+        Cas() : single(0) {}
+        Cas(uint64_t c) : single(c) {
+        }
+    } cas;
+
     // bySeqno is atomic primarily for TSAN, which would flag that we write/read
     // this in ephemeral backfills with different locks (which is true, but the
     // access is we believe actually safe)
@@ -1139,6 +1213,9 @@ class StoredValue {
     uint8_t deletionSource : 1;
     /// 3-bit value which encodes the CommittedState of the StoredValue
     uint8_t committed : 3;
+    // If set then CAS is currently encoded as SeparateDouble, if clear
+    // then CAS currently encoded as InlineSingle.
+    uint8_t casIsSeparate : 1;
 
     friend std::ostream& operator<<(std::ostream& os, const StoredValue& sv);
     friend void to_json(nlohmann::json& json, const StoredValue& sv);
diff --git a/engines/ep/tests/module_tests/stored_value_test.cc b/engines/ep/tests/module_tests/stored_value_test.cc
