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 Channels rather than BlockingCollection for block processing #7790

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

benaadams
Copy link
Member

Changes

  • Wrapping a ConcurrentQueue in a BlockingCollection burns a ton of CPU spinning; whereas its a pattern ideally suited to Channels; and using true async for rather than TaskCompletionSource for async is lower overhead.
  • Faster full sync

Types of changes

What types of changes does your code introduce?

  • Optimization
  • Refactoring

Testing

Requires testing

  • No

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Recovery thread should also have boosted priority.

My main concern is that before recovery and processing had dedicated threads and now they will compete on thread pool. Should we create a LongRunning task for that or handle it with custom scheduler that will onw a thread?

Can you demonstrate the gain?

{
TaskCompletionSource tcs = new();
_isMainProcessingThread.Value = IsMainProcessor;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is used to determine if tries should be recovered from network and whether the tx pool should be notified of invalidated accounts in its cache (both should only happen from the main processor)

@benaadams
Copy link
Member Author

Recovery thread should also have boosted priority.

Is start of block; is either not doing anything or is doing catch up when main thread is processing and then its competing with the Parallel.For s in main processing

My main concern is that before recovery and processing had dedicated threads and now they will compete on thread pool.

Can't use async on dedicated thread or loose the thread on first await. Don't think is hugely significant; still boost main processing and Recovery doesn't out compete all the parallel processing of main thread.

@LukaszRozmej
Copy link
Member

Would be good to remove other BlockingCollections too, especially the long running ones

@benaadams
Copy link
Member Author

Would be good to remove other BlockingCollections too, especially the long running ones

One of them breaks CI, so going to submit as separate PRs

@benaadams
Copy link
Member Author

Fixed; PRs #7846, #7845, #7844,

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

Successfully merging this pull request may close these issues.

2 participants