Skip to content
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

Adopt Mutex where available #70

Merged
merged 4 commits into from
Nov 13, 2024
Merged

Adopt Mutex where available #70

merged 4 commits into from
Nov 13, 2024

Conversation

guoye-zhang
Copy link
Contributor

Mutex should eliminate the extra allocation. Subclassing is the only way I found to work. I tried enums (enum case cannot be unavailable) and class-bound protocols (isKnownUniquelyReferenced doesn't work). @FranzBusch

@guoye-zhang guoye-zhang requested a review from ekinnear October 28, 2024 06:47
private final class _StorageWithMutex: _Storage, @unchecked Sendable {
let mutex = Mutex<Void>(())

override func withLock<Result>(_ body: () throws -> Result) rethrows -> Result {
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually compile without warnings under strict concurrency? The closure that Mutex takes is sending whereas ours is not. This should produce a warning/error.

Copy link

Choose a reason for hiding this comment

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

Needing dynamic dispatch is also a little gnarly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried .enableExperimentalFeature("StrictConcurrency") and didn't get any errors, but added sending in 948fe6c to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think dynamic dispatch is still better than an extra allocation for the lock?

Copy link
Contributor Author

@guoye-zhang guoye-zhang Oct 29, 2024

Choose a reason for hiding this comment

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

Had to revert 948fe6c due to it breaking Swift 5.9/5.10

@guoye-zhang guoye-zhang added 🔨 semver/patch No public API change. area/performance Improvements to performance. labels Oct 28, 2024
@guoye-zhang guoye-zhang force-pushed the mutex branch 2 times, most recently from 948fe6c to c82e407 Compare October 29, 2024 00:39
@@ -99,7 +109,40 @@ public struct HTTPFields: Sendable, Hashable {
}
}

private var _storage = _Storage()
#if canImport(Synchronization)
@available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to fix this on Swift 5.9/5.10.

/__w/swift-http-types/swift-http-types/Sources/HTTPTypes/HTTPFields.swift:113:63: error: unrecognized platform name 'visionOS'
    @available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by switching to a compiler version check instead of a canImport check.

Copy link

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I think this is honestly probably fine, we just need to be aware that one day we'll want to remove this.

@guoye-zhang guoye-zhang merged commit 7e038be into apple:main Nov 13, 2024
34 checks passed
@guoye-zhang guoye-zhang deleted the mutex branch November 13, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance Improvements to performance. 🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants