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

Send signals in parallel #51

Merged
merged 3 commits into from
Jul 17, 2023

Conversation

vstakhov
Copy link
Contributor

@vstakhov vstakhov commented Jul 7, 2023

This PR addresses #18

Rough architecture description.

  1. We first try to send signals via try_send_signal for each subsystem. This is not an async function and it is blazingly fast for the vast majority of the cases. So it is a fast path.
  2. If some subsystem is slow, then we create a new future where we perform the real async send_signal function.
  3. These futures are joint in FuturesUnordered to allow unordered polling. When some subsystem gets ready to accept a signal it immediately receives that signal.

With such an approach, we do not create unnecessary futures nor vectors of futures and we just utilise the fast path via try_send in the majority of this function calls.

@vstakhov
Copy link
Contributor Author

vstakhov commented Jul 7, 2023

This PR is now based against the master branch to allow CI to do it's job. I will rebase it later.

@vstakhov
Copy link
Contributor Author

vstakhov commented Jul 7, 2023

It seems I cannot now change the base branch for this PR :(

@vstakhov vstakhov changed the base branch from master to vstakhov-feature-async-channels July 7, 2023 17:29
@vstakhov
Copy link
Contributor Author

vstakhov commented Jul 7, 2023

It seems I cannot now change the base branch for this PR :(

Ok, Github has fixed this issue I suppose.

Copy link
Collaborator

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A few idiomatic suggestions and a missing doc comment, otherwise LGTM

vstakhov and others added 2 commits July 10, 2023 13:55
@vstakhov vstakhov requested a review from drahnr July 10, 2023 13:00
Copy link
Collaborator

@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.

Nice! This should likely be more useful outside the context of Polkadot maybe, because a subsystem not handling signals in time is likely stuck because of cycles or other bugs.

@drahnr drahnr merged commit 8b6e817 into vstakhov-feature-async-channels Jul 17, 2023
@drahnr drahnr deleted the vstakhov-parallel-signals branch July 17, 2023 12:44
vstakhov added a commit that referenced this pull request Jul 18, 2023
#48)

* Allow to select between channels implementations

* Fix tests

* Add futures-channel tests in CI

* Fix check job

* feat: send signals in parallel (#51)

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

---------

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
vstakhov added a commit that referenced this pull request Jul 18, 2023
#48)

* Allow to select between channels implementations

* Fix tests

* Add futures-channel tests in CI

* Fix check job

* feat: send signals in parallel (#51)

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

---------

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
vstakhov added a commit that referenced this pull request Jul 18, 2023
* Add benchmarks

* Move thread-pool feature to dev dependencies

* Allow to use both async-channel and futures channels via feature flags (#48)

* Allow to select between channels implementations

* Fix tests

* Add futures-channel tests in CI

* Fix check job

* feat: send signals in parallel (#51)

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

---------

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>

* Fix merge issue

* Another version fix

---------

Co-authored-by: Bernhard Schuster <bernhard@ahoi.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants