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.

Sign up for GitHub

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

Jump to bottom

cleanup(consensus): Avoid blocking threads by awaiting proof verification results from rayon in async context #6887

Merged
merged 9 commits into from
Jul 12, 2023
Merged

cleanup(consensus): Avoid blocking threads by awaiting proof verification results from rayon in async context #6887

merged 9 commits into from
Jul 12, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jun 9, 2023 •
edited by teor2345
Loading

Motivation

This PR updates verify_single_spawning() and flush_spawning() methods to delegate CPU-bound work using rayon instead of blocking tokio threads.

rayon::iter::once admits no parallelism on its own and will execute operations on the current thread if collected without being chained to another parallel iterator.

Depends-On: #7158

Solution

  • Replace tokio::spawn_blocking() calls with oneshot channels and rayon::spawn_fifo()

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@arya2 arya2 added A-consensus Area: Consensus rule updates P-Medium ⚡ A-concurrency Area: Async code, needs extra work to make it work properly. labels Jun 9, 2023
@arya2 arya2 requested a review from teor2345 June 9, 2023 01:10
@arya2 arya2 requested a review from a team as a code owner June 9, 2023 01:10
@arya2 arya2 self-assigned this Jun 9, 2023
@github-actions github-actions bot added the C-bug Category: This is a bug label Jun 9, 2023
teor2345

This comment was marked as outdated.

Sign in to view

@codecov

This comment was marked as outdated.

Sign in to view
@arya2

This comment was marked as outdated.

Sign in to view
@teor2345 teor2345 added the do-not-merge Tells Mergify not to merge this PR label Jun 9, 2023
@teor2345 teor2345 dismissed their stale review June 9, 2023 02:48

Not needed yet

@arya2 arya2 changed the title fix(consensus): Use rayon for CPU-bound proof verification cleanup(consensus): Avoid spawning excess threads in proof verifiers Jun 9, 2023
@dconnolly dconnolly removed the do-not-merge Tells Mergify not to merge this PR label Jun 14, 2023
teor2345

This comment was marked as resolved.

Sign in to view

@arya2 arya2 added the S-waiting-on-author Status: waiting on the ticket or PR author label Jun 21, 2023
@teor2345 teor2345 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Jun 30, 2023
@arya2 arya2 marked this pull request as draft July 3, 2023 22:06
@teor2345
Copy link
Contributor

teor2345 commented Jul 4, 2023

Let's try to merge this PR a while before a release, so it gets time on the main branch for testing unexpected behaviour or panics?

@github-actions github-actions bot added C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jul 6, 2023
@arya2

This comment was marked as outdated.

Sign in to view
@arya2

This comment was marked as resolved.

Sign in to view
@teor2345

This comment was marked as resolved.

Sign in to view
teor2345
teor2345 approved these changes Jul 9, 2023
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

The code looks good, thanks!

@teor2345

This comment was marked as resolved.

Sign in to view
@teor2345

This comment was marked as resolved.

Sign in to view
conradoplg
conradoplg approved these changes Jul 10, 2023
View reviewed changes
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Looks good!

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jul 11, 2023

update

✅ Branch has been successfully updated

teor2345
teor2345 approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@teor2345 teor2345 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 good to go!

@teor2345
Copy link
Contributor

Looks like mergify is waiting for the lightwalletd tip / Run lwd-full-sync test:
https://github.com/ZcashFoundation/zebra/pull/6887/checks?check_run_id=14929147913

Screenshot 2023-07-12 at 13 50 44

But that patch job actually ran:
https://github.com/ZcashFoundation/zebra/actions/runs/5514322056/jobs/10053440684?pr=6887

I can't work out what went wrong here, so I'm going to re-try running the patch job.

@teor2345
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2023

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Jul 12, 2023
@mergify
Merge of #6887
380ef47
@mergify mergify bot mentioned this pull request Jul 12, 2023
Closed
mergify bot added a commit that referenced this pull request Jul 12, 2023
@mergify
Merge of #6887
4e5a5d1
@mergify mergify bot mentioned this pull request Jul 12, 2023
Closed
@teor2345
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jul 12, 2023

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Jul 12, 2023
@mergify
Merge of #6887
5be9262
@mergify mergify bot mentioned this pull request Jul 12, 2023
Closed
@mergify mergify bot merged commit 797df67 into main Jul 12, 2023
@mergify mergify bot deleted the spawn-verify-proof branch July 12, 2023 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teor2345 teor2345 teor2345 approved these changes

@conradoplg conradoplg conradoplg approved these changes

Assignees

@arya2 arya2

Labels
A-concurrency Area: Async code, needs extra work to make it work properly. A-consensus Area: Consensus rule updates A-cryptography Area: Cryptography related C-bug Category: This is a bug C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG extra-reviews This PR needs at least 2 reviews to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arya2 @teor2345 @conradoplg @dconnolly