-
Notifications
You must be signed in to change notification settings - Fork 9
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
Optimize waker size for requirements #40
base: main
Are you sure you want to change the base?
Conversation
4c9e0f1
to
35222da
Compare
35222da
to
639584d
Compare
Hi! Thanks for wanting to contribute. This looks cool and all, but I have some concerns.
Features need to be additive only. I'm not sure if changing the type of Since I wrote the issue I have maybe changed opinion slightly. This change is pretty complex, and does it really save a lot of space? For this to be worth the extra complexity someone must use this in a way where they have tiny messages, and a huge amount of channels. Like with almost any optimization, I think it would be nice if we had any concrete proof this change is justified by real world usage/benchmarks. |
It's possible that this has small CPU benefits as well due to each waker being simpler than "support all waking methods". However it's really hard to measure. It's hard because you can only perform a single wake event per channel (by design), and allocation and other things related to creating and destroying each channel probably outweigh the waking part by a lot. But yeah, if we can't prove a performance win, then it's probably not worth the complexity anyway, since the same applies for production use, not just benchmarks. |
Yeah, those are fair concerns (: I'm also very unsure whether this is worth the effort! (that's part of the reason this is a draft pr)
Hmm, unless someone relies on the result of
I can't imagine there are any measurable performance wins here - at most you're saving one |
This is the result of me tinkering around with the idea described in #4, I would like your approval/disapproval on the basic idea before i write documentation etc.
TLDR; It's possible to reduce the size of a
ReceiverWaker
if it is known that it will only be used in blocking or non-blocking contexts beforehand. This is currently being decided using crate features, but it would be nice if it was possible to specify this for each channel individually.Design Idea
This PR uses two traits (
SyncWaker
/AsyncWaker
) to expose different methods onReceiver
based on what waker is being used.There are four different wakers, which are equivalent to the four possible feature combinations right now:
SyncWaker
AsyncWaker
Receiver<T, W>
DummyWaker
try_recv
onlythread::Thread
task::Waker
GenericWaker
To avoid breaking changes, a
DefaultWaker
is also exported, which equates to the "most permissive" of the available wakers based on the enabled features and, as the name implies, is used by default.The main design issue currently is how to adjust the crate API so the used waker can be specified.
Currently, channels are being constructed using
Ideally this would be possible:
but default values for generic parameters on functions are not allowed.
So the "next best" option would be to either export more constructor functions:
or to move these functions onto the
Channel
type itself, so the waker type is already clear.