ConnectBlock: don't serialize block hash twice #23819

jb55 · 2021-12-19T21:06:34Z

In the validation:block_connected tracepoint, we call block->GetHash(), which
ends up calling CBlockHeader::GetHash(), executing around 8000 serialization
instructions. We don't need to do this extra work, because block->GetHash() is
already called further up in the function. Let's save that value as a local
variable and re-use it in our tracepoint so there is no unnecessary tracepoint
overhead.

Shave off an extra 100 or so instructions from the validation:block_connected
tracepoint by reusing a nearby GetTimeMicros(). This brings the tracepoint down
to 54 instructions. Still high, but much better than the previous ~154 and
8000 instructions which it was originally.

Signed-off-by: William Casarin jb55@jb55.com

jb55 · 2021-12-19T21:09:43Z

cc @laanwj @0xB10C

0xB10C · 2021-12-20T11:48:14Z

Concept ACK, will review in the next days.

ref: #23724

theStack

Code-review ACK b60c8fd

Not related to the tracepoint overhead, but maybe still worth mentioning: note that in the PR branch, block.GetHash() is still called more than once in CChainState::ConnectBlock, as there is another invocation at the very start:

bitcoin/src/validation.cpp

Line 1869 in a1e04e1

assert(*pindex->phashBlock == block.GetHash());

(As far as I know, we enforce compiling with assertions on, i.e. NDEBUG never being set)

jb55 · 2022-01-04T17:04:29Z

On Sat, Jan 01, 2022 at 01:55:29PM -0800, Sebastian Falbesoner wrote: Not related to the tracepoint overhead, but maybe still worth mentioning: note that in the PR branch, `block.GetHash()` is still called more than once in `CChainState::ConnectBlock`, as there is another invocation at the very start: https://github.com/bitcoin/bitcoin/blob/a1e04e10799791a728cce4430808517598c8742c/src/validation.cpp#L1869 (As far as I know, we enforce compiling with assertions on, i.e. NDEBUG never being set)

great catch. I've moved the blockHash variable to above the assert, so we shave off ~16,000 instructions now.

theStack

Code review re-ACK 8f8b196

Slightly off-topic, but could you elaborate how exactly you measure the number of saved instructions (and on which architecture)? Do you statically analyse the compiled binary or do it dynamically via perf or alike?

jb55 · 2022-01-04T19:05:32Z

On Tue, Jan 04, 2022 at 10:57:28AM -0800, Sebastian Falbesoner wrote: Slightly off-topic, but could you elaborate how exactly you measure the number of saved instructions (and on which architecture)? Do you statically analyse the compiled binary or do it dynamically via `perf` or alike?

See #23724 (comment) for the method and #23724 (comment) for where I determined that GetHash hits 8000 instructions.

…ilds with USDT tracepoints 6200fbf build: rename --enable-ebpf to --enable-usdt (0xb10c) e158a2a build: add systemtap's sys/sdt.h as depends (0xb10c) Pull request description: There has been light conceptual agreement on including the Userspace, Statically Defined Tracing tracepoints in Bitcoin Core release builds. This, for example, enables user to hook into production deployments, if they need to. Binaries don't have to be switched out. This is possible because we don't do [expensive computations](https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#no-expensive-computations-for-tracepoints) only needed for the tracepoints. The tracepoints are NOPs when not used. Systemtap's `sys/sdt.h` header is required to build Bitcoin Core with USDT support. The header file defines the `DTRACE_PROBE` macros used in [`src/util/trace.h`](https://github.com/bitcoin/bitcoin/blob/master/src/util/trace.h). This PR adds Systemtap 4.5 (May 2021) as dependency. GUIX builds for Linux hosts now include the tracepoints. Closes #23297. ACKs for top commit: fanquake: ACK 6200fbf - tested enabling / disabling and with/without SDT from depends. We can follow up with #23819, #23907 and #23296, and if any serious issues arise before feature freeze, it is easy for us to flip depends such that USDT becomes opt-in, rather than opt-out, and thus, releases would be tracepoint free. Tree-SHA512: 0263f44892bf8450e8a593e4de7a498243687f8d81269e1c3283fa8354922c7cf93fddef4b92cf5192d33798424aa5812e03e68ef8de31af078a32dd34021382

…GUIX builds with USDT tracepoints 6200fbf build: rename --enable-ebpf to --enable-usdt (0xb10c) e158a2a build: add systemtap's sys/sdt.h as depends (0xb10c) Pull request description: There has been light conceptual agreement on including the Userspace, Statically Defined Tracing tracepoints in Bitcoin Core release builds. This, for example, enables user to hook into production deployments, if they need to. Binaries don't have to be switched out. This is possible because we don't do [expensive computations](https://github.com/bitcoin/bitcoin/blob/master/doc/tracing.md#no-expensive-computations-for-tracepoints) only needed for the tracepoints. The tracepoints are NOPs when not used. Systemtap's `sys/sdt.h` header is required to build Bitcoin Core with USDT support. The header file defines the `DTRACE_PROBE` macros used in [`src/util/trace.h`](https://github.com/bitcoin/bitcoin/blob/master/src/util/trace.h). This PR adds Systemtap 4.5 (May 2021) as dependency. GUIX builds for Linux hosts now include the tracepoints. Closes bitcoin#23297. ACKs for top commit: fanquake: ACK 6200fbf - tested enabling / disabling and with/without SDT from depends. We can follow up with bitcoin#23819, bitcoin#23907 and bitcoin#23296, and if any serious issues arise before feature freeze, it is easy for us to flip depends such that USDT becomes opt-in, rather than opt-out, and thus, releases would be tracepoint free. Tree-SHA512: 0263f44892bf8450e8a593e4de7a498243687f8d81269e1c3283fa8354922c7cf93fddef4b92cf5192d33798424aa5812e03e68ef8de31af078a32dd34021382

luke-jr

utACK

I'm surprised we're not caching block hashes like we do for transactions, though?

jb55 · 2022-01-12T00:55:37Z

On Tue, Jan 11, 2022 at 03:11:55PM -0800, Luke Dashjr wrote: I'm surprised we're not caching block hashes like we do for transactions, though?

Where is it cached for transactions? I see `CMutableTransaction::GetHash` but it looks roughly the same as `CBlockHeader::GetHash`.

luke-jr · 2022-01-12T01:02:54Z

Check CTransaction

jb55 · 2022-01-12T02:39:34Z

On Tue, Jan 11, 2022 at 05:03:06PM -0800, Luke Dashjr wrote: Check `CTransaction`

gotcha, I see ComputeHash in the constructor. Yes it looks like CBlockHeader doesn't have caching, I'm guessing because it's a mutable thing?

maflcko · 2022-01-12T07:48:14Z

Looks like this is also refactoring ConnectBlock, which is not tracing related? -> Renamed pull title

maflcko

lgtm. Left a style nit

src/validation.cpp

In the validation:block_connected tracepoint, we call block->GetHash(), which ends up calling CBlockHeader::GetHash(), executing around 8000 serialization instructions. We don't need to do this extra work, because block->GetHash() is already called further up in the function. Let's save that value as a local variable and re-use it in our tracepoint so there is no unnecessary tracepoint overhead. Signed-off-by: William Casarin <jb55@jb55.com>

Shave off an extra 100 or so instructions from the validation:block_connected tracepoint by reusing a nearby GetTimeMicros(). This brings the tracepoint down to 54 instructions. Still high, but much better than the previous ~154 and 8000 instructions which it was originally. Signed-off-by: William Casarin <jb55@jb55.com>

theStack

re-ACK eb8b22d

0xB10C · 2022-01-13T11:15:52Z

ACK eb8b22d

I'm measuring 7 instructions. Code changes look good to me. Still needs refactoring in the PR title :)

(gdb) b src/validation:2161
Breakpoint 1 at 0x2c57b0: file src/validation.cpp, line 2161.
(gdb) b src/validation:
Breakpoint 2 at 0x2c57de: file src/validation.cpp, line 2172.
(gdb) run
[..]

Thread 22 "b-msghand" hit Breakpoint 1, CChainState::ConnectBlock (this=0x555555d19980, block=..., state=...,
    pindex=<optimized out>, view=..., fJustCheck=false) at validation.cpp:2161
2161	    LogPrint(BCLog::BENCH, "    - Index writing: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5 - nTime4), nTimeIndex * MICRO, nTimeIndex * MILLI / nBlocksTotal);
(gdb) n
2163	    TRACE6(validation, block_connected,
(gdb) record btrace
(gdb) c
Continuing.

Thread 22 "b-msghand" hit Breakpoint 2, CCheckQueueControl<CScriptCheck>::~CCheckQueueControl (
    this=<optimized out>, __in_chrg=<optimized out>) at validation.cpp:2172
2172	    return true;
(gdb) record info
Undefined record command: "info".  Try "help record".
(gdb) info record
[..]
Recorded 7 instructions in 1 functions (0 gaps) for thread 22 (Thread 0x7fff2e7fc640 (LWP 508792)).

@jb55 you mention 54 instructions in eb8b22d. I think your measurements include the instructions from line 2161. I'm starting the measurement in line 2163.

bitcoin/src/validation.cpp

Lines 2161 to 2172 in eb8b22d

    
           LogPrint(BCLog::BENCH, "    - Index writing: %.2fms [%.2fs (%.2fms/blk)]\n", MILLI * (nTime5 - nTime4), nTimeIndex * MICRO, nTimeIndex * MILLI / nBlocksTotal); 
        
           TRACE6(validation, block_connected, 
        
               block_hash.data(), 
        
               pindex->nHeight, 
        
               block.vtx.size(), 
        
               nInputs, 
        
               nSigOpsCost, 
        
               nTime5 - nTimeStart // in microseconds (µs) 
        
           ); 
        
           return true;

laanwj · 2022-02-17T13:09:11Z

Code review ACK eb8b22d

…ash twice (#5042) * block_connected: don't serialize block hash twice In the validation:block_connected tracepoint, we call block->GetHash(), which ends up calling CBlockHeader::GetHash(), executing around 8000 serialization instructions. We don't need to do this extra work, because block->GetHash() is already called further up in the function. Let's save that value as a local variable and re-use it in our tracepoint so there is no unnecessary tracepoint overhead. Signed-off-by: William Casarin <jb55@jb55.com> * ConnectBlock: re-use hash on budget start Signed-off-by: William Casarin <jb55@jb55.com> Co-authored-by: William Casarin <jb55@jb55.com>

Summary: ``` In the validation:block_connected tracepoint, we call block->GetHash(), which ends up calling CBlockHeader::GetHash(), executing around 8000 serialization instructions. We don't need to do this extra work, because block->GetHash() is already called further up in the function. Let's save that value as a local variable and re-use it in our tracepoint so there is no unnecessary tracepoint overhead. Shave off an extra 100 or so instructions from the validation:block_connected tracepoint by reusing a nearby GetTimeMicros(). This brings the tracepoint down to 54 instructions. Still high, but much better than the previous ~154 and 8000 instructions which it was originally. ``` Backport of [[bitcoin/bitcoin#23819 | core#23819]]. Depends on D12674. Test Plan: sudo bpftrace ../contrib/tracing/connectblock_benchmark.bt 0 0 25 Reviewers: #bitcoin_abc, PiRK, sdulfari Reviewed By: #bitcoin_abc, PiRK, sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12705

DrahtBot added the Validation label Dec 19, 2021

fanquake mentioned this pull request Dec 30, 2021

build: add systemtap's sys/sdt.h as depends for GUIX builds with USDT tracepoints #23724

Merged

theStack approved these changes Jan 1, 2022

View reviewed changes

jb55 force-pushed the faster_block_connected_usdt branch from b60c8fd to 8f8b196 Compare January 4, 2022 16:59

theStack approved these changes Jan 4, 2022

View reviewed changes

fanquake requested a review from laanwj January 5, 2022 05:50

luke-jr approved these changes Jan 11, 2022

View reviewed changes

maflcko changed the title ~~tracing/block_connected: don't serialize block hash twice~~ ConnectBlock: don't serialize block hash twice Jan 12, 2022

maflcko added the Refactoring label Jan 12, 2022

maflcko approved these changes Jan 12, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

jb55 added 2 commits January 12, 2022 09:27

jb55 force-pushed the faster_block_connected_usdt branch from 8f8b196 to eb8b22d Compare January 12, 2022 17:28

theStack approved these changes Jan 12, 2022

View reviewed changes

fanquake added this to the 23.0 milestone Jan 18, 2022

maflcko mentioned this pull request Feb 8, 2022

Enforce Taproot script flags whenever WITNESS is set #23536

Merged

laanwj merged commit 922c49a into bitcoin:master Feb 17, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 18, 2022

Merge bitcoin#23819: ConnectBlock: don't serialize block hash twice

504a590

JSKitty mentioned this pull request Oct 15, 2022

backport: partial bitcoin#23819 ConnectBlock: don't serialize block hash twice dashpay/dash#5042

Merged

andrewtoth mentioned this pull request Dec 31, 2022

p2p: cache block hash in ProcessGetBlockData and CMPCTBLOCK processing #26782

Closed

bitcoin locked and limited conversation to collaborators Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ConnectBlock: don't serialize block hash twice #23819

ConnectBlock: don't serialize block hash twice #23819

jb55 commented Dec 19, 2021 •

edited

jb55 commented Dec 19, 2021

0xB10C commented Dec 20, 2021

theStack left a comment

jb55 commented Jan 4, 2022 via email

theStack left a comment

jb55 commented Jan 4, 2022 via email

luke-jr left a comment

jb55 commented Jan 12, 2022 via email

luke-jr commented Jan 12, 2022

jb55 commented Jan 12, 2022 via email

maflcko commented Jan 12, 2022

maflcko left a comment

theStack left a comment

0xB10C commented Jan 13, 2022

laanwj commented Feb 17, 2022

ConnectBlock: don't serialize block hash twice #23819

ConnectBlock: don't serialize block hash twice #23819

Conversation

jb55 commented Dec 19, 2021 • edited

jb55 commented Dec 19, 2021

0xB10C commented Dec 20, 2021

theStack left a comment

Choose a reason for hiding this comment

jb55 commented Jan 4, 2022 via email

theStack left a comment

Choose a reason for hiding this comment

jb55 commented Jan 4, 2022 via email

luke-jr left a comment

Choose a reason for hiding this comment

jb55 commented Jan 12, 2022 via email

luke-jr commented Jan 12, 2022

jb55 commented Jan 12, 2022 via email

maflcko commented Jan 12, 2022

maflcko left a comment

Choose a reason for hiding this comment

theStack left a comment

Choose a reason for hiding this comment

0xB10C commented Jan 13, 2022

laanwj commented Feb 17, 2022

jb55 commented Dec 19, 2021 •

edited