Simplify SHAMapItem construction:

The existing class offered several constructors which were mostly
unnecessary. This commit eliminates all existing constructors and
introduces a single new one, taking a `Slice`.

The internal buffer is switched from `std::vector` to `Buffer` to
save a minimum of 8 bytes (plus the buffer slack that is inherent
in `std::vector`) per SHAMapItem instance.

Builds/CMake/RippledCore.cmake

@@ -650,7 +650,6 @@ target_sources (rippled PRIVATE
   src/ripple/shamap/impl/SHAMap.cpp
   src/ripple/shamap/impl/SHAMapDelta.cpp
   src/ripple/shamap/impl/SHAMapInnerNode.cpp
-  src/ripple/shamap/impl/SHAMapItem.cpp
   src/ripple/shamap/impl/SHAMapLeafNode.cpp
   src/ripple/shamap/impl/SHAMapNodeID.cpp
   src/ripple/shamap/impl/SHAMapSync.cpp

src/ripple/app/consensus/RCLConsensus.cpp

@@ -316,7 +316,7 @@ RCLConsensus::Adaptor::onClose(
         tx.first->add(s);
         initialSet->addItem(
             SHAMapNodeType::tnTRANSACTION_NM,
-            SHAMapItem(tx.first->getTransactionID(), std::move(s)));
+            SHAMapItem(tx.first->getTransactionID(), s.slice()));
     }
 
     // Add pseudo-transactions to the set

src/ripple/app/consensus/RCLCxTx.h

@@ -91,8 +91,7 @@ public:
         insert(Tx const& t)
         {
             return map_->addItem(
-                SHAMapNodeType::tnTRANSACTION_NM,
+                SHAMapNodeType::tnTRANSACTION_NM, SHAMapItem{t.tx_});
-                SHAMapItem{t.id(), t.tx_.peekData()});
         }
 
         /** Remove a transaction from the set.

src/ripple/app/ledger/Ledger.cpp

@@ -371,7 +371,8 @@ Ledger::setAccepted(
 bool
 Ledger::addSLE(SLE const& sle)
 {
-    SHAMapItem item(sle.key(), sle.getSerializer());
+    auto const s = sle.getSerializer();
+    SHAMapItem item(sle.key(), s.slice());
     return stateMap_->addItem(SHAMapNodeType::tnACCOUNT_STATE, std::move(item));
 }
 
@@ -438,8 +439,7 @@ Ledger::read(Keylet const& k) const
     auto const& item = stateMap_->peekItem(k.key);
     if (!item)
         return nullptr;
-    auto sle = std::make_shared<SLE>(
+    auto sle = std::make_shared<SLE>(SerialIter{item->slice()}, item->key());
-        SerialIter{item->data(), item->size()}, item->key());
     if (!k.check(*sle))
         return nullptr;
     return sle;
@@ -533,7 +533,7 @@ Ledger::rawInsert(std::shared_ptr<SLE> const& sle)
     sle->add(ss);
     if (!stateMap_->addGiveItem(
             SHAMapNodeType::tnACCOUNT_STATE,
-            std::make_shared<SHAMapItem const>(sle->key(), std::move(ss))))
+            std::make_shared<SHAMapItem const>(sle->key(), ss.slice())))
         LogicError("Ledger::rawInsert: key already exists");
 }
 
@@ -544,7 +544,7 @@ Ledger::rawReplace(std::shared_ptr<SLE> const& sle)
     sle->add(ss);
     if (!stateMap_->updateGiveItem(
             SHAMapNodeType::tnACCOUNT_STATE,
-            std::make_shared<SHAMapItem const>(sle->key(), std::move(ss))))
+            std::make_shared<SHAMapItem const>(sle->key(), ss.slice())))
         LogicError("Ledger::rawReplace: key not found");
 }
 
@@ -562,7 +562,7 @@ Ledger::rawTxInsert(
     s.addVL(metaData->peekData());
     if (!txMap().addGiveItem(
             SHAMapNodeType::tnTRANSACTION_MD,
-            std::make_shared<SHAMapItem const>(key, std::move(s))))
+            std::make_shared<SHAMapItem const>(key, s.slice())))
         LogicError("duplicate_tx: " + to_string(key));
 }
 
@@ -578,9 +578,8 @@ Ledger::rawTxInsertWithHash(
     Serializer s(txn->getDataLength() + metaData->getDataLength() + 16);
     s.addVL(txn->peekData());
     s.addVL(metaData->peekData());
-    auto item = std::make_shared<SHAMapItem const>(key, std::move(s));
+    auto item = std::make_shared<SHAMapItem const>(key, s.slice());
-    auto hash = sha512Half(
+    auto hash = sha512Half(HashPrefix::txNode, item->slice(), item->key());
-        HashPrefix::txNode, makeSlice(item->peekData()), item->key());
     if (!txMap().addGiveItem(SHAMapNodeType::tnTRANSACTION_MD, std::move(item)))
         LogicError("duplicate_tx: " + to_string(key));
 
@@ -647,8 +646,7 @@ Ledger::peek(Keylet const& k) const
     auto const& value = stateMap_->peekItem(k.key);
     if (!value)
         return nullptr;
-    auto sle = std::make_shared<SLE>(
+    auto sle = std::make_shared<SLE>(SerialIter{value->slice()}, value->key());
-        SerialIter{value->data(), value->size()}, value->key());
     if (!k.check(*sle))
         return nullptr;
     return sle;

src/ripple/app/ledger/LedgerHistory.cpp

@@ -411,7 +411,7 @@ LedgerHistory::handleMismatch(
         }
         else
         {
-            if ((*b)->peekData() != (*v)->peekData())
+            if ((*b)->slice() != (*v)->slice())
             {
                 // Same transaction with different metadata
                 log_metadata_difference(

src/ripple/app/ledger/impl/LedgerReplayMsgHandler.cpp

@@ -264,8 +264,8 @@ LedgerReplayMsgHandler::processReplayDeltaResponse(
             STObject meta(metaSit, sfMetadata);
             orderedTxns.emplace(meta[sfTransactionIndex], std::move(tx));
 
-            auto item = std::make_shared<SHAMapItem const>(
+            auto item =
-                tid, std::move(shaMapItemData));
+                std::make_shared<SHAMapItem const>(tid, shaMapItemData.slice());
             if (!item ||
                 !txMap.addGiveItem(SHAMapNodeType::tnTRANSACTION_MD, item))
             {

src/ripple/app/ledger/impl/SkipListAcquire.cpp

@@ -147,8 +147,8 @@ SkipListAcquire::processData(
     JLOG(journal_.trace()) << "got data for " << hash_;
     try
     {
-        if (auto sle = std::make_shared<SLE>(
+        if (auto sle =
-                SerialIter{item->data(), item->size()}, item->key());
+                std::make_shared<SLE>(SerialIter{item->slice()}, item->key());
             sle)
         {
             if (auto const& skipList = sle->getFieldV256(sfHashes).value();

src/ripple/app/ledger/impl/TransactionMaster.cpp

@@ -123,7 +123,7 @@ TransactionMaster::fetch(
         }
         else if (type == SHAMapNodeType::tnTRANSACTION_MD)
         {
-            auto blob = SerialIter{item->data(), item->size()}.getVL();
+            auto blob = SerialIter{item->slice()}.getVL();
             txn = std::make_shared<STTx const>(
                 SerialIter{blob.data(), blob.size()});
         }

src/ripple/app/misc/AmendmentTable.h

@@ -157,7 +157,7 @@ public:
             initialPosition->addGiveItem(
                 SHAMapNodeType::tnTRANSACTION_NM,
                 std::make_shared<SHAMapItem>(
-                    amendTx.getTransactionID(), s.peekData()));
+                    amendTx.getTransactionID(), s.slice()));
         }
     }
 };

src/ripple/app/misc/FeeVoteImpl.cpp

@@ -246,7 +246,7 @@ FeeVoteImpl::doVoting(
 
         if (!initialPosition->addGiveItem(
                 SHAMapNodeType::tnTRANSACTION_NM,
-                std::make_shared<SHAMapItem>(txID, s.peekData())))
+                std::make_shared<SHAMapItem>(txID, s.slice())))
         {
             JLOG(journal_.warn()) << "Ledger already had fee change";
         }

src/ripple/app/misc/NegativeUNLVote.cpp

@@ -120,7 +120,7 @@ NegativeUNLVote::addTx(
     negUnlTx.add(s);
     if (!initialSet->addGiveItem(
             SHAMapNodeType::tnTRANSACTION_NM,
-            std::make_shared<SHAMapItem>(txID, s.peekData())))
+            std::make_shared<SHAMapItem>(txID, s.slice())))
     {
         JLOG(j_.warn()) << "N-UNL: ledger seq=" << seq
                         << ", add ttUNL_MODIFY tx failed";

src/ripple/protocol/Serializer.h

@@ -102,6 +102,8 @@ public:
     int
     addRaw(Blob const& vector);
     int
+    addRaw(Slice slice);
+    int
     addRaw(const void* ptr, int len);
     int
     addRaw(const Serializer& s);

src/ripple/protocol/impl/Serializer.cpp

@@ -104,6 +104,14 @@ Serializer::addRaw(Blob const& vector)
     return ret;
 }
 
+int
+Serializer::addRaw(Slice slice)
+{
+    int ret = mData.size();
+    mData.insert(mData.end(), slice.begin(), slice.end());
+    return ret;
+}
+
 int
 Serializer::addRaw(const Serializer& s)
 {

src/ripple/shamap/SHAMapAccountStateLeafNode.h

@@ -67,14 +67,14 @@ public:
     void
     updateHash() final override
     {
-        hash_ = SHAMapHash{sha512Half(
+        hash_ = SHAMapHash{
-            HashPrefix::leafNode, makeSlice(item_->peekData()), item_->key())};
+            sha512Half(HashPrefix::leafNode, item_->slice(), item_->key())};
     }
 
     void
     serializeForWire(Serializer& s) const final override
     {
-        s.addRaw(item_->peekData());
+        s.addRaw(item_->slice());
         s.addBitString(item_->key());
         s.add8(wireTypeAccountState);
     }
@@ -83,7 +83,7 @@ public:
     serializeWithPrefix(Serializer& s) const final override
     {
         s.add32(HashPrefix::leafNode);
-        s.addRaw(item_->peekData());
+        s.addRaw(item_->slice());
         s.addBitString(item_->key());
     }
 };

src/ripple/shamap/SHAMapItem.h

@@ -20,14 +20,10 @@
 #ifndef RIPPLE_SHAMAP_SHAMAPITEM_H_INCLUDED
 #define RIPPLE_SHAMAP_SHAMAPITEM_H_INCLUDED
 
-#include <ripple/basics/Blob.h>
+#include <ripple/basics/Buffer.h>
 #include <ripple/basics/CountedObject.h>
 #include <ripple/basics/Slice.h>
 #include <ripple/basics/base_uint.h>
-#include <ripple/beast/utility/Journal.h>
-#include <ripple/protocol/Serializer.h>
-
-#include <cstddef>
 
 namespace ripple {
 
@@ -36,60 +32,40 @@ class SHAMapItem : public CountedObject<SHAMapItem>
 {
 private:
     uint256 tag_;
-    Blob data_;
+    Buffer data_;
 
 public:
-    SHAMapItem(uint256 const& tag, Blob const& data);
+    SHAMapItem() = delete;
-    SHAMapItem(uint256 const& tag, Serializer const& s);
-    SHAMapItem(uint256 const& tag, Serializer&& s);
 
-    Slice
+    SHAMapItem(uint256 const& tag, Slice data) : tag_(tag), data_(data)
-    slice() const;
+    {
+    }
 
     uint256 const&
-    key() const;
+    key() const
+    {
+        return tag_;
+    }
 
-    Blob const&
+    Slice
-    peekData() const;
+    slice() const
+    {
+        return static_cast<Slice>(data_);
+    }
 
     std::size_t
-    size() const;
+    size() const
+    {
+        return data_.size();
+    }
+
     void const*
-    data() const;
+    data() const
+    {
+        return data_.data();
+    }
 };
 
-//------------------------------------------------------------------------------
-
-inline Slice
-SHAMapItem::slice() const
-{
-    return {data_.data(), data_.size()};
-}
-
-inline std::size_t
-SHAMapItem::size() const
-{
-    return data_.size();
-}
-
-inline void const*
-SHAMapItem::data() const
-{
-    return data_.data();
-}
-
-inline uint256 const&
-SHAMapItem::key() const
-{
-    return tag_;
-}
-
-inline Blob const&
-SHAMapItem::peekData() const
-{
-    return data_;
-}
-
 }  // namespace ripple
 
 #endif

src/ripple/shamap/SHAMapTreeNode.h

@@ -23,6 +23,7 @@
 #include <ripple/basics/CountedObject.h>
 #include <ripple/basics/TaggedCache.h>
 #include <ripple/beast/utility/Journal.h>
+#include <ripple/protocol/Serializer.h>
 #include <ripple/shamap/SHAMapItem.h>
 #include <ripple/shamap/SHAMapNodeID.h>
 

src/ripple/shamap/SHAMapTxLeafNode.h

@@ -65,14 +65,14 @@ public:
     void
     updateHash() final override
     {
-        hash_ = SHAMapHash{sha512Half(
+        hash_ =
-            HashPrefix::transactionID, makeSlice(item_->peekData()))};
+            SHAMapHash{sha512Half(HashPrefix::transactionID, item_->slice())};
     }
 
     void
     serializeForWire(Serializer& s) const final override
     {
-        s.addRaw(item_->peekData());
+        s.addRaw(item_->slice());
         s.add8(wireTypeTransaction);
     }
 
@@ -80,7 +80,7 @@ public:
     serializeWithPrefix(Serializer& s) const final override
     {
         s.add32(HashPrefix::transactionID);
-        s.addRaw(item_->peekData());
+        s.addRaw(item_->slice());
     }
 };
 

src/ripple/shamap/SHAMapTxPlusMetaLeafNode.h

@@ -66,14 +66,14 @@ public:
     void
     updateHash() final override
     {
-        hash_ = SHAMapHash{sha512Half(
+        hash_ = SHAMapHash{
-            HashPrefix::txNode, makeSlice(item_->peekData()), item_->key())};
+            sha512Half(HashPrefix::txNode, item_->slice(), item_->key())};
     }
 
     void
     serializeForWire(Serializer& s) const final override
     {
-        s.addRaw(item_->peekData());
+        s.addRaw(item_->slice());
         s.addBitString(item_->key());
         s.add8(wireTypeTransactionWithMeta);
     }
@@ -82,7 +82,7 @@ public:
     serializeWithPrefix(Serializer& s) const final override
     {
         s.add32(HashPrefix::txNode);
-        s.addRaw(item_->peekData());
+        s.addRaw(item_->slice());
         s.addBitString(item_->key());
     }
 };

src/ripple/shamap/impl/SHAMapDelta.cpp

@@ -78,7 +78,7 @@ SHAMap::walkBranch(
                 if (--maxCount <= 0)
                     return false;
             }
-            else if (item->peekData() != otherMapItem->peekData())
+            else if (item->slice() != otherMapItem->slice())
             {
                 // non-matching items with same tag
                 if (isFirstMap)
@@ -156,8 +156,7 @@ SHAMap::compare(SHAMap const& otherMap, Delta& differences, int maxCount) const
             auto other = static_cast<SHAMapLeafNode*>(otherNode);
             if (ours->peekItem()->key() == other->peekItem()->key())
             {
-                if (ours->peekItem()->peekData() !=
+                if (ours->peekItem()->slice() != other->peekItem()->slice())
-                    other->peekItem()->peekData())
                 {
                     differences.insert(std::make_pair(
                         ours->peekItem()->key(),

src/ripple/shamap/impl/SHAMapItem.cpp

@@ -1,42 +0,0 @@
-//------------------------------------------------------------------------------
-/*
-    This file is part of rippled: https://github.com/ripple/rippled
-    Copyright (c) 2012, 2013 Ripple Labs Inc.
-
-    Permission to use, copy, modify, and/or distribute this software for any
-    purpose  with  or without fee is hereby granted, provided that the above
-    copyright notice and this permission notice appear in all copies.
-
-    THE  SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
-    WITH  REGARD  TO  THIS  SOFTWARE  INCLUDING  ALL  IMPLIED  WARRANTIES  OF
-    MERCHANTABILITY  AND  FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
-    ANY  SPECIAL ,  DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
-    WHATSOEVER  RESULTING  FROM  LOSS  OF USE, DATA OR PROFITS, WHETHER IN AN
-    ACTION  OF  CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
-    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-*/
-//==============================================================================
-
-#include <ripple/protocol/Serializer.h>
-#include <ripple/shamap/SHAMapItem.h>
-
-namespace ripple {
-
-class SHAMap;
-
-SHAMapItem::SHAMapItem(uint256 const& tag, Blob const& data)
-    : tag_(tag), data_(data)
-{
-}
-
-SHAMapItem::SHAMapItem(uint256 const& tag, const Serializer& data)
-    : tag_(tag), data_(data.peekData())
-{
-}
-
-SHAMapItem::SHAMapItem(uint256 const& tag, Serializer&& data)
-    : tag_(tag), data_(std::move(data.modData()))
-{
-}
-
-}  // namespace ripple

src/ripple/shamap/impl/SHAMapLeafNode.cpp

@@ -28,7 +28,7 @@ SHAMapLeafNode::SHAMapLeafNode(
     std::uint32_t cowid)
     : SHAMapTreeNode(cowid), item_(std::move(item))
 {
-    assert(item_->peekData().size() >= 12);
+    assert(item_->size() >= 12);
 }
 
 SHAMapLeafNode::SHAMapLeafNode(
@@ -37,7 +37,7 @@ SHAMapLeafNode::SHAMapLeafNode(
     SHAMapHash const& hash)
     : SHAMapTreeNode(cowid, hash), item_(std::move(item))
 {
-    assert(item_->peekData().size() >= 12);
+    assert(item_->size() >= 12);
 }
 
 std::shared_ptr<SHAMapItem const> const&

src/ripple/shamap/impl/SHAMapSync.cpp

@@ -697,7 +697,7 @@ SHAMap::deepCompare(SHAMap& other) const
                 static_cast<SHAMapLeafNode*>(otherNode)->peekItem();
             if (nodePeek->key() != otherNodePeek->key())
                 return false;
-            if (nodePeek->peekData() != otherNodePeek->peekData())
+            if (nodePeek->slice() != otherNodePeek->slice())
                 return false;
         }
         else if (node->isInner())

src/ripple/shamap/impl/SHAMapTreeNode.cpp

@@ -42,11 +42,8 @@ SHAMapTreeNode::makeTransaction(
     SHAMapHash const& hash,
     bool hashValid)
 {
-    // FIXME: using a Serializer results in a copy; avoid it?
-    Serializer s(data.begin(), data.size());
-
     auto item = std::make_shared<SHAMapItem const>(
-        sha512Half(HashPrefix::transactionID, data), s);
+        sha512Half(HashPrefix::transactionID, data), data);
 
     if (hashValid)
         return std::make_shared<SHAMapTxLeafNode>(std::move(item), 0, hash);
@@ -74,7 +71,7 @@ SHAMapTreeNode::makeTransactionWithMeta(
 
     s.chop(tag.bytes);
 
-    auto item = std::make_shared<SHAMapItem const>(tag, s.peekData());
+    auto item = std::make_shared<SHAMapItem const>(tag, s.slice());
 
     if (hashValid)
         return std::make_shared<SHAMapTxPlusMetaLeafNode>(
@@ -106,7 +103,7 @@ SHAMapTreeNode::makeAccountState(
     if (tag.isZero())
         Throw<std::runtime_error>("Invalid AS node");
 
-    auto item = std::make_shared<SHAMapItem const>(tag, s.peekData());
+    auto item = std::make_shared<SHAMapItem const>(tag, s.slice());
 
     if (hashValid)
         return std::make_shared<SHAMapAccountStateLeafNode>(

src/test/app/LedgerReplay_test.cpp

@@ -25,6 +25,7 @@
 #include <ripple/app/ledger/impl/LedgerDeltaAcquire.h>
 #include <ripple/app/ledger/impl/LedgerReplayMsgHandler.h>
 #include <ripple/app/ledger/impl/SkipListAcquire.h>
+#include <ripple/basics/Slice.h>
 #include <ripple/overlay/PeerSet.h>
 #include <ripple/overlay/impl/PeerImp.h>
 #include <test/jtx.h>
@@ -1276,7 +1277,11 @@ struct LedgerReplayer_test : public beast::unit_test::suite
             InboundLedger::Reason::GENERIC, finalHash, totalReplay);
 
         auto skipList = net.client.findSkipListAcquire(finalHash);
-        auto item = std::make_shared<SHAMapItem>(uint256(12345), Blob(55, 55));
+
+        std::uint8_t payload[55] = {
+            0x6A, 0x09, 0xE6, 0x67, 0xF3, 0xBC, 0xC9, 0x08, 0xB2};
+        auto item = std::make_shared<SHAMapItem>(
+            uint256(12345), Slice(payload, sizeof(payload)));
         skipList->processData(l->seq(), item);
 
         std::vector<TaskStatus> deltaStatuses;

src/test/shamap/FetchPack_test.cpp

@@ -91,7 +91,7 @@ public:
         Serializer s;
         for (int d = 0; d < 3; ++d)
             s.add32(ripple::rand_int<std::uint32_t>(r));
-        return std::make_shared<Item>(s.getSHA512Half(), s.peekData());
+        return std::make_shared<Item>(s.getSHA512Half(), s.slice());
     }
 
     void

src/test/shamap/SHAMapSync_test.cpp

@@ -41,8 +41,7 @@ public:
 
         for (int d = 0; d < 3; ++d)
             s.add32(rand_int<std::uint32_t>(eng_));
-
+        return std::make_shared<SHAMapItem>(s.getSHA512Half(), s.slice());
-        return std::make_shared<SHAMapItem>(s.getSHA512Half(), s.peekData());
     }
 
     bool

src/test/shamap/SHAMap_test.cpp

@@ -18,10 +18,11 @@
 //==============================================================================
 
 #include <ripple/basics/Blob.h>
-#include <ripple/basics/StringUtilities.h>
+#include <ripple/basics/Buffer.h>
 #include <ripple/beast/unit_test.h>
 #include <ripple/beast/utility/Journal.h>
 #include <ripple/shamap/SHAMap.h>
+#include <algorithm>
 #include <test/shamap/common.h>
 #include <test/unit_test/SuiteJournal.h>
 
@@ -109,14 +110,11 @@ operator!=(SHAMapItem const& a, uint256 const& b)
 class SHAMap_test : public beast::unit_test::suite
 {
 public:
-    static Blob
+    static Buffer
     IntToVUC(int v)
     {
-        Blob vuc;
+        Buffer vuc(32);
-
+        std::fill_n(vuc.data(), vuc.size(), static_cast<std::uint8_t>(v));
-        for (int i = 0; i < 32; ++i)
-            vuc.push_back(static_cast<unsigned char>(v));
-
         return vuc;
     }
 
@@ -371,8 +369,9 @@ class SHAMapPathProof_test : public beast::unit_test::suite
         for (unsigned char c = 1; c < 100; ++c)
         {
             uint256 k(c);
-            Blob b(32, c);
+            map.addItem(
-            map.addItem(SHAMapNodeType::tnACCOUNT_STATE, SHAMapItem{k, b});
+                SHAMapNodeType::tnACCOUNT_STATE,
+                SHAMapItem{k, Slice{k.data(), k.size()}});
             map.invariants();
 
             auto root = map.getHash().as_uint256();