Basic: use std::atomic instead of home-grown synch #3006
Conversation
|
CC @gottesmm |
|
@swift-ci Please test |
|
Ping |
| } | ||
|
|
||
| void Release() const { | ||
| int refCount = static_cast<int>(llvm::sys::AtomicDecrement(&ref_cnt)); | ||
| int refCount = | ||
| static_cast<int>(ref_cnt.fetch_sub(1, std::memory_order_relaxed)); |
There was a problem hiding this comment.
Doesn't this require an acquire so that the destructor is synchronized with the last modification?
There was a problem hiding this comment.
Yes, that seems safer. However, this is maintaining the status quo (the current behaviour of AtomicDecrement is a memory_order_relaxed). I can do a subsequent change to change it to memory_order_acquire, or we can make it part of this change. I think doing a follow up is better as this change doesnt change semantics that way.
There was a problem hiding this comment.
Are you sure?.. I'm confused -- the GCC docs say "In most cases, these builtins are considered a full barrier." for __sync_add_and_fetch.
There was a problem hiding this comment.
Hmm, yeah, reading over the docs, it does say that. Ill update the patch since that seems like it should be acquire in light of the docs.
There was a problem hiding this comment.
Seems that all of them need to be acquire as both the __sync_add_and_fetch and __sync_sub_and_fetch which these were implemented with do full barriers.
Regardless, "retain" should definitely be a "release" and should only be an "acquire" when the count is zero. |
|
@atrick Im confused like @gribozavr now. Am I misunderstanding something, the docs indicate that both the |
|
Yes, it looks like the previous implementation was generating a full barrier. This change weakens the barrier and is incorrect:
...because it is not a release barrier. (you can see on arm64 it's generating stxr, not stlxr). You can conservatively make it acq_rel. Full barrier on both acquire and release is very innefficient. It's fine for your patch because then it's NFC. Also, I don't know anything about the clients of this class and what additional assumptions they might make. However, for strong Swift refcounts, we normally want:
It's actually pretty simple. The complexity arises with weak references. |
SVN r271540 removed the LLVM atomics. Use std::atomic instead. NFC.
|
Given what you have stated, yes, this is a bit overkill, but should be the most equivalent change. Updated accordingly. |
|
Looks equivalent (to the old code) to me. @swift-ci Please test and merge |
|
FYI |
What's in this pull request?
Resolved bug number: (SR-)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.
SVN r271540 removed the LLVM atomics. Use std::atomic instead. NFC.