From 575533ae21d3f00d8608d38d3eb63fa779724572 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Wed, 5 Feb 2025 12:30:33 -0500 Subject: [PATCH 1/5] Use `os_unfair_lock` on Darwin when available. This PR refactors the `Locked` type to be generic over the type of lock it uses, allowing us to adopt `os_unfair_lock` in most places, but also allowing us to use other lock types as needed (i.e. `pthread_mutex_t` in WaitFor.swift if libdispatch is unavailable.) Under `-O`, locking and unlocking is fully inlined, while initialization (which is relatively rare) is partially, but not completely, inlined. --- Sources/Testing/CMakeLists.txt | 1 + Sources/Testing/ExitTests/WaitFor.swift | 4 +- Sources/Testing/Support/Locked+Platform.swift | 96 ++++++++++++ Sources/Testing/Support/Locked.swift | 138 +++++++++--------- Sources/_TestingInternals/include/Includes.h | 4 + Tests/TestingTests/Support/LockTests.swift | 49 ++++++- .../Traits/ParallelizationTraitTests.swift | 2 +- 7 files changed, 212 insertions(+), 82 deletions(-) create mode 100644 Sources/Testing/Support/Locked+Platform.swift diff --git a/Sources/Testing/CMakeLists.txt b/Sources/Testing/CMakeLists.txt index 5971a57b4..cd9983c07 100644 --- a/Sources/Testing/CMakeLists.txt +++ b/Sources/Testing/CMakeLists.txt @@ -79,6 +79,7 @@ add_library(Testing Support/Graph.swift Support/JSON.swift Support/Locked.swift + Support/Locked+Platform.swift Support/SystemError.swift Support/Versions.swift Discovery.swift diff --git a/Sources/Testing/ExitTests/WaitFor.swift b/Sources/Testing/ExitTests/WaitFor.swift index fac3e1496..c6841c580 100644 --- a/Sources/Testing/ExitTests/WaitFor.swift +++ b/Sources/Testing/ExitTests/WaitFor.swift @@ -80,7 +80,7 @@ func wait(for pid: consuming pid_t) async throws -> ExitCondition { } #elseif SWT_TARGET_OS_APPLE || os(Linux) || os(FreeBSD) || os(OpenBSD) /// A mapping of awaited child PIDs to their corresponding Swift continuations. -private let _childProcessContinuations = Locked<[pid_t: CheckedContinuation]>() +private let _childProcessContinuations = LockedWith]>() /// A condition variable used to suspend the waiter thread created by /// `_createWaitThread()` when there are no child processes to await. @@ -137,7 +137,7 @@ private let _createWaitThread: Void = { // newly-scheduled waiter process. (If this condition is spuriously // woken, we'll just loop again, which is fine.) Note that we read errno // outside the lock in case acquiring the lock perturbs it. - _childProcessContinuations.withUnsafePlatformLock { lock, childProcessContinuations in + _childProcessContinuations.withUnsafeUnderlyingLock { lock, childProcessContinuations in if childProcessContinuations.isEmpty { _ = pthread_cond_wait(_waitThreadNoChildrenCondition, lock) } diff --git a/Sources/Testing/Support/Locked+Platform.swift b/Sources/Testing/Support/Locked+Platform.swift new file mode 100644 index 000000000..ec05addba --- /dev/null +++ b/Sources/Testing/Support/Locked+Platform.swift @@ -0,0 +1,96 @@ +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2023–2025 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for Swift project authors +// + +internal import _TestingInternals + +extension Never: Lockable { + static func initializeLock(at lock: UnsafeMutablePointer) {} + static func deinitializeLock(at lock: UnsafeMutablePointer) {} + static func unsafelyAcquireLock(at lock: UnsafeMutablePointer) {} + static func unsafelyRelinquishLock(at lock: UnsafeMutablePointer) {} +} + +#if SWT_TARGET_OS_APPLE && canImport(os) +extension os_unfair_lock_s: Lockable { + static func initializeLock(at lock: UnsafeMutablePointer) { + lock.initialize(to: .init()) + } + + static func deinitializeLock(at lock: UnsafeMutablePointer) { + // No deinitialization needed. + } + + static func unsafelyAcquireLock(at lock: UnsafeMutablePointer) { + os_unfair_lock_lock(lock) + } + + static func unsafelyRelinquishLock(at lock: UnsafeMutablePointer) { + os_unfair_lock_unlock(lock) + } +} +#endif + +#if os(FreeBSD) || os(OpenBSD) +typealias pthread_mutex_t = _TestingInternals.pthread_mutex_t? +#endif + +#if SWT_TARGET_OS_APPLE || os(Linux) || os(Android) || (os(WASI) && compiler(>=6.1) && _runtime(_multithreaded)) || os(FreeBSD) || os(OpenBSD) +extension pthread_mutex_t: Lockable { + static func initializeLock(at lock: UnsafeMutablePointer) { + _ = pthread_mutex_init(lock, nil) + } + + static func deinitializeLock(at lock: UnsafeMutablePointer) { + _ = pthread_mutex_destroy(lock) + } + + static func unsafelyAcquireLock(at lock: UnsafeMutablePointer) { + _ = pthread_mutex_lock(lock) + } + + static func unsafelyRelinquishLock(at lock: UnsafeMutablePointer) { + _ = pthread_mutex_unlock(lock) + } +} +#endif + +#if os(Windows) +extension SRWLOCK: Lockable { + static func initializeLock(at lock: UnsafeMutablePointer) { + InitializeSRWLock(lock) + } + + static func deinitializeLock(at lock: UnsafeMutablePointer) { + // No deinitialization needed. + } + + static func unsafelyAcquireLock(at lock: UnsafeMutablePointer) { + AcquireSRWLockExclusive(lock) + } + + static func unsafelyRelinquishLock(at lock: UnsafeMutablePointer) { + ReleaseSRWLockExclusive(lock) + } +} +#endif + +#if SWT_TARGET_OS_APPLE && canImport(os) +typealias DefaultLock = os_unfair_lock +#elseif SWT_TARGET_OS_APPLE || os(Linux) || os(Android) || (os(WASI) && compiler(>=6.1) && _runtime(_multithreaded)) || os(FreeBSD) || os(OpenBSD) +typealias DefaultLock = pthread_mutex_t +#elseif os(Windows) +typealias DefaultLock = SRWLOCK +#elseif os(WASI) +// No locks on WASI without multithreaded runtime. +typealias DefaultLock = Never +#else +#warning("Platform-specific implementation missing: locking unavailable") +typealias DefaultLock = Never +#endif diff --git a/Sources/Testing/Support/Locked.swift b/Sources/Testing/Support/Locked.swift index 1294da195..3c1b31d46 100644 --- a/Sources/Testing/Support/Locked.swift +++ b/Sources/Testing/Support/Locked.swift @@ -1,7 +1,7 @@ // // This source file is part of the Swift.org open source project // -// Copyright (c) 2023 Apple Inc. and the Swift project authors +// Copyright (c) 2023–2025 Apple Inc. and the Swift project authors // Licensed under Apache License v2.0 with Runtime Library Exception // // See https://swift.org/LICENSE.txt for license information @@ -10,6 +10,37 @@ internal import _TestingInternals +/// A protocol defining a type, generally platform-specific, that satisfies the +/// requirements of a lock or mutex. +protocol Lockable { + /// Initialize the lock at the given address. + /// + /// - Parameters: + /// - lock: A pointer to uninitialized memory that should be initialized as + /// an instance of this type. + static func initializeLock(at lock: UnsafeMutablePointer) + + /// Deinitialize the lock at the given address. + /// + /// - Parameters: + /// - lock: A pointer to initialized memory that should be deinitialized. + static func deinitializeLock(at lock: UnsafeMutablePointer) + + /// Acquire the lock at the given address. + /// + /// - Parameters: + /// - lock: The address of the lock to acquire. + static func unsafelyAcquireLock(at lock: UnsafeMutablePointer) + + /// Relinquish the lock at the given address. + /// + /// - Parameters: + /// - lock: The address of the lock to relinquish. + static func unsafelyRelinquishLock(at lock: UnsafeMutablePointer) +} + +// MARK: - + /// A type that wraps a value requiring access from a synchronous caller during /// concurrent execution. /// @@ -21,67 +52,26 @@ internal import _TestingInternals /// concurrency tools. /// /// This type is not part of the public interface of the testing library. -/// -/// - Bug: The state protected by this type should instead be protected using -/// actor isolation, but actor-isolated functions cannot be called from -/// synchronous functions. ([83888717](rdar://83888717)) -struct Locked: RawRepresentable, Sendable where T: Sendable { - /// The platform-specific type to use for locking. - /// - /// It would be preferable to implement this lock in Swift, however there is - /// no standard lock or mutex type available across all platforms that is - /// visible in Swift. C11 has a standard `mtx_t` type, but it is not widely - /// supported and so cannot be relied upon. - /// - /// To keep the implementation of this type as simple as possible, - /// `pthread_mutex_t` is used on Apple platforms instead of `os_unfair_lock` - /// or `OSAllocatedUnfairLock`. -#if SWT_TARGET_OS_APPLE || os(Linux) || os(Android) || (os(WASI) && compiler(>=6.1) && _runtime(_multithreaded)) - typealias PlatformLock = pthread_mutex_t -#elseif os(FreeBSD) || os(OpenBSD) - typealias PlatformLock = pthread_mutex_t? -#elseif os(Windows) - typealias PlatformLock = SRWLOCK -#elseif os(WASI) - // No locks on WASI without multithreaded runtime. - typealias PlatformLock = Never -#else -#warning("Platform-specific implementation missing: locking unavailable") - typealias PlatformLock = Never -#endif +struct LockedWith: RawRepresentable where L: Lockable { + /// The type of the underlying lock that guards instances of this type. + typealias UnderlyingLock = L /// A type providing heap-allocated storage for an instance of ``Locked``. - private final class _Storage: ManagedBuffer { + private final class _Storage: ManagedBuffer { deinit { withUnsafeMutablePointerToElements { lock in -#if SWT_TARGET_OS_APPLE || os(Linux) || os(FreeBSD) || os(OpenBSD) || os(Android) || (os(WASI) && compiler(>=6.1) && _runtime(_multithreaded)) - _ = pthread_mutex_destroy(lock) -#elseif os(Windows) - // No deinitialization needed. -#elseif os(WASI) - // No locks on WASI without multithreaded runtime. -#else -#warning("Platform-specific implementation missing: locking unavailable") -#endif + L.deinitializeLock(at: lock) } } } /// Storage for the underlying lock and wrapped value. - private nonisolated(unsafe) var _storage: ManagedBuffer + private nonisolated(unsafe) var _storage: ManagedBuffer init(rawValue: T) { _storage = _Storage.create(minimumCapacity: 1, makingHeaderWith: { _ in rawValue }) _storage.withUnsafeMutablePointerToElements { lock in -#if SWT_TARGET_OS_APPLE || os(Linux) || os(FreeBSD) || os(OpenBSD) || os(Android) || (os(WASI) && compiler(>=6.1) && _runtime(_multithreaded)) - _ = pthread_mutex_init(lock, nil) -#elseif os(Windows) - InitializeSRWLock(lock) -#elseif os(WASI) - // No locks on WASI without multithreaded runtime. -#else -#warning("Platform-specific implementation missing: locking unavailable") -#endif + L.initializeLock(at: lock) } } @@ -103,28 +93,16 @@ struct Locked: RawRepresentable, Sendable where T: Sendable { /// concurrency tools. nonmutating func withLock(_ body: (inout T) throws -> R) rethrows -> R { try _storage.withUnsafeMutablePointers { rawValue, lock in -#if SWT_TARGET_OS_APPLE || os(Linux) || os(FreeBSD) || os(OpenBSD) || os(Android) || (os(WASI) && compiler(>=6.1) && _runtime(_multithreaded)) - _ = pthread_mutex_lock(lock) - defer { - _ = pthread_mutex_unlock(lock) - } -#elseif os(Windows) - AcquireSRWLockExclusive(lock) + L.unsafelyAcquireLock(at: lock) defer { - ReleaseSRWLockExclusive(lock) + L.unsafelyRelinquishLock(at: lock) } -#elseif os(WASI) - // No locks on WASI without multithreaded runtime. -#else -#warning("Platform-specific implementation missing: locking unavailable") -#endif - return try body(&rawValue.pointee) } } /// Acquire the lock and invoke a function while it is held, yielding both the - /// protected value and a reference to the lock itself. + /// protected value and a reference to the underlying lock guarding it. /// /// - Parameters: /// - body: A closure to invoke while the lock is held. @@ -134,16 +112,16 @@ struct Locked: RawRepresentable, Sendable where T: Sendable { /// - Throws: Whatever is thrown by `body`. /// /// This function is equivalent to ``withLock(_:)`` except that the closure - /// passed to it also takes a reference to the underlying platform lock. This - /// function can be used when platform-specific functionality such as a - /// `pthread_cond_t` is needed. Because the caller has direct access to the - /// lock and is able to unlock and re-lock it, it is unsafe to modify the - /// protected value. + /// passed to it also takes a reference to the underlying lock guarding this + /// instance's wrapped value. This function can be used when platform-specific + /// functionality such as a `pthread_cond_t` is needed. Because the caller has + /// direct access to the lock and is able to unlock and re-lock it, it is + /// unsafe to modify the protected value. /// /// - Warning: Callers that unlock the lock _must_ lock it again before the /// closure returns. If the lock is not acquired when `body` returns, the /// effect is undefined. - nonmutating func withUnsafePlatformLock(_ body: (UnsafeMutablePointer, T) throws -> R) rethrows -> R { + nonmutating func withUnsafeUnderlyingLock(_ body: (UnsafeMutablePointer, T) throws -> R) rethrows -> R { try withLock { value in try _storage.withUnsafeMutablePointerToElements { lock in try body(lock, value) @@ -152,7 +130,16 @@ struct Locked: RawRepresentable, Sendable where T: Sendable { } } -extension Locked where T: AdditiveArithmetic { +extension LockedWith: Sendable where T: Sendable {} + +/// A type that wraps a value requiring access from a synchronous caller during +/// concurrent execution and which uses the default platform-specific lock type +/// for the current platform. +typealias Locked = LockedWith + +// MARK: - Additions + +extension LockedWith where T: AdditiveArithmetic { /// Add something to the current wrapped value of this instance. /// /// - Parameters: @@ -168,7 +155,7 @@ extension Locked where T: AdditiveArithmetic { } } -extension Locked where T: Numeric { +extension LockedWith where T: Numeric { /// Increment the current wrapped value of this instance. /// /// - Returns: The sum of ``rawValue`` and `1`. @@ -188,7 +175,7 @@ extension Locked where T: Numeric { } } -extension Locked { +extension LockedWith { /// Initialize an instance of this type with a raw value of `nil`. init() where T == V? { self.init(rawValue: nil) @@ -198,4 +185,9 @@ extension Locked { init() where T == Dictionary { self.init(rawValue: [:]) } + + /// Initialize an instance of this type with a raw value of `nil`. + init() where T == [V] { + self.init(rawValue: []) + } } diff --git a/Sources/_TestingInternals/include/Includes.h b/Sources/_TestingInternals/include/Includes.h index 9fde898f2..63238a1a9 100644 --- a/Sources/_TestingInternals/include/Includes.h +++ b/Sources/_TestingInternals/include/Includes.h @@ -131,6 +131,10 @@ #if !SWT_NO_DYNAMIC_LINKING #include #endif + +#if __has_include() +#include +#endif #endif #if defined(__FreeBSD__) diff --git a/Tests/TestingTests/Support/LockTests.swift b/Tests/TestingTests/Support/LockTests.swift index f41e66349..33904affd 100644 --- a/Tests/TestingTests/Support/LockTests.swift +++ b/Tests/TestingTests/Support/LockTests.swift @@ -9,17 +9,54 @@ // @testable import Testing +private import _TestingInternals @Suite("Locked Tests") struct LockTests { - @Test("Mutating a value within withLock(_:)") + func testLock(_ lock: LockedWith) { + #expect(lock.rawValue == 0) + lock.withLock { value in + value = 1 + } + #expect(lock.rawValue == 1) + } + + @Test("Platform-default lock") func locking() { - let value = Locked(rawValue: 0) + testLock(Locked(rawValue: 0)) + } - #expect(value.rawValue == 0) - value.withLock { value in - value = 1 +#if SWT_TARGET_OS_APPLE && canImport(os) + @Test("pthread_mutex_t (Darwin alternate)") + func lockingWith_pthread_mutex_t() { + testLock(LockedWith(rawValue: 0)) + } +#endif + + @Test("No lock") + func noLock() async { + let lock = LockedWith(rawValue: 0) + await withTaskGroup(of: Void.self) { taskGroup in + for _ in 0 ..< 100_000 { + taskGroup.addTask { + lock.increment() + } + } + } + #expect(lock.rawValue != 100_000) + } + + @Test("Get the underlying lock") + func underlyingLock() { + let lock = Locked(rawValue: 0) + testLock(lock) + lock.withUnsafeUnderlyingLock { underlyingLock, _ in + DefaultLock.unsafelyRelinquishLock(at: underlyingLock) + lock.withLock { value in + value += 1000 + } + DefaultLock.unsafelyAcquireLock(at: underlyingLock) } - #expect(value.rawValue == 1) + #expect(lock.rawValue == 1001) } } diff --git a/Tests/TestingTests/Traits/ParallelizationTraitTests.swift b/Tests/TestingTests/Traits/ParallelizationTraitTests.swift index e43ca50b7..776e5c320 100644 --- a/Tests/TestingTests/Traits/ParallelizationTraitTests.swift +++ b/Tests/TestingTests/Traits/ParallelizationTraitTests.swift @@ -17,7 +17,7 @@ struct ParallelizationTraitTests { var configuration = Configuration() configuration.isParallelizationEnabled = true - let indicesRecorded = Locked<[Int]>(rawValue: []) + let indicesRecorded = Locked<[Int]>() configuration.eventHandler = { event, _ in if case let .issueRecorded(issue) = event.kind, let comment = issue.comments.first, From cc29dc1cdb8855835b84decef518a742dd1b6564 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Wed, 5 Feb 2025 12:54:14 -0500 Subject: [PATCH 2/5] Use SWT_NO_OS_UNFAIR_LOCK instead of canImport(os) --- Sources/Testing/Support/Locked+Platform.swift | 4 ++-- Tests/TestingTests/Support/LockTests.swift | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/Testing/Support/Locked+Platform.swift b/Sources/Testing/Support/Locked+Platform.swift index ec05addba..951e62da8 100644 --- a/Sources/Testing/Support/Locked+Platform.swift +++ b/Sources/Testing/Support/Locked+Platform.swift @@ -17,7 +17,7 @@ extension Never: Lockable { static func unsafelyRelinquishLock(at lock: UnsafeMutablePointer) {} } -#if SWT_TARGET_OS_APPLE && canImport(os) +#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK extension os_unfair_lock_s: Lockable { static func initializeLock(at lock: UnsafeMutablePointer) { lock.initialize(to: .init()) @@ -81,7 +81,7 @@ extension SRWLOCK: Lockable { } #endif -#if SWT_TARGET_OS_APPLE && canImport(os) +#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK typealias DefaultLock = os_unfair_lock #elseif SWT_TARGET_OS_APPLE || os(Linux) || os(Android) || (os(WASI) && compiler(>=6.1) && _runtime(_multithreaded)) || os(FreeBSD) || os(OpenBSD) typealias DefaultLock = pthread_mutex_t diff --git a/Tests/TestingTests/Support/LockTests.swift b/Tests/TestingTests/Support/LockTests.swift index 33904affd..0113745e9 100644 --- a/Tests/TestingTests/Support/LockTests.swift +++ b/Tests/TestingTests/Support/LockTests.swift @@ -26,7 +26,7 @@ struct LockTests { testLock(Locked(rawValue: 0)) } -#if SWT_TARGET_OS_APPLE && canImport(os) +#if SWT_TARGET_OS_APPLE && !SWT_NO_OS_UNFAIR_LOCK @Test("pthread_mutex_t (Darwin alternate)") func lockingWith_pthread_mutex_t() { testLock(LockedWith(rawValue: 0)) From 2ebb5c45ad08ec645a64e6854f88f67188ba00e7 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Wed, 5 Feb 2025 12:55:16 -0500 Subject: [PATCH 3/5] Use SWT_NO_OS_UNFAIR_LOCK in C++ too --- Sources/_TestingInternals/include/Includes.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/_TestingInternals/include/Includes.h b/Sources/_TestingInternals/include/Includes.h index 63238a1a9..5ba496ee9 100644 --- a/Sources/_TestingInternals/include/Includes.h +++ b/Sources/_TestingInternals/include/Includes.h @@ -132,7 +132,7 @@ #include #endif -#if __has_include() +#if !SWT_NO_OS_UNFAIR_LOCK #include #endif #endif From 24a632f086a872d6a05cfca02add23273d659b4b Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Wed, 5 Feb 2025 16:14:35 -0500 Subject: [PATCH 4/5] Update Sources/Testing/Support/Locked.swift Co-authored-by: Stuart Montgomery --- Sources/Testing/Support/Locked.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Testing/Support/Locked.swift b/Sources/Testing/Support/Locked.swift index 3c1b31d46..04e52fc84 100644 --- a/Sources/Testing/Support/Locked.swift +++ b/Sources/Testing/Support/Locked.swift @@ -186,7 +186,7 @@ extension LockedWith { self.init(rawValue: [:]) } - /// Initialize an instance of this type with a raw value of `nil`. + /// Initialize an instance of this type with a raw value of `[]`. init() where T == [V] { self.init(rawValue: []) } From d77019cf6e66789e5c8401bd49ea3141fd2b9e41 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Wed, 5 Feb 2025 17:16:13 -0500 Subject: [PATCH 5/5] Remove typealias for the underlying lock --- Sources/Testing/Support/Locked.swift | 3 --- 1 file changed, 3 deletions(-) diff --git a/Sources/Testing/Support/Locked.swift b/Sources/Testing/Support/Locked.swift index 04e52fc84..e8b17be7b 100644 --- a/Sources/Testing/Support/Locked.swift +++ b/Sources/Testing/Support/Locked.swift @@ -53,9 +53,6 @@ protocol Lockable { /// /// This type is not part of the public interface of the testing library. struct LockedWith: RawRepresentable where L: Lockable { - /// The type of the underlying lock that guards instances of this type. - typealias UnderlyingLock = L - /// A type providing heap-allocated storage for an instance of ``Locked``. private final class _Storage: ManagedBuffer { deinit {