Skip to content

p2p: Serialize cmpctblock at most once in NewPoWValidBlock#23880

Merged
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:2112-p2pAsync
Mar 21, 2022
Merged

p2p: Serialize cmpctblock at most once in NewPoWValidBlock#23880
maflcko merged 1 commit intobitcoin:masterfrom
maflcko:2112-p2pAsync

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Dec 27, 2021

Instead of serializing for each peer, serialize at most once and copy the raw data for each peer.

@theStack
Copy link
Contributor

theStack commented Dec 27, 2021

Concept ACK

IIRC I also tried to tackle this TODO some time ago but failed as move-semantics prevented to store the serialized message. This PR is now a good opportunity to learn about futures and std::async.

@maflcko
Copy link
Member Author

maflcko commented Dec 27, 2021

Yeah, it is not possible to unconditionally std::move the result of a std::shared_future here, since it is unknown if there is another call to the getter later on. (In theory it might be possible to avoid the last copy and do a move instead, but I am not sure if this is worth it, let alone easily possible).

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 28, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #20799 (net processing: Only support version 2 compact blocks 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.

shaavan
shaavan approved these changes Dec 28, 2021
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

ACK fabe7843a767505abdc580581e330fb4f963c788

I was researching about this PR. I found this useful article. It talks about how using std::deferred allows lazy calculations. If the value has been already calculated, it will not be recalculated, potentially decreasing the number of times serialization is done to one.

I have reviewed the code changes in this PR, and I found them agreeable.

@DrahtBot DrahtBot mentioned this pull request Dec 28, 2021
Merged
theStack
theStack reviewed Jan 24, 2022
View reviewed changes
shaavan
shaavan approved these changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

reACK fa61dd4

Changes since my last review:

  • Fixed a typo. ser_cpmptblock -> ser_cmpctblock.
  • Rebased over current master.

theStack
theStack approved these changes Jan 30, 2022
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

After doing some more research about the concepts of futures and lazy evaluation in C++, I'm convinced now that this change is correct. This seems to be the first time ever that std::async and std::shared_future are used in the Bitcoin Core codebase.

Code-review ACK fa61dd4

@maflcko maflcko merged commit 6c72f31 into bitcoin:master Mar 21, 2022
@maflcko maflcko deleted the 2112-p2pAsync branch March 21, 2022 09:02
jonatack
jonatack reviewed Mar 21, 2022
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Nice. Posthumous ACK.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 22, 2022
Merge bitcoin#23880: p2p: Serialize cmpctblock at most once in NewPoW…
1afced4 
…ValidBlock

fa61dd4 p2p: Serialize cmpctblock at most once in NewPoWValidBlock (MarcoFalke)

Pull request description:

  Instead of serializing for each peer, serialize at most once and copy the raw data for each peer.

ACKs for top commit:
  shaavan:
    reACK fa61dd4
  theStack:
    Code-review ACK fa61dd4

Tree-SHA512: ed029aeaea67fdac8ddb865069f8166bc0dd8480418c405628e3e1a43b61161584a09a1814668bcd220602e8732e188be2bfed9242aa81bdbd92c64c702ed138
@jnewbery
Copy link
Contributor

jnewbery commented Mar 25, 2022

Concept ACK.

Is there a reason you need a std::shared_future rather than a std::future? It never gets copied for usage by multiple threads, which I believe is the usage for a shared_future. Would this be equivalent to what you've used here:

-    const std::shared_future<CSerializedNetMsg> lazy_ser{
+    std::future<CSerializedNetMsg> lazy_ser{

Could the same future be used to replace the most_recent_compact_block global, so that we don't have to reserialize the compact block for peers that request the compact block in low bandwidth mode?

@maflcko
Copy link
Member Author

maflcko commented Mar 25, 2022

No, that'd be UB, see https://en.cppreference.com/w/cpp/thread/future/get

valid() is false after a call to std::future<T>::get()
The behavior is undefined if valid() is false before the call to get()

@maflcko
Copy link
Member Author

maflcko commented Mar 25, 2022

Could the same future be used to replace the most_recent_compact_block global, so that we don't have to reserialize the compact block for peers that request the compact block in low bandwidth mode?

Likely, but I haven't checked.

@jnewbery
Copy link
Contributor

jnewbery commented Mar 25, 2022

No, that'd be UB, see en.cppreference.com/w/cpp/thread/future/get

Ah! Thanks.

@maflcko
Copy link
Member Author

maflcko commented Apr 1, 2022

For reference, for anyone wondering: A copy from the lazy ser is 25% faster on my machine than serialize.

@andrewtoth andrewtoth mentioned this pull request Dec 26, 2022
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@jonatack jonatack jonatack left review comments

@theStack theStack theStack approved these changes

@shaavan shaavan shaavan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @theStack @DrahtBot @jnewbery @jonatack @shaavan