refactor: Reuse block hash in AddToBlockIndex #16065

promag commented at 12:30 PM on May 21, 2019: member

Removes block hash computation from AddToBlockIndex. Apart from LoadGenesisBlock, the hash is already computed in AcceptBlockHeader.

fanquake added the label Refactoring on May 21, 2019

fanquake added the label Validation on May 21, 2019

MarcoFalke commented at 12:50 PM on May 21, 2019: member

Tend to NACK. This makes the code harder to read

refactor: Reuse block hash in AddToBlockIndex b7a3beccd0

promag force-pushed on May 21, 2019

promag commented at 2:00 PM on May 21, 2019: member

I don't think it makes the code harder to read honestly. This saves a bit of CPU for each new CBlockIndex since CBlockHeader::GetHash() is not cached.

However feel free to close, it's not my intention to waste review effort over saving some seconds in IBD.

jb55 commented at 3:06 PM on May 21, 2019: member

Concept ACK

MarcoFalke commented at 3:58 PM on May 21, 2019: member

Instead of passing the block to AddToBlockIndex, you'd have to pass the hash and the block. Unintuitive and easy to accidentally pass the wrong hash. If this is a performance optimization, maybe the hash should be cached? And it needs a benchmark either way.

DrahtBot commented at 5:01 PM on May 21, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15976 (refactor: move methods under CChainState (pt. 1) by jamesob)
#15921 (Tidy up ValidationState interface by jnewbery)

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.

promag commented at 5:23 PM on May 21, 2019: member

@MarcoFalke fair enough, I'll see if such change is worth it.

fixup: Add BlockHeaderHashed 9f834d78d8

promag commented at 9:48 PM on May 21, 2019: member

@MarcoFalke let me know what you think.

jonasschnelli commented at 10:03 AM on May 22, 2019: contributor

We are talking only about the block header hash? @promag: can you "quantify" the speedup?

in src/validation.cpp:67 in 9f834d78d8

  59 | @@ -60,6 +60,17 @@
  60 |  #define MICRO 0.000001
  61 |  #define MILLI 0.001
  62 |  
  63 | +struct BlockHeaderHashed
  64 | +{
  65 | +    const CBlockHeader& header;
  66 | +    const uint256 hash;
  67 | +    BlockHeaderHashed(const CBlockHeader& header)

practicalswift commented at 2:56 PM on May 22, 2019:

explicit? :-)

promag commented at 3:03 PM on May 22, 2019: member

Profiling was done in debug mode, going to close and reopen if relevant in release mode.

promag closed this on May 22, 2019

jamesob commented at 3:08 PM on May 22, 2019: member

Also tend to NACK unless we can show significant performance impact. Introducing an additional identity (BlockHeaderHashed) makes the code slightly more complicated, especially when introduced without any documentation (when should this be used and why?).

promag commented at 3:30 PM on May 22, 2019: member

Before reopening I'd like to share some profile results. The following is the result of running in release mode, with an empty -datadir before and after. It shows the total time spent on ProcessNewBlockHeaders (currently 574000 headers). As you can see reusing the hash reduces the total time from 2.44s to 1.67s.

Before: Screenshot 2019-05-22 at 16 26 12 Screenshot 2019-05-22 at 16 20 11

After: Screenshot 2019-05-22 at 16 25 57

Screenshot 2019-05-22 at 16 20 29

Results on a 2.9 GHz Intel Core i9.

MarcoFalke locked this on Dec 16, 2021