From f8ba4f6c1c910aaed558f155421137527d3ebfb9 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 25 Nov 2022 20:56:16 -0800 Subject: [PATCH] Eliminate memory allocation from critical path: When writing objects to the NodeStore, we need to convert them from the in-memory format to the binary format used by the node store. The conversion is handled by the `EncodedBlob` class, which is only instantianted on the stack. Coupled with the fact that most objects are under 1024 bytes in size, this presents an opportunity to elide a memory allocation in a critical path. This commit also makes the interface of `EncodedBlob` simpler while also eliminating a subtle corner case that could result in dangling pointers. --- Builds/CMake/RippledCore.cmake | 1 - .../nodestore/backend/CassandraFactory.cpp | 8 +- src/ripple/nodestore/backend/NuDBFactory.cpp | 3 +- .../nodestore/backend/RocksDBFactory.cpp | 4 +- src/ripple/nodestore/impl/EncodedBlob.cpp | 42 ------- src/ripple/nodestore/impl/EncodedBlob.h | 103 +++++++++++++++--- src/test/nodestore/Basics_test.cpp | 3 +- 7 files changed, 92 insertions(+), 72 deletions(-) delete mode 100644 src/ripple/nodestore/impl/EncodedBlob.cpp diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 82a57995a4c..d3f0df89f38 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -530,7 +530,6 @@ target_sources (rippled PRIVATE src/ripple/nodestore/impl/DeterministicShard.cpp src/ripple/nodestore/impl/DecodedBlob.cpp src/ripple/nodestore/impl/DummyScheduler.cpp - src/ripple/nodestore/impl/EncodedBlob.cpp src/ripple/nodestore/impl/ManagerImp.cpp src/ripple/nodestore/impl/NodeObject.cpp src/ripple/nodestore/impl/Shard.cpp diff --git a/src/ripple/nodestore/backend/CassandraFactory.cpp b/src/ripple/nodestore/backend/CassandraFactory.cpp index c8d0c139c44..d13cd71827b 100644 --- a/src/ripple/nodestore/backend/CassandraFactory.cpp +++ b/src/ripple/nodestore/backend/CassandraFactory.cpp @@ -670,7 +670,7 @@ class CassandraBackend : public Backend // confirmed persisted. Otherwise, it can become deleted // prematurely if other copies are removed from caches. std::shared_ptr no; - NodeStore::EncodedBlob e; + std::optional e; std::pair compressed; std::chrono::steady_clock::time_point begin; // The data is stored in this buffer. The void* in the above member @@ -686,10 +686,10 @@ class CassandraBackend : public Backend std::atomic& retries) : backend(f), no(nobj), totalWriteRetries(retries) { - e.prepare(no); + e.emplace(no); compressed = - NodeStore::nodeobject_compress(e.getData(), e.getSize(), bf); + NodeStore::nodeobject_compress(e->getData(), e->getSize(), bf); } }; @@ -722,7 +722,7 @@ class CassandraBackend : public Backend CassError rc = cass_statement_bind_bytes( statement, 0, - static_cast(data.e.getKey()), + static_cast(data.e->getKey()), keyBytes_); if (rc != CASS_OK) { diff --git a/src/ripple/nodestore/backend/NuDBFactory.cpp b/src/ripple/nodestore/backend/NuDBFactory.cpp index 2b20b574a2c..30b848e82af 100644 --- a/src/ripple/nodestore/backend/NuDBFactory.cpp +++ b/src/ripple/nodestore/backend/NuDBFactory.cpp @@ -250,8 +250,7 @@ class NuDBBackend : public Backend void do_insert(std::shared_ptr const& no) { - EncodedBlob e; - e.prepare(no); + EncodedBlob e(no); nudb::error_code ec; nudb::detail::buffer bf; auto const result = nodeobject_compress(e.getData(), e.getSize(), bf); diff --git a/src/ripple/nodestore/backend/RocksDBFactory.cpp b/src/ripple/nodestore/backend/RocksDBFactory.cpp index 1a9e529e103..b34560dba89 100644 --- a/src/ripple/nodestore/backend/RocksDBFactory.cpp +++ b/src/ripple/nodestore/backend/RocksDBFactory.cpp @@ -352,11 +352,9 @@ class RocksDBBackend : public Backend, public BatchWriter::Callback assert(m_db); rocksdb::WriteBatch wb; - EncodedBlob encoded; - for (auto const& e : batch) { - encoded.prepare(e); + EncodedBlob encoded(e); wb.Put( rocksdb::Slice( diff --git a/src/ripple/nodestore/impl/EncodedBlob.cpp b/src/ripple/nodestore/impl/EncodedBlob.cpp deleted file mode 100644 index 4ec15b10209..00000000000 --- a/src/ripple/nodestore/impl/EncodedBlob.cpp +++ /dev/null @@ -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 -#include - -namespace ripple { -namespace NodeStore { - -void -EncodedBlob::prepare(std::shared_ptr const& object) -{ - m_key = object->getHash().begin(); - - auto ret = m_data.alloc(object->getData().size() + 9); - - // the first 8 bytes are unused - std::memset(ret, 0, 8); - - ret[8] = static_cast(object->getType()); - - std::memcpy(ret + 9, object->getData().data(), object->getData().size()); -} - -} // namespace NodeStore -} // namespace ripple diff --git a/src/ripple/nodestore/impl/EncodedBlob.h b/src/ripple/nodestore/impl/EncodedBlob.h index 2094b52d338..77f8fbf3c6c 100644 --- a/src/ripple/nodestore/impl/EncodedBlob.h +++ b/src/ripple/nodestore/impl/EncodedBlob.h @@ -22,42 +22,109 @@ #include #include -#include +#include +#include +#include +#include +#include namespace ripple { namespace NodeStore { -/** Utility for producing flattened node objects. - @note This defines the database format of a NodeObject! -*/ -// VFALCO TODO Make allocator aware and use short_alloc -struct EncodedBlob +/** Convert a NodeObject from in-memory to database format. + + The (suboptimal) database format consists of: + + - 8 prefix bytes which will typically be 0, but don't assume that's the + case; earlier versions of the code would use these bytes to store the + ledger index either once or twice. + - A single byte denoting the type of the object. + - The payload. + + @note This class is typically instantiated on the stack, so the size of + the object does not matter as much as it normally would since the + allocation is, effectively, free. + + We leverage that fact to preallocate enough memory to handle most + payloads as part of this object, eliminating the need for dynamic + allocation. As of this writing ~94% of objects require fewer than + 1024 payload bytes. + */ + +class EncodedBlob { + /** The 32-byte key of the serialized object. */ + std::array key_; + + /** A pre-allocated buffer for the serialized object. + + The buffer is large enough for the 9 byte prefix and at least + 1024 more bytes. The precise size is calculated automatically + at compile time so as to avoid wasting space on padding bytes. + */ + std::array< + std::uint8_t, + boost::alignment::align_up(9 + 1024, alignof(std::uint32_t))> + payload_; + + /** The size of the serialized data. */ + std::uint32_t size_; + + /** A pointer to the serialized data. + + This may point to the pre-allocated buffer (if it is sufficiently + large) or to a dynamically allocated buffer. + */ + std::uint8_t* const ptr_; + public: - void - prepare(std::shared_ptr const& object); + explicit EncodedBlob(std::shared_ptr const& obj) + : size_([&obj]() { + assert(obj); + + if (!obj) + throw std::runtime_error( + "EncodedBlob: unseated std::shared_ptr used."); + + return obj->getData().size() + 9; + }()) + , ptr_( + (size_ <= payload_.size()) ? payload_.data() + : new std::uint8_t[size_]) + { + std::fill_n(ptr_, 8, std::uint8_t{0}); + ptr_[8] = static_cast(obj->getType()); + std::copy_n(obj->getData().data(), obj->getData().size(), ptr_ + 9); + std::copy_n(obj->getHash().data(), obj->getHash().size(), key_.data()); + } - void const* + ~EncodedBlob() + { + assert( + ((ptr_ == payload_.data()) && (size_ <= payload_.size())) || + ((ptr_ != payload_.data()) && (size_ > payload_.size()))); + + if (ptr_ != payload_.data()) + delete[] ptr_; + } + + [[nodiscard]] void const* getKey() const noexcept { - return m_key; + return static_cast(key_.data()); } - std::size_t + [[nodiscard]] std::size_t getSize() const noexcept { - return m_data.size(); + return size_; } - void const* + [[nodiscard]] void const* getData() const noexcept { - return reinterpret_cast(m_data.data()); + return static_cast(ptr_); } - -private: - void const* m_key; - Buffer m_data; }; } // namespace NodeStore diff --git a/src/test/nodestore/Basics_test.cpp b/src/test/nodestore/Basics_test.cpp index e9911980b7f..92f2ae15aaf 100644 --- a/src/test/nodestore/Basics_test.cpp +++ b/src/test/nodestore/Basics_test.cpp @@ -56,10 +56,9 @@ class NodeStoreBasic_test : public TestBase auto batch = createPredictableBatch(numObjectsToTest, seedValue); - EncodedBlob encoded; for (int i = 0; i < batch.size(); ++i) { - encoded.prepare(batch[i]); + EncodedBlob encoded(batch[i]); DecodedBlob decoded( encoded.getKey(), encoded.getData(), encoded.getSize());