Skip to content

dbwrapper: reuse scratch DataStream buffers #35156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
achow101 merged 4 commits into from
May 13, 2026
Merged

dbwrapper: reuse scratch DataStream buffers #35156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
31ce729
streams: add `ScopedDataStreamUsage`
l0rinc Apr 24, 2026
cb1ab0a
test: cover repeated dbwrapper stream use
l0rinc Apr 24, 2026
7403c0f
dbwrapper: guard `CDBBatch` scratch streams
l0rinc Apr 24, 2026
032223f
dbwrapper: reuse iterator scratch stream
l0rinc Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/dbwrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ CDBBatch::CDBBatch(const CDBWrapper& _parent)
: parent{_parent},
m_impl_batch{std::make_unique<CDBBatch::WriteBatchImpl>()}
{
m_key_scratch.reserve(DBWRAPPER_PREALLOC_KEY_SIZE);
m_value_scratch.reserve(DBWRAPPER_PREALLOC_VALUE_SIZE);
Clear();
};

Expand All @@ -171,13 +173,15 @@ CDBBatch::~CDBBatch() = default;
void CDBBatch::Clear()
{
m_impl_batch->batch.Clear();
assert(m_key_scratch.empty());
assert(m_value_scratch.empty());
}

void CDBBatch::WriteImpl(std::span<const std::byte> key, DataStream& ssValue)
void CDBBatch::WriteImpl(std::span<const std::byte> key, DataStream& value)
{
leveldb::Slice slKey(CharCast(key.data()), key.size());
dbwrapper_private::GetObfuscation(parent)(ssValue);
leveldb::Slice slValue(CharCast(ssValue.data()), ssValue.size());
dbwrapper_private::GetObfuscation(parent)(value);
leveldb::Slice slValue(CharCast(value.data()), value.size());
m_impl_batch->batch.Put(slKey, slValue);
}

Expand Down Expand Up @@ -356,7 +360,10 @@ struct CDBIterator::IteratorImpl {
};

CDBIterator::CDBIterator(const CDBWrapper& _parent, std::unique_ptr<IteratorImpl> _piter) : parent(_parent),
m_impl_iter(std::move(_piter)) {}
m_impl_iter(std::move(_piter))
{
m_scratch.reserve(DBWRAPPER_PREALLOC_KEY_SIZE);
}

CDBIterator* CDBWrapper::NewIterator()
{
Expand Down
39 changes: 18 additions & 21 deletions src/dbwrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@ class CDBBatch
struct WriteBatchImpl;
const std::unique_ptr<WriteBatchImpl> m_impl_batch;

DataStream ssKey{};
DataStream ssValue{};
DataStream m_key_scratch{};
DataStream m_value_scratch{};

void WriteImpl(std::span<const std::byte> key, DataStream& ssValue);
void WriteImpl(std::span<const std::byte> key, DataStream& value);
void EraseImpl(std::span<const std::byte> key);

public:
Expand All @@ -96,22 +96,18 @@ class CDBBatch
template <typename K, typename V>
void Write(const K& key, const V& value)
{
ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE);
ssValue.reserve(DBWRAPPER_PREALLOC_VALUE_SIZE);
Comment thread
l0rinc marked this conversation as resolved.
ssKey << key;
ssValue << value;
WriteImpl(ssKey, ssValue);
ssKey.clear();
ssValue.clear();
ScopedDataStreamUsage scoped_key{m_key_scratch}, scoped_value{m_value_scratch};
m_key_scratch << key;
m_value_scratch << value;
WriteImpl(m_key_scratch, m_value_scratch);
}

template <typename K>
void Erase(const K& key)
{
ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE);
ssKey << key;
EraseImpl(ssKey);
ssKey.clear();
ScopedDataStreamUsage scoped_key{m_key_scratch};
m_key_scratch << key;
EraseImpl(m_key_scratch);
Comment on lines +108 to +110

@sedited sedited May 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, but mixing between the scoped and member stream here is a bit jarring to read in my opinion. Might it be better to just access the scratch stream through the scoped stream instead?

@achow101 achow101 May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I guess doing so would require implementing all of DataStream's functions as passthroughs which would be a bunch of extra boilerplate.

}

size_t ApproximateSize() const;
Expand All @@ -125,6 +121,7 @@ class CDBIterator
private:
const CDBWrapper &parent;
const std::unique_ptr<IteratorImpl> m_impl_iter;
DataStream m_scratch{};

void SeekImpl(std::span<const std::byte> key);
std::span<const std::byte> GetKeyImpl() const;
Expand All @@ -144,10 +141,9 @@ class CDBIterator
void SeekToFirst();

template<typename K> void Seek(const K& key) {
DataStream ssKey{};
ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE);
ssKey << key;
SeekImpl(ssKey);
ScopedDataStreamUsage scoped_scratch{m_scratch};
m_scratch << key;
SeekImpl(m_scratch);
}

void Next();
Expand All @@ -164,9 +160,10 @@ class CDBIterator

template<typename V> bool GetValue(V& value) {
try {
DataStream ssValue{GetValueImpl()};
dbwrapper_private::GetObfuscation(parent)(ssValue);
ssValue >> value;
ScopedDataStreamUsage scoped_scratch{m_scratch};
m_scratch.write(GetValueImpl());
dbwrapper_private::GetObfuscation(parent)(m_scratch);
m_scratch >> value;
} catch (const std::exception&) {
return false;
}
Expand Down
14 changes: 14 additions & 0 deletions src/streams.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,20 @@ class DataStream
size_t GetMemoryUsage() const noexcept;
};

// Require empty scratch streams on entry and reset them on exit.

@optout21 optout21 May 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

31ce729 streams: add ScopedDataStreamUsage:

Nit: class description. This description is more about what the class does, not about what its purpose is and how it should be used. I suggest something like this:

//! A guard class facilitating the safe temporary 'scratchbook' reuse of
//! a longer-lifetime `DataStream` instance, in order to save on memory allocations.
//! When used in a restricted scope, it ensures/enforces that the buffer is empty
//! at the beginning and at the end, so the instance can be reused repeatedly without
//! the risk of data being mixed between usages.
//! Usages should be non-overlapping, short-lived, and single-threaded.

class ScopedDataStreamUsage

@optout21 optout21 May 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

31ce729 streams: add ScopedDataStreamUsage:

Nit: naming. I find the name not very descriptive (it's rather hard to pinpoint the essence).
My best proposals are "ScratchDataStream" (using the "scratchbook" metaphor), or "DataStreamGuard" (showing that it's a RAII guard).
"Scoped" refers that it should be used in a restricted scope, but it's not very descriptive. The fact that it's a RAII pattern is relevant from implementation perspective, but not much from usage perspective. The "Usage" word is just too vague. The name feels like it was written to describe the class itself rather than what it does for the caller. The caller-facing purpose should drive the name.

@l0rinc l0rinc May 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will adjust if I need to push again

{
DataStream& m_stream;

public:
explicit ScopedDataStreamUsage(DataStream& stream) : m_stream{stream} { assert(m_stream.empty()); }

ScopedDataStreamUsage(const ScopedDataStreamUsage&) = delete;
ScopedDataStreamUsage& operator=(const ScopedDataStreamUsage&) = delete;

~ScopedDataStreamUsage() { m_stream.clear(); }
};

template <typename IStream>
class BitStreamReader
{
Expand Down
27 changes: 26 additions & 1 deletion src/test/dbwrapper_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,13 @@ BOOST_AUTO_TEST_CASE(dbwrapper_batch)

// key3 should've never been written
BOOST_CHECK(dbw.Read(key3, res) == false);

batch.Clear();
batch.Write(key3, in3);
dbw.WriteBatch(batch);

BOOST_CHECK(dbw.Read(key3, res));
BOOST_CHECK_EQUAL(res.ToString(), in3.ToString());
}
}

Expand Down Expand Up @@ -212,18 +219,36 @@ BOOST_AUTO_TEST_CASE(dbwrapper_iterator)
BOOST_CHECK(!it->GetKey(key_too_large));

uint8_t key_res;

BOOST_REQUIRE(it->GetKey(key_res));
BOOST_CHECK_EQUAL(key_res, key);
// A failed value decode must not leave the iterator's scratch stream dirty.
std::pair<uint256, uint8_t> value_too_large;
BOOST_CHECK(!it->GetValue(value_too_large));

uint256 val_res;
BOOST_REQUIRE(it->GetValue(val_res));
BOOST_CHECK_EQUAL(val_res.ToString(), in.ToString());

it->Seek(key2);

BOOST_REQUIRE(it->GetKey(key_res));
BOOST_CHECK_EQUAL(key_res, key2);
BOOST_REQUIRE(it->GetValue(val_res));
BOOST_CHECK_EQUAL(val_res.ToString(), in2.ToString());

it->Seek(key);

BOOST_REQUIRE(it->GetKey(key_res));
BOOST_CHECK_EQUAL(key_res, key);
BOOST_REQUIRE(it->GetValue(val_res));
BOOST_CHECK_EQUAL(val_res.ToString(), in.ToString());

it->Next();

BOOST_REQUIRE(it->GetKey(key_res));
BOOST_REQUIRE(it->GetValue(val_res));
BOOST_CHECK_EQUAL(key_res, key2);
BOOST_REQUIRE(it->GetValue(val_res));
BOOST_CHECK_EQUAL(val_res.ToString(), in2.ToString());

it->Next();
Expand Down
18 changes: 18 additions & 0 deletions src/test/streams_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,24 @@ BOOST_AUTO_TEST_CASE(obfuscation_empty)
BOOST_CHECK(non_null_obf);
}

BOOST_AUTO_TEST_CASE(streams_scoped_data_stream_usage)
{
DataStream stream{};
{
ScopedDataStreamUsage usage{stream};
stream << uint8_t{42};
BOOST_CHECK_GT(stream.size(), 0U);
}
BOOST_CHECK(stream.empty());

{
ScopedDataStreamUsage usage{stream};
stream << uint16_t{42};
BOOST_CHECK_GT(stream.size(), 0U);
}
BOOST_CHECK(stream.empty());
}

BOOST_AUTO_TEST_CASE(xor_file)
{
fs::path xor_path{m_args.GetDataDirBase() / "test_xor.bin"};
Expand Down
Loading