-
Notifications
You must be signed in to change notification settings - Fork 650
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
Changes from 37 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
//===----------------------------------------------------------------------===// | ||
|
||
import Benchmark | ||
import NIOCore | ||
import NIOPosix | ||
|
||
private let eventLoop = MultiThreadedEventLoopGroup.singleton.next() | ||
|
@@ -64,4 +65,43 @@ let benchmarks = { | |
) | ||
} | ||
#endif | ||
|
||
Benchmark( | ||
"MTELG.scheduleTask(in:_:)", | ||
configuration: Benchmark.Configuration( | ||
metrics: [.mallocCountTotal, .instructions], | ||
scalingFactor: .kilo | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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. |
||
) | ||
) { benchmark in | ||
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) | ||
defer { try! group.syncShutdownGracefully() } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Why not? IIUC, that's what If you're suggesting that I use the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 It also has the benefit for local reasoning of what's being benchmarked. It's all contained in the call to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe 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. |
||
let loop = group.next() | ||
|
||
benchmark.startMeasurement() | ||
for _ in benchmark.scaledIterations { | ||
loop.scheduleTask(in: .hours(1), {}) | ||
} | ||
} | ||
|
||
Benchmark( | ||
"MTELG.scheduleCallback(in:_:)", | ||
configuration: Benchmark.Configuration( | ||
metrics: [.mallocCountTotal, .instructions], | ||
scalingFactor: .kilo | ||
) | ||
) { benchmark in | ||
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) | ||
defer { try! group.syncShutdownGracefully() } | ||
let loop = group.next() | ||
|
||
final class Timer: NIOScheduledCallbackHandler { | ||
func handleScheduledCallback(eventLoop: some EventLoop) {} | ||
} | ||
let timer = Timer() | ||
|
||
benchmark.startMeasurement() | ||
for _ in benchmark.scaledIterations { | ||
let handle = try! loop.scheduleCallback(in: .hours(1), handler: timer) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,166 @@ | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This source file is part of the SwiftNIO open source project | ||
// | ||
// Copyright (c) 2024 Apple Inc. and the SwiftNIO project authors | ||
// Licensed under Apache License v2.0 | ||
// | ||
// See LICENSE.txt for license information | ||
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
/// A type that handles callbacks scheduled with `EventLoop.scheduleCallback(at:handler:)`. | ||
/// | ||
/// - Seealso: `EventLoop.scheduleCallback(at:handler:)`. | ||
public protocol NIOScheduledCallbackHandler { | ||
/// This function is called at the scheduled time, unless the scheduled callback is cancelled. | ||
/// | ||
/// - Parameter eventLoop: The event loop on which the callback was scheduled. | ||
func handleScheduledCallback(eventLoop: some EventLoop) | ||
simonjbeaumont marked this conversation as resolved.
Show resolved
Hide resolved
FranzBusch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// This function is called if the scheduled callback is cancelled. | ||
/// | ||
/// The callback could be cancelled explictily, by the user calling ``NIOScheduledCallback/cancel()``, or | ||
/// implicitly, if it was still pending when the event loop was shut down. | ||
/// | ||
/// - Parameter eventLoop: The event loop on which the callback was scheduled. | ||
func onCancelScheduledCallback(eventLoop: some EventLoop) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ultra naming nit: Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that A quick However, it's fair that the only
I can change to using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
} | ||
|
||
extension NIOScheduledCallbackHandler { | ||
/// Default implementation of `onCancelScheduledCallback(eventLoop:)`: does nothing. | ||
public func onCancelScheduledCallback(eventLoop: some EventLoop) {} | ||
} | ||
|
||
/// An opaque handle that can be used to cancel a scheduled callback. | ||
/// | ||
/// Users should not create an instance of this type; it is returned by `EventLoop.scheduleCallback(at:handler:)`. | ||
/// | ||
/// - Seealso: `EventLoop.scheduleCallback(at:handler:)`. | ||
public struct NIOScheduledCallback: Sendable { | ||
@usableFromInline | ||
enum Backing: Sendable { | ||
/// A task created using `EventLoop.scheduleTask(deadline:_:)` by the default implementation. | ||
case `default`(_ task: Scheduled<Void>) | ||
/// A custom callback identifier, used by event loops that provide a custom implementation. | ||
case custom(id: UInt64) | ||
} | ||
|
||
@usableFromInline | ||
var eventLoop: any EventLoop | ||
|
||
@usableFromInline | ||
var backing: Backing | ||
|
||
/// This initializer is only for the default implementation and is fileprivate to avoid use in EL implementations. | ||
fileprivate init(_ eventLoop: any EventLoop, _ task: Scheduled<Void>) { | ||
self.eventLoop = eventLoop | ||
self.backing = .default(task) | ||
} | ||
|
||
/// Create a handle for the scheduled callback with an opaque identifier managed by the event loop. | ||
/// | ||
/// - NOTE: This initializer is for event loop implementors only, end users should use `EventLoop.scheduleCallback`. | ||
/// | ||
/// - Seealso: `EventLoop.scheduleCallback(at:handler:)`. | ||
@inlinable | ||
public init(_ eventLoop: any EventLoop, id: UInt64) { | ||
self.eventLoop = eventLoop | ||
self.backing = .custom(id: id) | ||
} | ||
|
||
/// Cancel the scheduled callback associated with this handle. | ||
@inlinable | ||
public func cancel() { | ||
self.eventLoop.cancelScheduledCallback(self) | ||
} | ||
|
||
/// The callback identifier, if the event loop uses a custom scheduled callback implementation; nil otherwise. | ||
/// | ||
/// - NOTE: This property is for event loop implementors only. | ||
@inlinable | ||
public var customCallbackID: UInt64? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: Do we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😄 |
||
guard case .custom(let id) = self.backing else { return nil } | ||
return id | ||
} | ||
} | ||
|
||
extension EventLoop { | ||
// This could be package once we drop Swift 5.8. | ||
public func _scheduleCallback( | ||
at deadline: NIODeadline, | ||
handler: some NIOScheduledCallbackHandler | ||
) -> NIOScheduledCallback { | ||
let task = self.scheduleTask(deadline: deadline) { handler.handleScheduledCallback(eventLoop: self) } | ||
task.futureResult.whenFailure { error in | ||
if case .cancelled = error as? EventLoopError { | ||
handler.onCancelScheduledCallback(eventLoop: self) | ||
} | ||
} | ||
return NIOScheduledCallback(self, task) | ||
} | ||
|
||
/// Default implementation of `scheduleCallback(at deadline:handler:)`: backed by `EventLoop.scheduleTask`. | ||
/// | ||
/// Ideally the scheduled callback handler should be called exactly once for each call to `scheduleCallback`: | ||
/// either the callback handler, or the cancellation handler. | ||
/// | ||
/// In order to support cancellation in the default implementation, we hook the future of the scheduled task | ||
/// backing the scheduled callback. This requires two calls to the event loop: `EventLoop.scheduleTask`, and | ||
/// `EventLoopFuture.whenFailure`, both of which queue onto the event loop if called from off the event loop. | ||
/// | ||
/// This can present a challenge during event loop shutdown, where typically: | ||
/// 1. Scheduled work that is past its deadline gets run. | ||
/// 2. Scheduled future work gets cancelled. | ||
/// 3. New work resulting from (1) and (2) gets handled differently depending on the EL: | ||
/// a. `SelectableEventLoop` runs new work recursively and crashes if not quiesced in some number of ticks. | ||
/// b. `EmbeddedEventLoop` and `NIOAsyncTestingEventLoop` will fail incoming work. | ||
/// | ||
/// `SelectableEventLoop` has a custom implementation for scheduled callbacks so warrants no further discussion. | ||
/// | ||
/// As a practical matter, the `EmbeddedEventLoop` is OK because it shares the thread of the caller, but for | ||
/// other event loops (including any outside this repo), it's possible that the call to shutdown interleaves | ||
/// with the call to create the scheduled task and the call to hook the task future. | ||
/// | ||
/// Because this API is synchronous and we cannot block the calling thread, users of event loops with this | ||
/// default implementation will have cancellation callbacks delivered on a best-effort basis when the event loop | ||
/// is shutdown and depends on how the event loop deals with newly scheduled tasks during shutdown. | ||
/// | ||
/// The implementation of this default conformance has been further factored out so we can use it in | ||
/// `NIOAsyncTestingEventLoop`, where the use of `wait()` is _less bad_. | ||
@discardableResult | ||
public func scheduleCallback( | ||
at deadline: NIODeadline, | ||
handler: some NIOScheduledCallbackHandler | ||
) -> NIOScheduledCallback { | ||
self._scheduleCallback(at: deadline, handler: handler) | ||
} | ||
|
||
/// Default implementation of `scheduleCallback(in amount:handler:)`: calls `scheduleCallback(at deadline:handler:)`. | ||
@discardableResult | ||
@inlinable | ||
public func scheduleCallback( | ||
in amount: TimeAmount, | ||
handler: some NIOScheduledCallbackHandler | ||
) throws -> NIOScheduledCallback { | ||
try self.scheduleCallback(at: .now() + amount, handler: handler) | ||
} | ||
|
||
/// Default implementation of `cancelScheduledCallback(_:)`: only cancels callbacks scheduled by the default implementation of `scheduleCallback`. | ||
/// | ||
/// - NOTE: Event loops that provide a custom scheduled callback implementation **must** implement _both_ | ||
/// `sheduleCallback(at deadline:handler:)` _and_ `cancelScheduledCallback(_:)`. Failure to do so will | ||
/// result in a runtime error. | ||
@inlinable | ||
public func cancelScheduledCallback(_ scheduledCallback: NIOScheduledCallback) { | ||
switch scheduledCallback.backing { | ||
case .default(let task): | ||
task.cancel() | ||
case .custom: | ||
preconditionFailure("EventLoop missing custom implementation of cancelScheduledCallback(_:)") | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -508,28 +508,38 @@ struct ErasedUnownedJob { | |
|
||
@usableFromInline | ||
internal struct ScheduledTask { | ||
@usableFromInline | ||
enum Kind { | ||
case task(task: () -> Void, failFn: (Error) -> Void) | ||
case callback(any NIOScheduledCallbackHandler) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. The requirements for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 So TL;DR: yes, changing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
} | ||
|
||
@usableFromInline | ||
let kind: Kind | ||
|
||
/// The id of the scheduled task. | ||
/// | ||
/// - Important: This id has two purposes. First, it is used to give this struct an identity so that we can implement ``Equatable`` | ||
/// Second, it is used to give the tasks an order which we use to execute them. | ||
/// This means, the ids need to be unique for a given ``SelectableEventLoop`` and they need to be in ascending order. | ||
@usableFromInline | ||
let id: UInt64 | ||
let task: () -> Void | ||
private let failFn: (Error) -> Void | ||
|
||
@usableFromInline | ||
internal let readyTime: NIODeadline | ||
|
||
@usableFromInline | ||
init(id: UInt64, _ task: @escaping () -> Void, _ failFn: @escaping (Error) -> Void, _ time: NIODeadline) { | ||
self.id = id | ||
self.task = task | ||
self.failFn = failFn | ||
self.readyTime = time | ||
self.kind = .task(task: task, failFn: failFn) | ||
} | ||
|
||
func fail(_ error: Error) { | ||
failFn(error) | ||
@usableFromInline | ||
init(id: UInt64, _ handler: any NIOScheduledCallbackHandler, _ time: NIODeadline) { | ||
self.id = id | ||
self.readyTime = time | ||
self.kind = .callback(handler) | ||
} | ||
} | ||
|
||
|
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 use our
defaultMetrics
here? We don't have instruction based benchmarks yetThere 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.
ISTR using
.instructions
because you asked me too, in place of wall clock time. I can usedefaultMetrics
though, sure.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.
Done.