-
Notifications
You must be signed in to change notification settings - Fork 429
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
Atomic
: The Unexpected Journey.
#124
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we accept this PR, need to fix a couple issues.
return try action(&_value) | ||
let returnValue = try action(&_value) | ||
didSetObserver?(_value) | ||
lock.unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without defer
, if try action(&_value)
throws, the lock will never be unlocked.
swap(newValue) | ||
} | ||
let returnValue = try action(_value) | ||
lock.unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should bring back defer
here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is an oversight. Thanks for catching this.
The hole @sharplet caught is fixed using P.S. In |
ecaf9b3
to
81c3885
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding didSetObserver
, is it possible that the throwing closure can change the inout parameter before throwing, and have that change copied back to the caller? Or does Swift guarantee that in the throwing case the inout parameter won't be changed?
precondition(result == 0, "Failed to destroy mutex with error \(result).") | ||
|
||
UnsafeMutableRawPointer(mutex).deallocate(bytes: MemoryLayout<pthread_mutex_t>.size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think of using a raw pointer here it might make sense to use mutex.deinitialize(1)
and mutex.deallocate(1)
? I'm not super familiar with the new pointer APIs though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems UnsafeMutablePointer
is enough.
Instances must be aligned to MemoryLayout<Pointee>.alignment, i.e. (UnsafePointer<Int8>(self) - nil) % MemoryLayout<Pointee>.alignment == 0
let value = try action(&_value) | ||
lock.unlock() | ||
return value | ||
} catch let error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can omit let error
here, and in the other catch
blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
@discardableResult | ||
@inline(__always) | ||
public func swap(_ newValue: Value) -> Value { | ||
return modify { (value: inout Value) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the closure type annotation necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Hmm. It won't restore automatically. But making it restoring on throw doesn't make sense in performance for the common use cases either. Maybe it should be left as is. |
0a1862a
to
43cde25
Compare
precondition(result == 0, "Failed to destroy mutex with error \(result).") | ||
mutex.deallocate(capacity: 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://developer.apple.com/reference/swift/unsafemutablepointer/2295090-deallocate:
Precondition: The memory is not initialized.
This is what made me think that perhaps we should be calling mutex.deinitialize()
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the memory model, it has to be first initialised before being used too. Addressed in 974663c.
43cde25
to
f1128ce
Compare
f1128ce
to
54e5715
Compare
} | ||
} | ||
|
||
internal final class PosixThreadMutex: NSLocking { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like this is used anywhere? Let's remove it and then remove the underscore from the version we are using.
|
||
extension AtomicProtocol { | ||
/// Atomically get or set the value of the variable. | ||
public var value: Value { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still keep this and swap
as part of AtomicProtocol
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The performance is much worse due to the need to query the protocol witness table, even with the @inline(__always)
annotation. I have rerun my benchmarks and it is still the case.
I am actually thinking of removing AtomicProtocol
, hmm...
Embed PosixThreadMutex into Atomic.
0.179 sec (4%)Implement value and swap in Atomic.
0.149 sec (4%)
@@ -8,27 +8,37 @@ | |||
|
|||
import Foundation | |||
|
|||
final class PosixThreadMutex: NSLocking { | |||
private var mutex = pthread_mutex_t() | |||
internal struct PosixThreadMutex { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a struct
with an UnsafeMutablePointer
instead of a class
with a deinit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is usually used with an apparent owner. So defining it as a struct gives the flexibility to have it embedded in the containing type, should it be a stored property, and have one less pointer to chase.
Drop precondition in favour of if-fatalError.
0.188 sec (2%)Embed PosixThreadMutex into Atomic.
0.179 sec (4%)
Related: https://lists.swift.org/pipermail/swift-users/Week-of-Mon-20161205/004147.html
|
1fa9fd6
to
43554ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good overall. Just needs a couple tweaks.
} catch { | ||
lock.unlock() | ||
throw error | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about keeping the defer
s?
Removing them seems to have a very minor effect on the speed but makes mistakes more likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How likely are we to change this again though? It are is one of the pieces that perhaps would never be altered otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no way to know. 🙃
@@ -277,7 +378,8 @@ extension AtomicProtocol { | |||
/// | |||
/// - returns: The old value. | |||
@discardableResult | |||
public func swap(_ newValue: Value) -> Value { | |||
@inline(__always) | |||
func swap(_ newValue: Value) -> Value { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.Atomic.swap
is. RecursiveAtomic.swap
isn't.
final class PosixThreadMutex: NSLocking { | ||
private var mutex = pthread_mutex_t() | ||
internal struct PosixThreadMutex { | ||
private let mutex: UnsafeMutablePointer<pthread_mutex_t> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment that explains this?
Any time we choose the non-obvious route to get a speed boost, we should document it.
@@ -131,7 +131,7 @@ internal struct UnsafeAtomicState<State: RawRepresentable>: AtomicStateProtocol | |||
/// The wrapper is a struct so that it can be inlined when it is contained as | |||
/// part of a reference type, and dodges an extra level of calling indirection | |||
/// that would otherwise happen if the wrapper itself is an reference type. | |||
internal struct PosixThreadMutex { | |||
internal struct PthreadMutex { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it is generally called pthread anyway... So let's make it a bit shorter...
The benchmark
Source
It simulates the critical path of
Signal
, which is grabbing the signal state + querying theinterrupted
.Compiler Settings: Both the framework and the test are compiled with
-Owholemodule
. Checked builds.Test Environment: i5 4258U, macOS 10.12.1, Xcode 8.1, Swift 3.0.1
Final Speedup
4.641x 😦
The optimised
Atomic
now takes only 0.029 sec (5%) in the synthetic test of the lock-free disposal PR, significantly faster than it was before.Baseline Performance
master
0.673 sec (2%)
The Journey
Mark
AtomicProtocol.value
andAtomicProtocol.swap
as@inline(__always)
.0.434 sec (2%)
Mark
Atomic.modify
andAtomic.swap
as@inline(__always)
.0.296 sec (2%)
Mark
PosixThreadMutex.lock
andPosixThreadMutex.unlock
as@inline(__always)
.0.223 sec (2%)
Drop
defer
statements inAtomic.modify
andAtomic.swap
.0.212 sec (3%)
Drop
precondition
in favour ofif-fatalError
.0.188 sec (2%)
Embed
PosixThreadMutex
intoAtomic
.0.179 sec (4%)
Implement
value
andswap
inAtomic
.0.149 sec (4%)
Use the non-copying
modify
instead ofwithValue
invalue.get
.0.145 sec (2%)
Misc
Inline
modify
intoswap
manually.0.145 sec (2%)
Closure based
_PosixThreadMutex.lock
.0.179 sec (3%)
-Onone
0.479 sec (3%)