Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework broadcast logic #2741

Merged
merged 3 commits into from
Oct 12, 2022
Merged

Conversation

roman-khimov
Copy link
Member

We have a number of queues for different purposes:
 * regular broadcast queue
 * direct p2p queue
 * high-priority queue

And two basic egress scenarios:
 * direct p2p messages (replies to requests in Server's handle* methods)
 * broadcasted messages

Low priority broadcasted messages:
 * transaction inventories
 * block inventories
 * notary inventories
 * non-consensus extensibles

High-priority broadcasted messages:
 * consensus extensibles
 * getdata transaction requests from consensus process
 * getaddr requests

P2P messages are a bit more complicated, most of the time they use p2p queue,
but extensible message requests/replies use HP queue.

Server's handle* code is run from Peer's handleIncoming, every peer has this
thread that handles incoming messages. When working with the peer it's
important to reply to requests and blocking this thread until we send (queue)
a reply is fine, if the peer is slow we just won't get anything new from
it. The queue used is irrelevant wrt this issue.

Broadcasted messages are radically different, we want them to be delivered to
many peers, but we don't care about specific ones. If it's delivered to 2/3 of
the peers we're fine, if it's delivered to more of them --- it's not an
issue. But doing this fairly is not an easy thing, current code tries performing
unblocked sends and if this doesn't yield enough results it then blocks (but
has a timeout, we can't wait indefinitely). But it does so in sequential
manner, once the peer is chosen the code will wait for it (and only it) until
timeout happens.

What can be done instead is an attempt to push the message to all of the peers
simultaneously (or close to that). If they all deliver --- OK, if some block
and wait then we can wait until _any_ of them pushes the message through (or
global timeout happens, we still can't wait forever). If we have enough
deliveries then we can cancel pending ones and it's again not an error if
these canceled threads still do their job.

This makes the system more dynamic and adds some substantial processing
overhead, but it's a networking code, any of this overhead is much lower than
the actual packet delivery time. It also allows to spread the load more
fairly, if there is any spare queue it'll get the packet and release the
broadcaster. On the next broadcast iteration another peer is more likely to be
chosen just because it didn't get a message previously (and had some time to
deliver already queued messages).

It works perfectly in tests, with optimal networking conditions we have much
better block times and TPS increases by 5-25%% depending on the scenario.

I'd go as far as to say that it fixes the original problem of #2678, because
in this particular scenario we have empty queues in ~100% of the cases and
this new logic will likely lead to 100% fan out in this case (cancelation just
won't happen fast enough). But when the load grows and there is some waiting
in the queue it will optimize out the slowest links.

cpu_four_(30_wrk,_50K_pool,_200ms)
cpu_four_(30_wrk,_50K_pool)
cpu_seven_(30_wrk,_50K_pool,_200ms)
cpu_seven_(30_wrk,_50K_pool)
mem_four_(30_wrk,_50K_pool,_200ms)
mem_four_(30_wrk,_50K_pool)
mem_seven_(30_wrk,_50K_pool,_200ms)
mem_seven_(30_wrk,_50K_pool)
ms_per_block_four_(30_wrk,_50K_pool,_200ms)
ms_per_block_four_(30_wrk,_50K_pool)
ms_per_block_seven_(30_wrk,_50K_pool,_200ms)
ms_per_block_seven_(30_wrk,_50K_pool)
tpb_four_(30_wrk,_50K_pool,_200ms)
tpb_four_(30_wrk,_50K_pool)
tpb_seven_(30_wrk,_50K_pool,_200ms)
tpb_seven_(30_wrk,_50K_pool)
tps_four_(30_wrk,_50K_pool,_200ms)
tps_four_(30_wrk,_50K_pool)
tps_seven_(30_wrk,_50K_pool,_200ms)
tps_seven_(30_wrk,_50K_pool)

@roman-khimov roman-khimov added the network P2P layer label Oct 11, 2022
@roman-khimov roman-khimov added this to the v0.99.5 milestone Oct 11, 2022
We have a number of queues for different purposes:
 * regular broadcast queue
 * direct p2p queue
 * high-priority queue

And two basic egress scenarios:
 * direct p2p messages (replies to requests in Server's handle* methods)
 * broadcasted messages

Low priority broadcasted messages:
 * transaction inventories
 * block inventories
 * notary inventories
 * non-consensus extensibles

High-priority broadcasted messages:
 * consensus extensibles
 * getdata transaction requests from consensus process
 * getaddr requests

P2P messages are a bit more complicated, most of the time they use p2p queue,
but extensible message requests/replies use HP queue.

Server's handle* code is run from Peer's handleIncoming, every peer has this
thread that handles incoming messages. When working with the peer it's
important to reply to requests and blocking this thread until we send (queue)
a reply is fine, if the peer is slow we just won't get anything new from
it. The queue used is irrelevant wrt this issue.

Broadcasted messages are radically different, we want them to be delivered to
many peers, but we don't care about specific ones. If it's delivered to 2/3 of
the peers we're fine, if it's delivered to more of them --- it's not an
issue. But doing this fairly is not an easy thing, current code tries performing
unblocked sends and if this doesn't yield enough results it then blocks (but
has a timeout, we can't wait indefinitely). But it does so in sequential
manner, once the peer is chosen the code will wait for it (and only it) until
timeout happens.

What can be done instead is an attempt to push the message to all of the peers
simultaneously (or close to that). If they all deliver --- OK, if some block
and wait then we can wait until _any_ of them pushes the message through (or
global timeout happens, we still can't wait forever). If we have enough
deliveries then we can cancel pending ones and it's again not an error if
these canceled threads still do their job.

This makes the system more dynamic and adds some substantial processing
overhead, but it's a networking code, any of this overhead is much lower than
the actual packet delivery time. It also allows to spread the load more
fairly, if there is any spare queue it'll get the packet and release the
broadcaster. On the next broadcast iteration another peer is more likely to be
chosen just because it didn't get a message previously (and had some time to
deliver already queued messages).

It works perfectly in tests, with optimal networking conditions we have much
better block times and TPS increases by 5-25%% depending on the scenario.

I'd go as far as to say that it fixes the original problem of #2678, because
in this particular scenario we have empty queues in ~100% of the cases and
this new logic will likely lead to 100% fan out in this case (cancelation just
won't happen fast enough). But when the load grows and there is some waiting
in the queue it will optimize out the slowest links.
Otherwise we routinely get "unexpected addr received" error.
@roman-khimov roman-khimov force-pushed the separate-broadcast-queue-handling branch from 03700f6 to 8b26d94 Compare October 11, 2022 15:42
@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #2741 (8b26d94) into master (0294e2e) will decrease coverage by 0.07%.
The diff coverage is 46.15%.

❗ Current head 8b26d94 differs from pull request most recent head e1d5f18. Consider uploading reports for the commit e1d5f18 to get more accurate results

@@            Coverage Diff             @@
##           master    #2741      +/-   ##
==========================================
- Coverage   85.41%   85.33%   -0.08%     
==========================================
  Files         324      324              
  Lines       40071    40064       -7     
==========================================
- Hits        34226    34188      -38     
- Misses       4494     4525      +31     
  Partials     1351     1351              
Impacted Files Coverage Δ
pkg/network/tcp_peer.go 27.96% <20.00%> (-0.80%) ⬇️
pkg/network/server.go 73.05% <70.37%> (+0.06%) ⬆️
pkg/network/payload/mptdata.go 81.25% <0.00%> (-18.75%) ⬇️
pkg/services/oracle/oracle.go 72.99% <0.00%> (-14.60%) ⬇️
pkg/services/oracle/request.go 58.18% <0.00%> (-5.00%) ⬇️
pkg/core/transaction/transaction.go 85.18% <0.00%> (-1.49%) ⬇️
pkg/consensus/consensus.go 73.58% <0.00%> (+0.43%) ⬆️
pkg/network/message_string.go 83.87% <0.00%> (+12.90%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Nice optimisation!

@roman-khimov roman-khimov merged commit ec4983e into master Oct 12, 2022
@roman-khimov roman-khimov deleted the separate-broadcast-queue-handling branch October 12, 2022 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
network P2P layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants