refactor: use `SpanReader` in deserialization benchmarks #35025

l0rinc commented at 7:28 PM on April 7, 2026: contributor

Problem

Block deserialization benches still read immutable fixture bytes through DataStream, which keeps around mutable stream semantics and old compaction-oriented setup that these call sites do not need anymore.

Fix

We first remove the stale Rewind() parameter and failure path, which reduces rewinding to a simple reset of the read position that clear() can reuse.

We then route fully consumed read() and ignore() paths through clear(), remove the leftover compaction references and dummy-byte workaround, and finally switch the block deserialization benchmark readers to SpanReader.

DeserializeBlockTest can then deserialize directly from the fixture bytes without an untimed setup phase, while CheckBlockTest still keeps setup only to rebuild a fresh CBlock before the timed CheckBlock() call.

Context

This follows the same direction as #34483 and is a follow-up to #34208. The modified benchmarks retain their previous timing.

Benchmarks

The affected benchmarks speeds don't seem to be affected by the changes.

<details><summary>Before & After</summary>

Before:


|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       37,591,891.96 |               26.60 |    1.0% |     11.07 | `BlockToJsonVerboseWrite`
|          155,664.09 |            6,424.09 |    0.1% |     10.99 | `BlockToJsonVerbosity1`
|       28,620,345.39 |               34.94 |    0.1% |     10.99 | `BlockToJsonVerbosity2`
|       28,637,604.74 |               34.92 |    0.1% |     11.01 | `BlockToJsonVerbosity3`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          530,167.00 |            1,886.20 |    4.7% |      0.01 | `CheckBlockTest`
|        1,439,417.00 |              694.73 |    0.7% |      0.02 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              269.95 |        3,704,375.43 |    0.4% |     11.01 | `PrevectorDeserializeNontrivial`
|               14.90 |       67,114,436.52 |    0.0% |     10.88 | `PrevectorDeserializeTrivial`

After:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       37,114,824.07 |               26.94 |    1.8% |     10.89 | `BlockToJsonVerboseWrite`
|          154,881.99 |            6,456.53 |    0.2% |     10.99 | `BlockToJsonVerbosity1`
|       28,546,697.37 |               35.03 |    0.2% |     10.98 | `BlockToJsonVerbosity2`
|       28,547,328.27 |               35.03 |    0.3% |     11.02 | `BlockToJsonVerbosity3`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          522,750.00 |            1,912.96 |    4.7% |      0.01 | `CheckBlockTest`
|        1,404,510.54 |              711.99 |    0.1% |     11.00 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              273.52 |        3,655,991.66 |    0.4% |     11.00 | `PrevectorDeserializeNontrivial`
|               14.31 |       69,863,193.52 |    1.4% |     11.03 | `PrevectorDeserializeTrivial`

</details>

DrahtBot added the label Refactoring on Apr 7, 2026

DrahtBot commented at 7:28 PM on April 7, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
#32554 (bench: replace embedded raw block with configurable block generator by l0rinc)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/bench/prevector.cpp:84 in 00067fa1f7

  80 | @@ -81,7 +81,7 @@ static void PrevectorDeserialize(benchmark::Bench& bench)
  81 |          for (auto x = 0; x < 1000; ++x) {
  82 |              s0 >> t1;
  83 |          }
  84 | -        s0.Rewind();
  85 | +        s0.rewind();

maflcko commented at 7:44 PM on April 7, 2026:

not sure about this change. This should just use a span reader than to rewind or prevent compaction with hacks

l0rinc commented at 7:51 PM on April 7, 2026:

I tried that originally (8 months ago when I opened https://github.com/l0rinc/bitcoin/pull/31), but decided against it for some reason - let me try again, thanks for the quick feedback.

l0rinc commented at 9:23 PM on April 7, 2026:

Thanks, pushed, the benchmarks indicate no significant change in performance:

After:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       37,591,891.96 |               26.60 |    1.0% |     11.07 | `BlockToJsonVerboseWrite`
|          155,664.09 |            6,424.09 |    0.1% |     10.99 | `BlockToJsonVerbosity1`
|       28,620,345.39 |               34.94 |    0.1% |     10.99 | `BlockToJsonVerbosity2`
|       28,637,604.74 |               34.92 |    0.1% |     11.01 | `BlockToJsonVerbosity3`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          530,167.00 |            1,886.20 |    4.7% |      0.01 | `CheckBlockTest`
|        1,439,417.00 |              694.73 |    0.7% |      0.02 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              269.95 |        3,704,375.43 |    0.4% |     11.01 | `PrevectorDeserializeNontrivial`
|               14.90 |       67,114,436.52 |    0.0% |     10.88 | `PrevectorDeserializeTrivial`

After:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|       37,114,824.07 |               26.94 |    1.8% |     10.89 | `BlockToJsonVerboseWrite`
|          154,881.99 |            6,456.53 |    0.2% |     10.99 | `BlockToJsonVerbosity1`
|       28,546,697.37 |               35.03 |    0.2% |     10.98 | `BlockToJsonVerbosity2`
|       28,547,328.27 |               35.03 |    0.3% |     11.02 | `BlockToJsonVerbosity3`

|            ns/block |             block/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          522,750.00 |            1,912.96 |    4.7% |      0.01 | `CheckBlockTest`
|        1,404,510.54 |              711.99 |    0.1% |     11.00 | `DeserializeBlockTest`

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|              273.52 |        3,655,991.66 |    0.4% |     11.00 | `PrevectorDeserializeNontrivial`
|               14.31 |       69,863,193.52 |    1.4% |     11.03 | `PrevectorDeserializeTrivial`

maflcko commented at 6:15 AM on April 8, 2026:

I think the pull title can be adjusted now, because prevector is not a block deser bench, same for BlockToJson anyway ...

l0rinc force-pushed on Apr 7, 2026

l0rinc renamed this:
~~refactor: use `SpanReader` in block deserialization benchmarks~~
refactor: use `SpanReader` in deserialization benchmarks
on Apr 8, 2026

in src/bench/checkblock.cpp:22 in 53d858e620 outdated

  29 |  {
  30 | -    DataStream stream;
  31 | -    bench.unit("block").epochIterations(1)
  32 | -        .setup([&] { stream = DataStream{benchmark::data::block413567}; })
  33 | -        .run([&] { CBlock block; stream >> TX_WITH_WITNESS(block); });
  34 | +    const auto block_data{benchmark::data::block413567};

sedited commented at 12:01 PM on April 20, 2026:

Do we need this separate variable as opposed to SpanReader consuming the data span directly?

l0rinc commented at 1:28 PM on April 20, 2026:

This way it's compatible with https://github.com/bitcoin/bitcoin/pull/32554/changes#diff-547fa26a77a99ccd77aca7ce1c69c0544666f788d463dc7bff664001f9ff1c88R23

sedited approved

sedited commented at 12:02 PM on April 20, 2026: contributor

ACK 53d858e6200f5617a2a5d1625cee82ff4eaf20ed

in src/streams.h:177 in 89668a4bfd outdated

 173 | @@ -174,12 +174,6 @@ class DataStream
 174 |      value_type* data()                               { return vch.data() + m_read_pos; }
 175 |      const value_type* data() const                   { return vch.data() + m_read_pos; }
 176 |  
 177 | -    inline void Compact()

maflcko commented at 6:38 AM on April 21, 2026:

nit in the second commit 89668a4bfd6e7d7a7608e8096717f64a7cbb7720: I think this is better put in the first commit. The function is already unused on current master, so this should compile unrelated to the bench changes?

In theory, there is also nothing wrong with this method. It is just that it is unused, for a long time? Not sure when, but I guess commit 607dbfdeaf7ec053d959c47c125d60c0b7e7216a?

So the commit message could say:

""" Remove the unused Compact() method, which has been unused for a long time. If there is ever need for it in the future, it can trivially be added back. """

l0rinc commented at 8:57 AM on April 21, 2026:

The function is already unused on current master, so this should compile unrelated to the bench changes?

Yes, but both were compaction leftovers, seemed simpler to group them. If I need to touch again I'll consider splitting, thanks.

l0rinc commented at 10:29 AM on April 21, 2026:

Added it to the first commit instead, updated the commit messages.

in src/bench/prevector.cpp:76 in ecaf5708ff

  73 |      for (auto x = 0; x < 900; ++x) {
  74 | -        s0 << t0;
  75 | +        data << t0;
  76 |      }
  77 |      t0.resize(100);
  78 |      for (auto x = 0; x < 101; ++x) {

maflcko commented at 6:40 AM on April 21, 2026:

nit in ecaf5708ff4a566682f9f44d0ffaf952e4956012: In theory this can now also remove the compaction workaround, because it is no longer used and may be confusing.

l0rinc commented at 9:01 AM on April 21, 2026:

I'm not sure I realized that the reason we weren't consuming everything (1001 created vs 1000 consumed) was a compaction leftover, thanks for the hint. @sedited are you okay with reacking if I take this?

sedited commented at 9:32 AM on April 21, 2026:

Sure

l0rinc commented at 10:29 AM on April 21, 2026:

Thanks, pushed

maflcko commented at 6:48 AM on April 21, 2026: member

review ACK 53d858e6200f5617a2a5d1625cee82ff4eaf20ed 🔎

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 53d858e6200f5617a2a5d1625cee82ff4eaf20ed 🔎
2knPvwEgRwTAdJS4dZxQjdcYKGOOB5ZHbkpzuHo/EcuD1us0dE2XAw9BXw4am81C9bwfnBVZfBJcsJxwR4cfDA==

</details>

refactor: use `DataStream::clear` in `::read` and `::ignore`

When `DataStream` is fully consumed, both `read()` and `ignore()` reset it to an empty state by clearing the backing buffer and resetting the read position.

Call `clear()` in both places instead of open-coding the same state transition.
This keeps the behavior unchanged while documenting the fully-consumed reset in one place.

Remove the unused `Compact()` method as well - it has been unused for a long time and can be added back if it is ever needed.

61d678a6e3

refactor: use `SpanReader` in `TestBlockAndIndex`

`TestBlockAndIndex` still deserialized its fixed block fixture through `DataStream` and appended a dummy byte to avoid compaction after full consumption.

Use `SpanReader` for that fixture instead.
This removes the leftover dummy-byte workaround and reads the immutable fixture through a read-only view.

b8eb6c2081

refactor: use `SpanReader` in `PrevectorDeserialize`

`PrevectorDeserialize` only needs a reusable read-only view over fixed serialized bytes.
Keeping a mutable `DataStream` around just to call `Rewind()` is unnecessary.

Rebuild a fresh `SpanReader` for each benchmark run and remove `DataStream::Rewind()`, whose remaining use was this benchmark-only reset path.
The benchmark can now serialize exactly the 1000 entries it deserializes, so drop the stale extra element that used to avoid full consumption.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

2529f25555

refactor: replace `DataStream` with `SpanReader` in block deserialization tests

These benchmark inputs are immutable fixture bytes, so `DataStream` adds an unnecessary owned buffer and the setup needed to recreate or preserve its state.

Use `SpanReader` for block deserialization in `checkblock` instead.
This keeps `DeserializeBlockTest` focused on deserialization work, while `CheckBlockTest` still uses untimed setup only to rebuild a fresh uncached `CBlock` for the timed `CheckBlock()` call.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

13c8df4d5a

l0rinc force-pushed on Apr 21, 2026

maflcko commented at 11:06 AM on April 21, 2026: member

review ACK 13c8df4d5a9eeca18a77644fada1816c0ec10308 🐠

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 13c8df4d5a9eeca18a77644fada1816c0ec10308 🐠
thCKRpcr/7KG4hm5y6dZxuUGkaTQHGqxpuN8cSWIMW3XNjqtb2X49im0n3jEEPr2dIKEWowUUtOiEw5bG31TCA==

</details>

DrahtBot requested review from sedited on Apr 21, 2026

sedited approved

sedited commented at 11:37 AM on April 21, 2026: contributor

Re-ACK 13c8df4d5a9eeca18a77644fada1816c0ec10308

sedited commented at 11:52 AM on April 21, 2026: contributor

Looks like this is about to run into #34731 , so will merge this now, since that is unrelated.

sedited merged this on Apr 21, 2026

sedited closed this on Apr 21, 2026

l0rinc deleted the branch on Apr 21, 2026