Convert ScheduledTask to a struct to reduce allocations for scheduling
#2010
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
semver/patch
Lukasa
requested changes
Dec 13, 2021
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-1
branch
from
075586f
to
8f0219b
Compare
|
@swift-nio-bot test perf please |
performance reportbuild id: 88 timestamp: Tue Dec 14 09:07:41 UTC 2021 results
comparison
significant differences found |
glbrntt
reviewed
Dec 14, 2021
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-1
branch
2 times, most recently
from
8b6d27f
to
5b75a75
Compare
FranzBusch
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Dec 14, 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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Dec 14, 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-1
branch
3 times, most recently
from
7e9b916
to
b77f283
Compare
glbrntt
reviewed
Dec 14, 2021
Sources/NIOEmbedded/Embedded.swift
Outdated
| @@ -63,6 +67,7 @@ public final class EmbeddedEventLoop: EventLoop { | |||
| /// The current "time" for this event loop. This is an amount in nanoseconds. | |||
| /* private but tests */ internal var _now: NIODeadline = .uptimeNanoseconds(0) | |||
|
|
|||
| private var scheduledTaskCounter = NIOAtomic.makeAtomic(value: UInt64(0)) | |||
There was a problem hiding this comment.
It's a shame that this now regresses a bunch of allocation counting tests that use EmbeddedEventLoop (and will no doubt regress allocation counting tests in other swift-nio-* packages). I wonder if we should make the counter for embedded loops global (or static) to reduce the noise of this one.
There was a problem hiding this comment.
Actually EmbeddedEventLoop isn't thread safe, can we just use a UInt64?
There was a problem hiding this comment.
We can, and probably should.
|
@swift-nio-bot test perf please |
performance reportbuild id: 89 timestamp: Tue Dec 14 13:09:22 UTC 2021 results
comparison
significant differences found |
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-1
branch
2 times, most recently
from
397dbdf
to
419ced9
Compare
FranzBusch
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Dec 14, 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
|
@swift-server-bot test this please |
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-1
branch
from
419ced9
to
15b1bdd
Compare
### Motivation: In my previous PR apple#2009, I added baseline performance and allocation tests around `scheduleTask` and `execute`. After analysing, the various allocations that happen when scheduling a task there were only a few that could be optimized away potentially. ### Modifications: This PR converts the `ScheduledTask` class to a struct which will reduce the number of allocations for scheduling tasks by 1. The only thing that needs to be worked around when converting to a struct is giving it an identity so that we can implement `Equatable` conformance properly. I explored two options. First, using an `ObjectIdentifier` passed to the init. Second, using an atomic counter per EventLoop. I went with the latter since the former requires an additional allocation in the case of calling `execute` ### Result: `scheduleTask` and `execute` require one less allocation
FranzBusch
force-pushed
the
feature/scheduling-task-allocations-part-1
branch
from
15b1bdd
to
584c6aa
Compare
Lukasa
approved these changes
Dec 14, 2021
FranzBusch
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Dec 14, 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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
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
added a commit
that referenced
this pull request
Dec 15, 2021
### Motivation: In my previous PR #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 #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
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Dec 27, 2021
### Motivation: When scheduling tasks with the same deadline the current order of execution is undefined. apple#1541 ### Modifications: In my previous PR apple#2010, I added an internal id to every `ScheduledTask` to give them an identity for cancellation purposes. In this PR, I am now using the same id to also ensure that the execution of tasks with the same deadline is the same as the order they were scheduled in. ### Result: `ScheduledTask`s are now executed in their scheduled order when they have the same deadline.
FranzBusch
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Dec 27, 2021
### Motivation: When scheduling tasks with the same deadline the current order of execution is undefined. Fixes apple#1541 ### Modifications: In my previous PR apple#2010, I added an internal id to every `ScheduledTask` to give them an identity for cancellation purposes. In this PR, I am now using the same id to also ensure that the execution of tasks with the same deadline is the same as the order they were scheduled in. ### Result: `ScheduledTask`s are now executed in their scheduled order when they have the same deadline.
FranzBusch
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Dec 27, 2021
### Motivation: When scheduling tasks with the same deadline the current order of execution is undefined. Fixes apple#1541 ### Modifications: In my previous PR apple#2010, I added an internal id to every `ScheduledTask` to give them an identity for cancellation purposes. In this PR, I am now using the same id to also ensure that the execution of tasks with the same deadline is the same as the order they were scheduled in. ### Result: `ScheduledTask`s are now executed in their scheduled order when they have the same deadline.
FranzBusch
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Dec 27, 2021
### Motivation: When scheduling tasks with the same deadline the current order of execution is undefined. Fixes apple#1541 ### Modifications: In my previous PR apple#2010, I added an internal id to every `ScheduledTask` to give them an identity for cancellation purposes. In this PR, I am now using the same id to also ensure that the execution of tasks with the same deadline is the same as the order they were scheduled in. ### Result: `ScheduledTask`s are now executed in their scheduled order when they have the same deadline.
FranzBusch
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Jan 4, 2022
### Motivation: When scheduling tasks with the same deadline the current order of execution is undefined. Fixes apple#1541 ### Modifications: In my previous PR apple#2010, I added an internal id to every `ScheduledTask` to give them an identity for cancellation purposes. In this PR, I am now using the same id to also ensure that the execution of tasks with the same deadline is the same as the order they were scheduled in. ### Result: `ScheduledTask`s are now executed in their scheduled order when they have the same deadline.
FranzBusch
added a commit
to FranzBusch/swift-nio
that referenced
this pull request
Jan 4, 2022
### Motivation: When scheduling tasks with the same deadline the current order of execution is undefined. Fixes apple#1541 ### Modifications: In my previous PR apple#2010, I added an internal id to every `ScheduledTask` to give them an identity for cancellation purposes. In this PR, I am now using the same id to also ensure that the execution of tasks with the same deadline is the same as the order they were scheduled in. ### Result: `ScheduledTask`s are now executed in their scheduled order when they have the same deadline.
FranzBusch
added a commit
that referenced
this pull request
Jan 4, 2022
### Motivation: When scheduling tasks with the same deadline the current order of execution is undefined. Fixes #1541 ### Modifications: In my previous PR #2010, I added an internal id to every `ScheduledTask` to give them an identity for cancellation purposes. In this PR, I am now using the same id to also ensure that the execution of tasks with the same deadline is the same as the order they were scheduled in. ### Result: `ScheduledTask`s are now executed in their scheduled order when they have the same deadline.
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 #2009, I added baseline performance and allocation tests around
scheduleTaskandexecute. After analysing, the various allocations that happen when scheduling a task there were only a few that could be optimized away potentially.Modifications:
This PR converts the
ScheduledTaskclass to a struct which will reduce the number of allocations for scheduling tasks by 1. The only thing that needs to be worked around when converting to a struct is giving it an identity so that we can implementEquatableconformance properly. I explored two options. First, using anObjectIdentifierpassed to the init. Second, using an atomic counter per EventLoop. I went with the latter since the former requires an additional allocation in the case of callingexecuteResult:
scheduleTaskandexecuterequire one less allocation