Skip to content

Move Isolated EL operations to EL protocol #3070

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.

Sign up for GitHub

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

Merged
Lukasa merged 4 commits into from
Jan 20, 2025
Merged

Move Isolated EL operations to EL protocol #3070

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
db61aa1
Move Isolated EL operations to EL protocol
Lukasa Jan 17, 2025
f45e6dc
Already started improving matters
Lukasa Jan 17, 2025
b98d8cb
Make implementations inlinable
Lukasa Jan 20, 2025
3923209
Merge branch 'main' into cb-move-isolated-el-operations-to-protocol
glbrntt Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion IntegrationTests/tests_04_performance/Thresholds/5.10.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"encode_1000_ws_frames_new_buffer_with_space_with_mask": 5050,
"execute_hop_10000_tasks": 0,
"flat_schedule_10000_tasks": 130100,
"flat_schedule_assume_isolated_10000_tasks": 150100,
"flat_schedule_assume_isolated_10000_tasks": 110100,
"future_assume_isolated_lots_of_callbacks": 74050,
"future_erase_result": 4050,
"future_lots_of_callbacks": 74050,
Expand Down
2 changes: 1 addition & 1 deletion IntegrationTests/tests_04_performance/Thresholds/5.9.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"encode_1000_ws_frames_new_buffer_with_space_with_mask": 5050,
"execute_hop_10000_tasks": 0,
"flat_schedule_10000_tasks": 130100,
"flat_schedule_assume_isolated_10000_tasks": 150100,
"flat_schedule_assume_isolated_10000_tasks": 110100,
"future_assume_isolated_lots_of_callbacks": 74050,
"future_erase_result": 4050,
"future_lots_of_callbacks": 74050,
Expand Down
2 changes: 1 addition & 1 deletion IntegrationTests/tests_04_performance/Thresholds/6.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"encode_1000_ws_frames_new_buffer_with_space_with_mask": 5050,
"execute_hop_10000_tasks": 0,
"flat_schedule_10000_tasks": 130100,
"flat_schedule_assume_isolated_10000_tasks": 150100,
"flat_schedule_assume_isolated_10000_tasks": 110100,
"future_assume_isolated_lots_of_callbacks": 74050,
"future_erase_result": 4050,
"future_lots_of_callbacks": 74050,
Expand Down
2 changes: 1 addition & 1 deletion IntegrationTests/tests_04_performance/Thresholds/nightly-6.0.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"encode_1000_ws_frames_new_buffer_with_space_with_mask": 5050,
"execute_hop_10000_tasks": 0,
"flat_schedule_10000_tasks": 130100,
"flat_schedule_assume_isolated_10000_tasks": 150100,
"flat_schedule_assume_isolated_10000_tasks": 110100,
"future_assume_isolated_lots_of_callbacks": 74050,
"future_erase_result": 4050,
"future_lots_of_callbacks": 74050,
Expand Down
2 changes: 1 addition & 1 deletion IntegrationTests/tests_04_performance/Thresholds/nightly-main.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"encode_1000_ws_frames_new_buffer_with_space_with_mask": 5050,
"execute_hop_10000_tasks": 0,
"flat_schedule_10000_tasks": 130100,
"flat_schedule_assume_isolated_10000_tasks": 150100,
"flat_schedule_assume_isolated_10000_tasks": 110100,
"future_assume_isolated_lots_of_callbacks": 74050,
"future_erase_result": 4050,
"future_lots_of_callbacks": 74050,
Expand Down
107 changes: 107 additions & 0 deletions Sources/NIOCore/EventLoop.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,69 @@ public protocol EventLoop: EventLoopGroup {
///
/// - NOTE: Event loops only need to implemented this if they provide a custom scheduled callback implementation.
func cancelScheduledCallback(_ scheduledCallback: NIOScheduledCallback)

/// Submit a given task to be executed by the ``EventLoop``, from a context where the caller
/// statically knows that the context is isolated.
///
/// This is an optional performance hook. ``EventLoop`` implementers are not required to implement
/// this witness, but may choose to do so to enable better performance of the isolated EL views. If
/// they do so, ``EventLoop/Isolated/execute`` will perform better.
func _executeIsolatedUnsafeUnchecked(_ task: @escaping () -> Void)

/// Submit a given task to be executed by the ``EventLoop```, from a context where the caller
/// statically knows that the context is isolated.
///
/// Once the execution is complete the returned ``EventLoopFuture`` is notified.
///
/// This is an optional performance hook. ``EventLoop`` implementers are not required to implement
/// this witness, but may choose to do so to enable better performance of the isolated EL views. If
/// they do so, ``EventLoop/Isolated/submit`` will perform better.
///
/// - Parameters:
/// - task: The closure that will be submitted to the ``EventLoop`` for execution.
/// - Returns: ``EventLoopFuture`` that is notified once the task was executed.
func _submitIsolatedUnsafeUnchecked<T>(_ task: @escaping () throws -> T) -> EventLoopFuture<T>

/// Schedule a `task` that is executed by this ``EventLoop`` at the given time, from a context where the caller
/// statically knows that the context is isolated.
///
/// This is an optional performance hook. ``EventLoop`` implementers are not required to implement
/// this witness, but may choose to do so to enable better performance of the isolated EL views. If
/// they do so, ``EventLoop/Isolated/scheduleTask(deadline:_:)`` will perform better.
///
/// - Parameters:
/// - deadline: The instant in time before which the task will not execute.
/// - task: The synchronous task to run. As with everything that runs on the ``EventLoop```, it must not block.
/// - Returns: A ``Scheduled``` object which may be used to cancel the task if it has not yet run, or to wait
/// on the completion of the task.
///
/// - Note: You can only cancel a task before it has started executing.
@discardableResult
func _scheduleTaskIsolatedUnsafeUnchecked<T>(
deadline: NIODeadline,
_ task: @escaping () throws -> T
) -> Scheduled<T>

/// Schedule a `task` that is executed by this ``EventLoop`` after the given amount of time, from a context where the caller
/// statically knows that the context is isolated.
///
/// This is an optional performance hook. ``EventLoop`` implementers are not required to implement
/// this witness, but may choose to do so to enable better performance of the isolated EL views. If
/// they do so, ``EventLoop/Isolated/scheduleTask(in:_:)`` will perform better.
///
/// - Parameters:
/// - in: The amount of time before which the task will not execute.
/// - task: The synchronous task to run. As with everything that runs on the ``EventLoop``, it must not block.
/// - Returns: A ``Scheduled`` object which may be used to cancel the task if it has not yet run, or to wait
/// on the completion of the task.
///
/// - Note: You can only cancel a task before it has started executing.
/// - Note: The `in` value is clamped to a maximum when running on a Darwin-kernel.
@discardableResult
func _scheduleTaskIsolatedUnsafeUnchecked<T>(
in: TimeAmount,
_ task: @escaping () throws -> T
) -> Scheduled<T>
}

extension EventLoop {
Expand Down Expand Up @@ -422,6 +485,50 @@ extension EventLoop {
{
nil
}

/// Default implementation: wraps the task in an UnsafeTransfer.
public func _executeIsolatedUnsafeUnchecked(_ task: @escaping () -> Void) {
self.assertInEventLoop()
let unsafeTransfer = UnsafeTransfer(task)
self.execute {
unsafeTransfer.wrappedValue()
}
}

/// Default implementation: wraps the task in an UnsafeTransfer.
public func _submitIsolatedUnsafeUnchecked<T>(_ task: @escaping () throws -> T) -> EventLoopFuture<T> {
self.assertInEventLoop()
let unsafeTransfer = UnsafeTransfer(task)
return self.submit {
try unsafeTransfer.wrappedValue()
}
}

/// Default implementation: wraps the task in an UnsafeTransfer.
@discardableResult
public func _scheduleTaskIsolatedUnsafeUnchecked<T>(
deadline: NIODeadline,
_ task: @escaping () throws -> T
) -> Scheduled<T> {
self.assertInEventLoop()
let unsafeTransfer = UnsafeTransfer(task)
return self.scheduleTask(deadline: deadline) {
try unsafeTransfer.wrappedValue()
}
}

/// Default implementation: wraps the task in an UnsafeTransfer.
@discardableResult
public func _scheduleTaskIsolatedUnsafeUnchecked<T>(
in delay: TimeAmount,
_ task: @escaping () throws -> T
) -> Scheduled<T> {
self.assertInEventLoop()
let unsafeTransfer = UnsafeTransfer(task)
return self.scheduleTask(in: delay) {
try unsafeTransfer.wrappedValue()
}
}
}

extension EventLoop {
Expand Down
29 changes: 9 additions & 20 deletions Sources/NIOCore/EventLoopFuture+AssumeIsolated.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,7 @@ public struct NIOIsolatedEventLoop {
@available(*, noasync)
@inlinable
public func execute(_ task: @escaping () -> Void) {
let unsafeTransfer = UnsafeTransfer(task)
self._wrapped.execute {
unsafeTransfer.wrappedValue()
}
self._wrapped._executeIsolatedUnsafeUnchecked(task)
}

/// Submit a given task to be executed by the `EventLoop`. Once the execution is complete the returned `EventLoopFuture` is notified.
Expand All @@ -48,10 +45,7 @@ public struct NIOIsolatedEventLoop {
@available(*, noasync)
@inlinable
public func submit<T>(_ task: @escaping () throws -> T) -> EventLoopFuture<T> {
let unsafeTransfer = UnsafeTransfer(task)
return self._wrapped.submit {
try unsafeTransfer.wrappedValue()
}
self._wrapped._submitIsolatedUnsafeUnchecked(task)
}

/// Schedule a `task` that is executed by this `EventLoop` at the given time.
Expand All @@ -70,10 +64,7 @@ public struct NIOIsolatedEventLoop {
deadline: NIODeadline,
_ task: @escaping () throws -> T
) -> Scheduled<T> {
let unsafeTransfer = UnsafeTransfer(task)
return self._wrapped.scheduleTask(deadline: deadline) {
try unsafeTransfer.wrappedValue()
}
self._wrapped._scheduleTaskIsolatedUnsafeUnchecked(deadline: deadline, task)
}

/// Schedule a `task` that is executed by this `EventLoop` after the given amount of time.
Expand All @@ -93,10 +84,7 @@ public struct NIOIsolatedEventLoop {
in delay: TimeAmount,
_ task: @escaping () throws -> T
) -> Scheduled<T> {
let unsafeTransfer = UnsafeTransfer(task)
return self._wrapped.scheduleTask(in: delay) {
try unsafeTransfer.wrappedValue()
}
self._wrapped._scheduleTaskIsolatedUnsafeUnchecked(in: delay, task)
}

/// Schedule a `task` that is executed by this `EventLoop` at the given time.
Expand All @@ -122,10 +110,11 @@ public struct NIOIsolatedEventLoop {
line: UInt = #line,
_ task: @escaping () throws -> EventLoopFuture<T>
) -> Scheduled<T> {
let unsafeTransfer = UnsafeTransfer(task)
return self._wrapped.flatScheduleTask(deadline: deadline, file: file, line: line) {
try unsafeTransfer.wrappedValue()
}
let promise: EventLoopPromise<T> = self._wrapped.makePromise(file: file, line: line)
let scheduled = self.scheduleTask(deadline: deadline, task)

scheduled.futureResult.flatMap { $0 }.cascade(to: promise)
Copy link
Contributor

@glbrntt glbrntt Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we save an alloc here if we use scheduled.futureResult.whenComplete { ... }?

Copy link
Contributor Author

@Lukasa Lukasa Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so: the flatMap unwraps an extra level of futureness.

Copy link
Contributor

@glbrntt glbrntt Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean like this (I appreciate it's more code than the flatMap + cascade):

scheduled.futureResult.whenComplete { result in
    switch result {
    case .success(let futureResult):
        promise.completeWith(futureResult)
    case .failure(let error):
        promise.fail(error)
    }
}

Copy link
Contributor Author

@Lukasa Lukasa Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're probably right. However, all the flatScheduleTask implementations in swift-nio follow this spelling. I've filed #3071 to track making the improvement generically, and so for now would rather keep this in line with the others.

Copy link
Contributor

@glbrntt glbrntt Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filing that, makes sense to track them together.

return .init(promise: promise, cancellationTask: { scheduled.cancel() })
}

/// Returns the wrapped event loop.
Expand Down
Loading