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 18 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.
Can we align the maximum duration/iterations with #2839
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.
Can I gently push back on that ask.
You're asking me to align with a new precedent in an un-merged PR that was opened well after this one, instead of this PR keeping the conventions of the branch it is targeting.
This PR has been subject to a lot of scope creep already.
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 see it's been merged already. So I've now merged this PR with main and updated the benchmarks to use the same configuration as the rest.
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 don't think we should do this setup inside the benchmark.
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.
Why not? IIUC, that's what
startMeasurement
is for.If you're suggesting that I use the
setup
andteardown
closures of theBenchmark()
function, then I cannot since I need to actually use these variables in the benchmark. The only alternative would be to make them implicitly unwrapped optionals at global scope, which was pretty gross.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.
We can reuse ELs across benchmarks so I was thinking it might just be better if we create one EL that we share. I would just define a
let eventLoop = MultithreadedEventLoopGroup.singleton.any()
at the top.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'm not sure that's great, I think I'd rather each benchmark do it's own setup and teardown to ensure its unpolluted by the side effects of running other benchmarks.
Where we can, I'm happy to try and use the
setup
/teardown
closure style, if you prefer it, and where we cannot, use this.It also has the benefit for local reasoning of what's being benchmarked. It's all contained in the call to
Benchmark { ... }
.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 disagree and I just looked at the code in the benchmark again and we are already defining an EL for exactly this purpose. There is a static EL defined in this file called
eventLoop
.The reason why I think it's not important to keep is that it should be irrelevant for the benchmark here. The important part of the benchmark is the scheduling and not how the EL is constructed and shut down so it keeps the benchmark more concise.
IMO we should definitely only have one style here and not mix static EL with one EL per benchmark.
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.
While I still disagree with this feedback and think we should change the other benchmarks too for local reasoning reasons, I'm more interested in converging this PR, so I've updated to accommodate this feedback.
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 naming reads (to me) like this function is called at the time when the callback is scheduled (when
eventLoop.scheduledCallback
is called), as opposed to the time when the callback is scheduled to run.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.
handleScheduledCallback
?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've updated the name to
handleScheduledCallback
in 2ec5543.Question: do we need to be more considerate about name clashes since we're expecting folks to conform their types to this protocol; e.g. should this be something like
handleNIOScheduledCallback
?This case isn't quite covered by https://github.com/apple/swift-nio/blob/main/docs/public-api.md, but it seems similar in nature.
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.
@glbrntt did you have any thoughts regarding the namespacing or is it fine the way it is?
The function takes a NIO type, so at worst it could cause an overload? Maybe it's OK the way it is now.
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 😄