Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

approval-distribution: process messages while waiting after approval-voting #7393

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented Jun 19, 2023

In, the current implementation every time we process an assignment or an approval that needs checking in the approval voting, we will wait till approval-voting answers the message.

Given that approval-voting will execute some signatures checks that take significant time(between 400us and 1 millis) per message, that's where most of the time in the approval-distribution, see
paritytech/polkadot-sdk#732 for the numbers.

So, modify approval-distribution, so that it picks another message from the queue while the approval-voting is busy doing it's work.

This will have a few benefits:

  1. Better pipelinening of the messages, approval-voting will always have work to do and it won't have to wait for the approval-distribution to send it a message. Additionally, some of the works of the approval-distribution will be executed in parallel with work in approval-voting instead of serially.
  2. By allowing approval-distribution to process messages from it's queue while approval-voting confirms that a message is valid we give the approval-distribution the ability to build a better view about what messages other peers already know, so it won't decide to gossip messages to some of it's peers once we confirm that message as being correct.
  3. It opens the door for other optimizations in approval-voting subsystem, which would still be the bottleneck.

Note!

I still expect the amount of work the combo of this two systems can do, to still be bounded by the numbers of signatures checks it has to do, so we would have to stack this with other optimizations we have in the queue.

TODO:

[] Evaluate impact in versi
[] Cleanup code an make CI happy to make the PR meargeable.

…voting

In, the current implementation every time we process an assignment or an
approval that needs checking in the approval voting, we will wait till
approval-voting answers the message.

Given that approval-voting will execute some signatures checks that take
significant time(between 400us and 1 millis) per message, that's where most of
the time in the approval-distribution, see
https://github.com/paritytech/polkadot/issues/6608#issuecomment-1590942235 for
the numbers.

So, modify approval-distribution, so that it picks another message from the
queue while the approval-voting is busy doing it's work.

This will have a few benefits:
1. Better pipelinening of the messages, approval-voting will always have work to
do and it won't have to wait for the approval-distribution to send it a message.
Additionally, some of the works of the approval-distribution will be executed in
parallel with work in approval-voting instead of serially.
2. By allowing approval-distribution to process messages from it's queue while
approval-voting confirms that a message is valid we give the
approval-distribution the ability to build a better view about what messages
other peers already know, so it won't decide to gossip messages to some of it's
peers once we confirm that message as being correct.
3. It opens the door for other optimizations in approval-voting subsystem, which
would still be the bottleneck.

Note!
I still expect the amount of work the combo of this two systems can do, to still
be bounded by the numbers of signatures checks it has to do, so we would have to
stack this with other optimizations we have in the queue.
- https://github.com/paritytech/polkadot/issues/6608
- https://github.com/paritytech/polkadot/issues/6831

[] Evaluate impact in versi
[] Cleanup code an make CI happy to make the PR meargeable.

Signed-off-by: Alexandru Gheorghe <[email protected]>
Signed-off-by: Alexandru Gheorghe <[email protected]>
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

This is the same approach as in #6247 but it's missing the back pressure mechanism or bounds on the futures unordered

@@ -837,6 +880,26 @@ impl State {
return
}

// The approval is in process of being verified, nothing to do here, we don't want to check it multiple times
// just mark that the peer knew about it, so we don't send it to him again
if entry.waiting_to_be_verified.contains(&message_subject, message_kind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

message_kind is assignment and assignment comes before the approval. This should be done in import approval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the comment is wrong it should say.

The assignment is in the process of being verified.

}
.boxed();
self.answers_from_approval_voting.push(await_future);
Copy link
Contributor

Choose a reason for hiding this comment

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

This essentially creates an unbounded subsystem internal channel

Copy link
Contributor

Choose a reason for hiding this comment

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

We'd need to limit the number of futures we can have at one time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it is not finished yet, in current state it is mostly to assess the behaviour in versi.

@sandreim
Copy link
Contributor

This is the same approach as in #6247 but it's missing the back pressure mechanism.

AFAIK #6247 showed some improvements when running at scale on Versi.

Signed-off-by: Alexandru Gheorghe <[email protected]>
@sandreim
Copy link
Contributor

sandreim commented Jun 19, 2023

#6285 is actually the latest work on the subject

@alexggh
Copy link
Contributor Author

alexggh commented Jun 20, 2023

Ran this in versi on a node, the behaviour is as expected, overall we don't improve the capacity of the approval-distribution/approval-voting duo to process messages, but we do push more messages into approval-voting to notice that's where the bottleneck. See bellow messages:

Screenshot 2023-06-20 at 10 36 36 Screenshot 2023-06-20 at 10 36 55

Notice that the time of flight for messages arriving in approval-distribution decreases significantly, but that just moves the bottleneck into the approval-voting where it actually is.

@alexggh
Copy link
Contributor Author

alexggh commented Jun 21, 2023

Update! Ignore the results for now because there seems to be some bugs in the implementation

Went a bit further and did some naive parallelisation in approval-voting where we have 6 workers per assignment and 6 workers per approval doing just the crypto work which is passed over a channel and once the check finished we continue on the main loop to save the approval and send the response back to approval-distribution.

The results confirm that the bottleneck is actually the amounts of signature checks we have to do, see how the measurements look over 20 minutes period.

Screenshot 2023-06-21 at 15 51 43 Screenshot 2023-06-21 at 15 52 00 Screenshot 2023-06-21 at 15 58 38

It can be seen in this graphs that there are no messages taking longer than 400ms to be processed, which is way better than the previous results where we had some messages waiting for 2-3 seconds in the queue.

Signed-off-by: Alexandru Gheorghe <[email protected]>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3108622

@alexggh
Copy link
Contributor Author

alexggh commented Jul 4, 2023

With the fixed implementation the behaviour of a node that process things in parallel versus one that doesn't looks like this.

  1. First we need to make sure that both nodes are doing the same amount of work and that can be seem the dashboards counting the number of assignments/approvals processed.
Screenshot 2023-07-04 at 10 38 19
  1. Looking at time of flight from approval-distribution that improves significantly.

Original version
Screenshot 2023-07-04 at 10 46 49

Parallelised version
Screenshot 2023-07-04 at 10 32 43

  1. However that doesn't tell the full story, since now we are just pushing quickly more work towards approval-voting, it does not mean it gets processed faster. To confirm that we will have to look at two metrics.
  • ToF for approval voting, which shows us how fast messages get picked by approval voting. This metric should get worse since now approval voting will have more work to do at the same time.

Original version
Screenshot 2023-07-04 at 10 36 27

Parallelised work
Screenshot 2023-07-04 at 10 33 47

  • Time awaiting approval voting - which tells us for each assignment/approval how much time the approval-distribution spent waiting for it to be processed, this metric is the best proxy to understand how fast are we processing each assignment/approval.

Parallelised version
Screenshot 2023-07-04 at 10 35 24

Original version
Screenshot 2023-07-04 at 10 46 49

Comparing the time awaiting approval-voting in the Parallelised version with the ToF for approval-distribution-subsystem in master, gives us a sense of which system processes assignment/approval faster, and looking at the above two pictures I would say it seems clearer that the parallelised version is significantly faster at processing approvals/assignments, with just rare occasions where we have 1-2 messages being in the 400ms-1.6s bucket.

With this data, I tend to concur that regardless of the directions we go with coalescing the approvals(paritytech/polkadot-sdk#701) to reduce the amount of work, we would still benefit from implementing a mechanism to push more work towards approval-voting and process the cpu intensive work(signatures checks for approvals and assignments) in parallel.

Note!:
There a few shortcuts that I took in this PoC, like having unbounded queues for the amount of work we push towards the approval-voting, but I don't expect that to change significantly the results.
The mergeable version will look more closely to an improved version of (paritytech/polkadot-sdk#701)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants