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

Use spawn_blocking when processing synedrion messages #1285

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

ameba23
Copy link
Contributor

@ameba23 ameba23 commented Feb 6, 2025

This was suggested by @fjarri here:

entropyxyz/manul#91 (comment)

Since synedrion's message processing methods are synchronous and computationally heavy, it does not make sense to run them in async tokio tasks as this may prevent the executor from moving other tasks forward.

This PR uses tokio::task::spawn_blocking to run them as blocking, syncronous tasks instead.

Unfortunately this does not appear to give any improvement in performance when testing signing with 8 parties.

Since we will be soon swapping this out for the protocol execution loop provided by manul, it probably makes no difference whether or not we merge this. But gonna put it up anyway.

@@ -154,7 +154,8 @@ where
let tx = process_tx.clone();
tokio::spawn(async move {
let result = session_arc.process_message(&mut OsRng, preprocessed);
if tx.send(result).await.is_err() {

if futures::executor::block_on(tx.send(result)).is_err() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we have the issue that the channel is async but we are now in a sync block. However because tx.send is likely to be considerably faster than session.process_message, i think it still makes sense to do this.

The alternative would be to use std::sync::mpsc rather than tokio::sync::mpsc, but then we have the problem that we can't use the receiver with tokio::select!.

Copy link
Collaborator

@HCastano HCastano left a comment

Choose a reason for hiding this comment

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

I don't really know what's going on behind the scenes in the protocol session code, but from the spawn_blocking link you provided it mentions that if the workload is CPU bound we could benefit from using something like Rayon.

Maybe worth looking into in a different PR.

@fjarri
Copy link
Member

fjarri commented Feb 10, 2025

I am not sure rayon would fit naturally here since we need to create new tasks and add them to the pool as we receive messages. But maybe it allows that too. In any case, in my tests spawn_blocking works well enough.

@ameba23 ameba23 merged commit 4a2bce4 into master Feb 11, 2025
8 checks passed
@ameba23 ameba23 deleted the peg/protocol-execute-spawn-blocking branch February 11, 2025 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants