Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add EventLoop APIs for simpler scheduling of callbacks #2759
Add EventLoop APIs for simpler scheduling of callbacks #2759
Changes from all commits
5f7065f
33e82e0
b59e31d
17cf105
0afb3a2
df3cdb1
b0d3f66
907c087
b3e4903
fb5e835
89c57ec
59a04de
06a4ce7
950db0c
d6ae472
4439ac6
abd4b28
ceabc7b
80d91f6
2ec5543
5d6ac17
bbf8f9f
21fd1cc
9bf65f6
dbba9e3
86b8ac3
b16a575
4e71ffb
b909365
5559772
7f159be
3ad5647
bd8fa73
6af561d
81cc415
37ebfde
4467728
6544461
c48b7aa
151c2c8
5e61930
0a2baf3
ba63fda
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Do we need the
custom
here in the naming?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it adds value when glancing at it as the property name implies that it's only relevant for custom implementations. How strongly do you feel about it. It's public API so if there's a consensus that this needs a different name I'll suck it up 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One performance thought here. Currently we are storing an existential callback handler. However, we could just store the two closures itself which we can get while we are in the generic method. This way we would avoid calling through an existential on every scheduled callback task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requirements for
protocol NIOScheduledCallbackHandler
are generic functions. Is there a way I can store these as closures in the enum associated value, without making theKind
generic?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to add the second half of this. Yes this is why I think we should go back to
any EventLoop
. I assume the perf benefit outweighs this. This is an assumption but the scheduling and running of tasks is probably hotter than whatever we do in their callback with the passed EL. @Lukasa WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions being generic don't really matter here: we can store generic closures in the
Kind
type without promoting the generic to theKind
type itself. We won't get specialization, but that's fine.So TL;DR: yes, changing
ScheduledEventLoop
's representation to a pair of closures is probably the right thing to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some time working this and couldn't come up with a representation of this form that resulted in the amortized zero allocations we were looking for. Shall we shelve the suggestion on this thread? IIUC this is all internal anyway so we're not painting ourselves into a corner AFAICT?