p2p: cache compact block message to use for low bandwidth CMPCT_BLOCK #26755

pull andrewtoth wants to merge 2 commits into bitcoin:master from andrewtoth:cache-cmpctblk-msg changing 2 files +55 −10

andrewtoth commented at 8:29 PM on December 26, 2022: contributor

Follow-up from #23880 (comment).

This stores the lazy serialized compact block message future for the last seen block, rather than a shared pointer to the compact block itself. Then we don't have to reserialize the message for any incoming CMPCTBLOCK messages from low-bandwidth peers.

It was mentioned there was a speed savings of 25% using this method for sending to high-bandwidth peers #23880 (comment). I've included benchmarks which seems to be an even higher speedup.

| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 38,462.08 | 25,999.63 | 0.3% | 150,324.88 | 84,927.50 | 1.770 | 34,126.12 | 0.1% | 0.01 | SerializeNetMsg

| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark |--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:---------- | 616.47 | 1,622,135.41 | 0.9% | 796.14 | 1,359.27 | 0.586 | 177.03 | 0.0% | 0.01 | CacheSerializeNetMsg
DrahtBot commented at 8:29 PM on December 26, 2022: 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. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #26326 (net: don't lock cs_main while reading blocks in net processing by andrewtoth)
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.
DrahtBot added the label P2P on Dec 26, 2022
andrewtoth force-pushed on Dec 26, 2022
p2p: cache compact block message to use for low bandwidth CMPCT_BLOCK 61556f0f5e
bench: serialize CMPCTBLOCK and cache serialize 6947ca3f96
andrewtoth force-pushed on Dec 28, 2022
in src/bench/checkblock.cpp:88 in 6947ca3f96
```
  83 | +            return msgMaker.Make(NetMsgType::CMPCTBLOCK, *pcmpctblock);
  84 | +        }
  85 | +    )};
  86 | +
  87 | +    bench.run([&] {
  88 | +        CSerializedNetMsg msg{lazy_ser.get().Copy()};
```
maflcko commented at 10:52 AM on December 28, 2022:

Not sure how useful this is. Won't this massively elevate the duration of the first call compared to all following ones?

The speed-up I reported in #23880 (comment) is obviously dependent on the number of peers you send the message to. If there is only one peer, it will be slower, because the duration is Ser+Copy*1, which is more work than just Ser*1. However with sufficient peers N, there is a speed-up because Ser+Copy*N is faster than Ser*N.

andrewtoth commented at 4:58 PM on December 28, 2022:

Hmm I suppose a better benchmark would be time it takes to copy to see what N needs to be for this optimization to make sense. I was making the assumption here that Copy is always faster than Ser, so if there's at least one high bandwidth peer we relayed too already then it makes sense to just Copy for any low bandwidth requests that come in rather than Ser.

andrewtoth commented at 9:21 PM on December 28, 2022:

I think the confusion is that this PR was changing the first low bandwidth response to use a lazy serializer and copy. This is actually using a copy of the same lazy future that is used for the high bandwidth peers. So as long as there's at least one high bandwidth peer then this optimization makes sense because the serialization will already have been done.
andrewtoth commented at 6:19 PM on December 28, 2022: contributor

Benefit doesn't seem to be worth the change here.
andrewtoth closed this on Dec 28, 2022
andrewtoth deleted the branch on Dec 28, 2022
bitcoin locked this on Dec 28, 2023

Contributors

Labels

Linked (view graph)

#23880 p2p: Serialize cmpctblock at most once in NewPoWValidBlock

p2p: cache compact block message to use for low bandwidth CMPCT_BLOCK #26755

Reviews

Conflicts