(chore): Use Native swift Atomics #1969
Open
+111
−273
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
ahoppen
reviewed
Feb 6, 2025
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.
ahoppen
reviewed
Feb 6, 2025
| @@ -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.
Suggested change
| 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
ahoppen
approved these changes
Feb 6, 2025
|
@swift-ci Please test |
Replaces the usage of CAtomics with the native Atomics from the swift library and bumps the development platform to macOS 15
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
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 |
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.
The following PR addresses the following:
Closes #1949