Skip to content

Add select_biased! macro - #1040

Merged
taiki-e merged 5 commits into
crossbeam-rs:masterfrom
ryoqun:biased-channel-select
May 19, 2024
Merged

Add select_biased! macro#1040
taiki-e merged 5 commits into
crossbeam-rs:masterfrom
ryoqun:biased-channel-select

Conversation

@ryoqun

@ryoqun ryoqun commented Nov 25, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

(this is a draft; there's too much of code duplication...)

much like rust-lang/futures-rs#1976, i want biased select in crossbeam-channel as well. :)

@ryoqun

ryoqun commented Nov 25, 2023

Copy link
Copy Markdown
Contributor Author

@taiki-e

thanks for maintaining this crate. :)

i'll group my gentle reminders with this single comment. but, I'd want some initial feedback for following draft prs. if there's some hope for merging them, I'll finish off these to cross the line, very happily:

@ryoqun ryoqun mentioned this pull request Dec 8, 2023
Draft
@ryoqun

ryoqun commented Jan 9, 2024

Copy link
Copy Markdown
Contributor Author

@taiki-e hey, thanks for active crossbeam maintenance. could you give this pr a initial assessment whether this pr can be merge or not ever? like other my prs, I'd like all of them to be merged eventually in some way...

thanks in advance.

@taiki-e

taiki-e commented May 2, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Sorry for the slow response, I thought this was a patch that was 1k lines of complex implementation without any tests or documentation.

I would like to merge this once the tests and documentation indicating that this is properly biased have been added, and eliminated duplicate code by something like passing a bool literal to an internal macro.

@ryoqun
ryoqun force-pushed the biased-channel-select branch from d37a73a to 364bda6 Compare May 4, 2024 09:00
@ryoqun

ryoqun commented May 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@taiki-e

Sorry for the slow response

no problem. i really thanks for replying!

I thought this was a patch that was 1k lines of complex implementation without any tests or documentation.

sorry for confusion... I meant this is just pre-rfc api addition with copy-paste impl by marking it as draft pr... but i lacked explicit explanation...

I would like to merge this ...

super cool!! I just wanted to confirm that this api addition isn't controversial as much as other prs... ;)

... once the tests and documentation indicating that this is properly biased have been added, and eliminated duplicate code by something like passing a bool literal to an internal macro.

I've just done all the above after rebased. Also marked it as ready for review. :) Thanks in advance.

@ryoqun
ryoqun marked this pull request as ready for review May 4, 2024 09:04
ryoqun
ryoqun commented May 4, 2024
View reviewed changes
Comment thread crossbeam-channel/tests/select_macro.rs
}

#[test]
fn unfairness() {

@ryoqun ryoqun May 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i omitted more test cases like send one. let me know if i need to add more...

ryoqun
ryoqun commented May 4, 2024
View reviewed changes
Comment thread crossbeam-channel/src/select_macro.rs
macro_rules! select_biased {
($($tokens:tt)*) => {
{
const _IS_BIASED: bool = true;

@ryoqun ryoqun May 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i think this was easiest way to implement... but this is accessible to user code. however, it seems we have precedent: _LEN.

@ryoqun ryoqun mentioned this pull request May 4, 2024
Closed
@ryoqun

ryoqun commented May 4, 2024

Copy link
Copy Markdown
Contributor Author

I've just done all the above after rebased. Also marked it as ready for review. :) Thanks in advance.

sorry, i pushed more to fix bugs since this comment...

taiki-e
taiki-e approved these changes May 13, 2024
View reviewed changes

@taiki-e taiki-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks @ryoqun!

@ryoqun

ryoqun commented May 14, 2024

Copy link
Copy Markdown
Contributor Author

@taiki-e Really and really thanks for approving this pr. Then, is this pr in a kind of internal testing now before merging? Also, do i need to add entries to crossbeam-channel/CHANGELOG.md via separate pr? I'm not sure the release process of crossbeam-channel... Anyway, I'm very looking forward to seeing my first pr for the crossbeam project to be shipped. :)

@taiki-e

taiki-e commented May 14, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

is this pr in a kind of internal testing now before merging

CI is failing on the master branch (mostly due to rust-lang/rust#124800) and we need to fix that before merging PRs...
https://github.com/crossbeam-rs/crossbeam/actions/runs/9072283469

@ryoqun

ryoqun commented May 14, 2024

Copy link
Copy Markdown
Contributor Author

is this pr in a kind of internal testing now before merging

CI is failing on the master branch (mostly due to rust-lang/rust#124800) and we need to fix that before merging PRs... https://github.com/crossbeam-rs/crossbeam/actions/runs/9072283469

Thanks for the quick reply. hmm, that's quite unfortunate, considering this pr is so close to be shipped... I just skimmed over the rust issue and its background. That said, I wonder how soon this resolves upstream..

Maybe pinning to older nightly (nightly-2024-05-04 or earlier according to https://blog.rust-lang.org/2024/05/06/check-cfg.html) in crossbeam CI isn't a viable option?

@ryoqun ryoqun mentioned this pull request May 15, 2024
Closed
@ryoqun

ryoqun commented May 15, 2024

Copy link
Copy Markdown
Contributor Author

we need to fix that before merging PRs...
https://github.com/crossbeam-rs/crossbeam/actions/runs/9072283469

@taiki-e fyi, i fixed that: #1109 :)

@taiki-e
taiki-e merged commit c9c5b9c into crossbeam-rs:master May 19, 2024
@ryoqun

ryoqun commented May 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@taiki-e I'm really appreciate merging this pr (and the ci fix pr as well). seems the master branch ci is green as well. Now, the only thing I'm looking forward to is crossbeam-channel 0.5.13... sorry for asking too much :)

taiki-e pushed a commit that referenced this pull request May 19, 2024
@ryoqun @taiki-e
Add select_biased! macro (#1040)
3f2ee7d
@taiki-e taiki-e mentioned this pull request May 19, 2024
Merged
taiki-e pushed a commit that referenced this pull request May 19, 2024
@ryoqun @taiki-e
Add select_biased! macro (#1040)
91edb88
@taiki-e

taiki-e commented May 19, 2024

Copy link
Copy Markdown
Member

Published in crossbeam-channel 0.5.13.

@ryoqun ryoqun mentioned this pull request May 20, 2024
Merged
@ryoqun ryoqun mentioned this pull request May 22, 2024
Closed
@ryoqun

ryoqun commented May 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@taiki-e hi, I'm reporting back with a good news. :) Really thanks to your collaboration with this pr shipped, using select_biased! resulted in 12-13% performance improvement: anza-xyz/agave#1437. and the pr just got merged there.

@ryoqun ryoqun mentioned this pull request May 22, 2024
Open
@SimonSapin SimonSapin mentioned this pull request Nov 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@taiki-e taiki-e taiki-e approved these changes

Assignees

No one assigned

Labels

crossbeam-channel

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ryoqun @taiki-e