Call the cancellationTask of Scheduled directly
#2011
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FranzBusch
added
patch-version-bump-only
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-2
branch
from
9d369f7
to
7a185d4
Compare
|
@swift-nio-bot test perf please |
performance reportbuild id: 91 timestamp: Wed Dec 15 08:39:45 UTC 2021 results
comparison
significant differences found |
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-2
branch
from
7a185d4
to
7a9b590
Compare
glbrntt
reviewed
Dec 15, 2021
Sources/NIOCore/EventLoop.swift
Outdated
| @@ -21,18 +21,13 @@ import Dispatch | |||
| /// will be notified once the execution is complete. | |||
| public struct Scheduled<T> { | |||
| /* private but usableFromInline */ @usableFromInline let _promise: EventLoopPromise<T> | |||
|
|
|||
| @usableFromInline let cancellationTask: (() -> Void) | |||
There was a problem hiding this comment.
Presumably this would be private if it wasn't @usableFromInline. By convention in these cases we prefix the name with a _ so this should be _cancellationTask.
Sources/NIOEmbedded/Embedded.swift
Outdated
Comment on lines
192
to
193
| while self.scheduledTasks.peek() != nil { | ||
| self.scheduledTasks.pop()?.fail(EventLoopError.shutdown) |
There was a problem hiding this comment.
Why not while let task = self.scheduledTasks.pop() { ... }?
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-2
branch
from
7a9b590
to
7883758
Compare
Lukasa
reviewed
Dec 15, 2021
| while self.scheduledTasks.pop() != nil {} | ||
| while let task = self.scheduledTasks.pop() { | ||
| task.fail(EventLoopError.shutdown) | ||
| } |
There was a problem hiding this comment.
I suspect this is a behavioural change. Can you write a test that encodes this?
There was a problem hiding this comment.
@Lukasa So there is a test that is executing this testFinishWithRecursivelyScheduledTasks and without this change the test is actually crashing because the future is leaking. Which I am still not 100% sure why it did work before. Should I write an additional test that checks if the error is actually the EventLoopError.shutdown?
There was a problem hiding this comment.
I found a different test that was already testing the EmbeddedEventLoop specifically when draining the remaining tasks. I just added an assertion that makes sure the last task is failed with the proper error. Is that fine with you?
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-2
branch
from
7883758
to
6a935b8
Compare
Lukasa
approved these changes
Dec 15, 2021
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-2
branch
2 times, most recently
from
f04f2ab
to
fbd9eae
Compare
glbrntt
approved these changes
Dec 15, 2021
### Motivation: In my previous PR apple#2010, I was able to decrease the allocations for both `scheduleTask` and `execute` by 1 already. Gladly, there are no more allocations left to remove from `execute` now; however, `scheduleTask` still provides a couple of allocations that we can try to get rid of. ### Modifications: This PR removes two allocations inside `Scheduled` where we were using the passed in `EventLoopPromise` to call the `cancellationTask` once the `EventLoopFuture` of the promise fails. This requires two allocations inside `whenFailure` and inside `_whenComplete`. However, since we are passing the `cancellationTask` to `Scheduled` anyhow and `Scheduled` is also the one that is failing the promise from the `cancel()` method. We can just go ahead and store the `cancellationTask` inside `Scheduled` and call it from the `cancel()` method directly instead of going through the future. Importantly, here is that the `cancellationTask` is not allowed to retain the `ScheduledTask.task` otherwise we would change the semantics and retain the `ScheduledTask.task` longer than necessary. My previous PR apple#2010, already implemented the work to get rid of the retain from the `cancellationTask` closure. So we are good to go ahead and store the `cancellationTask` inside `Scheduled` now ### Result: `scheduleTask` requires two fewer allocations
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-2
branch
from
fbd9eae
to
7fcc0bb
Compare
glbrntt
added a commit
to glbrntt/swift-nio-transport-services
that referenced
this pull request
Jan 12, 2022
Motivation: When a task is scheduled using a `DispatchTimerSource` on a `NIOTSEventLoop` the source is retain via a callback on the promise associated with the scheduled task. This stops the source from deinit'd before the task is run. However, the callback being added is an implementation details of `Scheduled` and the retain cycle is incidental. If that detail chnaged (such as in apple/swift-nio#2011) then tasks may not run and the promise could be leaked. Modifications: - Explicitly add a retain cycle between the future and the timer source which is broken by the promise being completed and callbacks run. Result: The timer source is kept alive until the event fires.
Lukasa
pushed a commit
to apple/swift-nio-transport-services
that referenced
this pull request
Jan 12, 2022
Motivation: When a task is scheduled using a `DispatchTimerSource` on a `NIOTSEventLoop` the source is retain via a callback on the promise associated with the scheduled task. This stops the source from deinit'd before the task is run. However, the callback being added is an implementation details of `Scheduled` and the retain cycle is incidental. If that detail chnaged (such as in apple/swift-nio#2011) then tasks may not run and the promise could be leaked. Modifications: - Explicitly add a retain cycle between the future and the timer source which is broken by the promise being completed and callbacks run. Result: The timer source is kept alive until the event fires.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation:
In my previous PR #2010, I was able to decrease the allocations for both
scheduleTaskandexecuteby 1 already. Gladly, there are no more allocations left to remove fromexecutenow; however,scheduleTaskstill provides a couple of allocations that we can try to get rid of.Modifications:
This PR removes two allocations inside
Scheduledwhere we were using the passed inEventLoopPromiseto call thecancellationTaskonce theEventLoopFutureof the promise fails. This requires two allocations insidewhenFailureand inside_whenComplete. However, since we are passing thecancellationTasktoScheduledanyhow andScheduledis also the one that is failing the promise from thecancel()method. We can just go ahead and store thecancellationTaskinsideScheduledand call it from thecancel()method directly instead of going through the future.Importantly, here is that the
cancellationTaskis not allowed to retain theScheduledTask.taskotherwise we would change the semantics and retain theScheduledTask.tasklonger than necessary. My previous PR #2010, already implemented the work to get rid of the retain from thecancellationTaskclosure. So we are good to go ahead and store thecancellationTaskinsideSchedulednowResult:
scheduleTaskrequires two fewer allocations