(chore): Use Native swift Atomics #1969
Conversation
There was a problem hiding this comment.
Thanks for the update. I have two high-level comments:
- Should we just use
sequentiallyConsistentas the ordering? As far as I can tell, the currentreleasingandacquiringproduce the same semantics and I findsequentiallyConsistenteasier to reason about. Or am I missing something. - I think some of the
Atomic<Int32>andAtomic<UInt32>can now becomeAtomic<Int>if that simplified their usage. They were onlyInt32andUInt32because of C interop.
| @@ -216,7 +217,7 @@ package final class TestSourceKitLSPClient: MessageHandler, Sendable { | |||
| // It's really unfortunate that there are no async deinits. If we had async | |||
| // deinits, we could await the sending of a ShutdownRequest. | |||
| let sema = DispatchSemaphore(value: 0) | |||
| server.handle(ShutdownRequest(), id: .number(Int(nextRequestID.fetchAndIncrement()))) { result in | |||
| server.handle(ShutdownRequest(), id: .number(Int(nextRequestID.add(1, ordering: .sequentiallyConsistent).newValue))) { result in | |||
There was a problem hiding this comment.
| server.handle(ShutdownRequest(), id: .number(Int(nextRequestID.add(1, ordering: .sequentiallyConsistent).newValue))) { result in | |
| server.handle(ShutdownRequest(), id: .number(Int(nextRequestID.add(1, ordering: .sequentiallyConsistent).oldValue))) { result in |
There was a problem hiding this comment.
and I am assuming that when subtracting or adding in the following places its the new value that we need and not the old one.
let count = bytesCollected.add(bytes.count, ordering: .sequentiallyConsistent).newValue
var progress = Double(count) / Double(expectedLogSize)let count = pendingUnitCount.subtract(count, ordering: .sequentiallyConsistent).newValue
if count == 0 {There was a problem hiding this comment.
Yes, that’s correct. It’s just this case where we want to use the oldValue because nextRequestID is the next request ID that hasn’t been used yet. We use nextRequestID with oldValue in other places as well.
mustiikhalil
force-pushed
the
issue-1949-refactor-atomic
branch
2 times, most recently
from
d53797e to
7fb0adb
Compare
mustiikhalil
changed the title
|
@swift-ci Please test |
mustiikhalil
force-pushed
the
issue-1949-refactor-atomic
branch
from
7fb0adb to
df47d38
Compare
|
The Ci failed because of a newly merged commit that uses the AtomicUint32 |
|
Thanks for the update @mustiikhalil @swift-ci Please test |
|
@swift-ci Please test Windows |
|
@swift-ci Please test |
|
I'm assuming this will never pass on macOS since the Ci has is OS as 13.6 instead of 15.0 |
Replaces the usage of CAtomics with the native Atomics from the swift library and bumps the development platform to macOS 15 Removes all references to CAtomics from the library Removes atomic wrappers infavor of using them directly Replacing the usage of Int32, and UInt32 with Int since the previous was only used because of c bridging Uses .sequentiallyConsistent for all loading and storing methods
mustiikhalil
force-pushed
the
issue-1949-refactor-atomic
branch
from
22425e9 to
d788e44
Compare
|
Oh no, I completely forgot about that 😢. Thank you for the PR anyway. This will be great once we can update the deployment target. |
|
No worries! Always happy to help, I guess we can make it a draft until all of that is done |
The following PR addresses the following:
Closes #1949