-
Notifications
You must be signed in to change notification settings - Fork 18
Adopt NonisolatedNonsendingByDefault
#272
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Fabian Fett <[email protected]>
Signed-off-by: Fabian Fett <[email protected]>
c742432 to
5b23941
Compare
Signed-off-by: Fabian Fett <[email protected]>
Signed-off-by: Fabian Fett <[email protected]>
Signed-off-by: Fabian Fett <[email protected]>
|
❌ Benchmark comparison failed with error ❌ Summary
New baseline 'pull_request' is BETTER than the 'main' baseline thresholds. Full Benchmark ComparisonComparing results between 'main' and 'pull_request'ValkeyBenchmarksClient: GET benchmark metricsMalloc (total): results within specified thresholds, fold down for details.
Client: GET benchmark | parallel 20 | 20 concurrent connections metricsMalloc (total): results within specified thresholds, fold down for details.
Connection: GET benchmark metricsMalloc (total): results within specified thresholds, fold down for details.
Connection: GET benchmark – NoOpTracer metricsMalloc (total): results within specified thresholds, fold down for details.
Connection: Pipeline array benchmark metricsMalloc (total): results within specified thresholds, fold down for details.
Connection: Pipeline benchmark metricsMalloc (total): results within specified thresholds, fold down for details.
HashSlot – {user}.whatever metricsMalloc (total): results within specified thresholds, fold down for details.
ValkeyCommandEncoder – Command with 7 words metricsMalloc (total): results within specified thresholds, fold down for details.
ValkeyCommandEncoder – Simple GET metricsMalloc (total): results within specified thresholds, fold down for details.
ValkeyCommandEncoder – Simple MGET 15 keys metricsMalloc (total): results within specified thresholds, fold down for details.
|
Signed-off-by: Fabian Fett <[email protected]>
Signed-off-by: Fabian Fett <[email protected]>
adam-fowler
left a comment
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.
A couple of comments, mainly based on my ignorance of some of the concurrency keywords
| var base: BaseAsyncSequence.AsyncIterator | ||
|
|
||
| #if compiler(>=6.2) | ||
| @concurrent |
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.
Could this be
public mutating func next() async throws -> Element? {
try await self.base.next(isolation: #isolation)
}Instead of adding @concurrent
| /// ``ValkeyConnection/psubscribe(to:process:)`` | ||
| @inlinable | ||
| public func _subscribe<Value>( | ||
| public nonisolated(nonsending) func _subscribe<Value>( |
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 is this nonisolated(nonsending) when the functions that call it are nonisolated?
| /// ``ValkeyConnection/subscribe(to:process:)`` or | ||
| /// ``ValkeyConnection/psubscribe(to:process:)`` | ||
| @inlinable | ||
| public func _subscribe<Value>( |
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.
Instead of using a later swift-format which the majority of people aren't using yet can you add this instead
// swift-format-ignore
@inlinable
public nonisolated(nonsending) func _subscribe<Value>(
command: some ValkeySubscribeCommand,
process: nonisolated(nonsending) (ValkeySubscription) async throws -> Value
) async throws -> Value {| with: | ||
| api_breakage_check_container_image: swift:latest | ||
| docs_check_container_image: swift:latest | ||
| # 6.2 does not understand `nonisolated(nonsending)` correctly |
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.
Use // swift-format-ignore to get around swift-format not understanding nonisolated(nonsending)
Fixes: #261.