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

Support faster send/recv by not notifying the other side #1039

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

Conversation

ryoqun
Copy link

@ryoqun ryoqun commented Nov 25, 2023

(this is a draft)

Introduce a new send variant called (send_buffered()), which doesn't notify receivers to avoid syscalls/context switches at the cost of increased latency.

sometimes, it's not desirable to incur the potential futex wake syscall cost for each and every message send.

as far as i can tell, there is no way to do like this in the current api:

// sender thread
// some tight looping
loop {
    ...
    let task = ...;
    if !needs_flush {
        sender.send_buffered(Frame::Payload(task)).unwrap();
    } else {
        sender.send(Frame::Flush).unwrap();
    }
}

// receiver thread
loop {
    // receiver.try_recv() and std::thread::sleep() can't be used
    // because we want Flush to be handled immediately.
    while let Ok(msg) = receier.recv_timeout(Duration::from_millis(20)) {
      ...
    }
}

naming

I haven't decided yet.. considering .recv(), {try_,}send_{timeout_,}unnotified()/{try_,}recv_{timeout_,}unnotified() might be better.

@taiki-e
Copy link
Member

taiki-e commented Nov 29, 2023

Thanks for the PR! I think it's actually less-syscall, not syscall-free, but this could be more efficient when sending multiple messages continuously. However, we would need a clear caveat in the documentation about the footgun: misuse of this will block the receiver forever.

As for the name, I think unnotified is clearer.

@ryoqun ryoqun changed the title Add syscall-free & cas-ony buffering send variant Support send/recv operations without notifying the other side Nov 30, 2023
@ryoqun
Copy link
Author

ryoqun commented Nov 30, 2023

@taiki-e really thanks for the initial feedback!

Thanks for the PR! I think it's actually less-syscall, not syscall-free, but this could be more efficient when sending multiple messages continuously.

thanks for understanding of the use case.

However, we would need a clear caveat in the documentation about the footgun: misuse of this will block the receiver forever.

agreed. added doc comments what that in mind.

As for the name, I think unnotified is clearer.

agreed with this too.

so, I've refreshed this pr with full coverage of all send methods for the unnotified variant and finished writing doc comments, also ci should be passing.

i have a few questions:

  • is it okay to add these bunch of _unnotified methods naively? i mean, as you can see, the diff is already daunting... ;) Or, some other api is desired instead like builder api or a generic one like send_with_options().
  • recv methods should be added for the unnotified variant as well before this pr can be merged for the sake of completeness?
  • also what about select!?

lastly, I'm planning to write some unit tests once the direction of this pr is good.

last and not least, really thanks for reviewing this pr. I'll try to be very responsive in coming days. my work is blocked on this and other open prs at crossbeam.. so, I'd like them to be shipped asap... :)

@ryoqun ryoqun changed the title Support send/recv operations without notifying the other side Support faster send/recv operations by not notifying the other side Nov 30, 2023
@ryoqun ryoqun changed the title Support faster send/recv operations by not notifying the other side Support faster send/recv by not notifying the other side Nov 30, 2023
@ryoqun ryoqun marked this pull request as ready for review December 17, 2023 02:24
@ibraheemdev
Copy link
Contributor

send_silent might be a good name for this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants