{"action":"created","issue":{"url":"https://api.github.com/repos/ReactiveCocoa/ReactiveSwift/issues/211","repository_url":"https://api.github.com/repos/ReactiveCocoa/ReactiveSwift","labels_url":"https://api.github.com/repos/ReactiveCocoa/ReactiveSwift/issues/211/labels{/name}","comments_url":"https://api.github.com/repos/ReactiveCocoa/ReactiveSwift/issues/211/comments","events_url":"https://api.github.com/repos/ReactiveCocoa/ReactiveSwift/issues/211/events","html_url":"https://github.com/ReactiveCocoa/ReactiveSwift/pull/211","id":198611988,"number":211,"title":"Let's make `Atomic` more simple, efficient and reusable","user":{"login":"dmcrodrigues","id":1098602,"avatar_url":"https://avatars.githubusercontent.com/u/1098602?v=3","gravatar_id":"","url":"https://api.github.com/users/dmcrodrigues","html_url":"https://github.com/dmcrodrigues","followers_url":"https://api.github.com/users/dmcrodrigues/followers","following_url":"https://api.github.com/users/dmcrodrigues/following{/other_user}","gists_url":"https://api.github.com/users/dmcrodrigues/gists{/gist_id}","starred_url":"https://api.github.com/users/dmcrodrigues/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dmcrodrigues/subscriptions","organizations_url":"https://api.github.com/users/dmcrodrigues/orgs","repos_url":"https://api.github.com/users/dmcrodrigues/repos","events_url":"https://api.github.com/users/dmcrodrigues/events{/privacy}","received_events_url":"https://api.github.com/users/dmcrodrigues/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2017-01-04T01:37:32Z","updated_at":"2017-01-04T01:56:23Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ReactiveCocoa/ReactiveSwift/pulls/211","html_url":"https://github.com/ReactiveCocoa/ReactiveSwift/pull/211","diff_url":"https://github.com/ReactiveCocoa/ReactiveSwift/pull/211.diff","patch_url":"https://github.com/ReactiveCocoa/ReactiveSwift/pull/211.patch"},"body":"`Atomic` is an important part of ReactiveSwift but it's also something additive for any project using `ReactiveSwift` since it's publicly available for instantiation and can be used outside RAC codebases. However, it's only provided in a non-recursive version even when internally there's also a recursive implementation.\r\n\r\nThe idea behind this PR is provide both a recursive and non-recursive versions of `Atomic` and simplify the current implementation by reducing types, promoting consistency and improve performance.\r\n\r\nThis is achieved by exposing a strategy about the locking wanted plus a new initializer. I think `didSet` can be useful in certain use cases, internally it's useful for example, and it could be something to publicly expose to provide an extra flexibility or we could left this out.\r\n\r\n```swift\r\npublic enum AtomicLocking {\r\n    case recursive\r\n    case nonRecursive\r\n}\r\n\r\npublic class Atomic<Value> {\r\n\r\n    public convenience init(_ value: Value) {}\r\n\r\n    public init(_ value: Value, locking: AtomicLocking = .nonRecursive, didSet action: ((Value) -> Void)? = nil)\r\n}\r\n```\r\n\r\nHaving a single `Atomic` with this kind of parameterization avoids having two types for each version: recursive and non-recursive, plus avoiding both a protocol and extension to share implementation between them when the only thing that changes is the lock used. This was originally discussed and introduced in https://github.com/ReactiveCocoa/ReactiveCocoa/pull/3079.\r\n\r\nThis also promotes consistency since `Atomic` is currently using a POSIX's mutex but `RecursiveAtomic` is using a `NSRecursiveLock` instead of a POSIX's mutex configured as recursive. And there's also the question of performance.\r\n\r\n## Performance Tests\r\n\r\n#### Test\r\n- Xcode 8.2.1\r\n- Swift 3.0.2\r\n- Compiled with `-O -whole-module-optimization`\r\n- 10 million iterations\r\n\r\n[Source Code](https://gist.github.com/dmcrodrigues/e879828a665dd3a723789a7516bf6933)\r\n\r\n#### Results\r\n\r\n##### 1. No recursive lock\r\n\r\n- `NSRecursiveLock`: 0.662 sec. (1.521% STDEV)\r\n- `pthread_mutex_t`: 0.506 sec. (2.167% STDEV)\r\n\r\n##### 2. Lock with recursion = 2\r\n\r\n- `NSRecursiveLock`:  1.209 sec. (1.284% STDEV) \r\n- `pthread_mutex_t`: 0.733 sec. (4.105% STDEV)\r\n\r\n##### 3. Lock with recursion = 4\r\n\r\n- `NSRecursiveLock`:  1.885 sec. (1.401% STDEV)\r\n- `pthread_mutex_t`: 1.001 sec. (1.572% STDEV)\r\n\r\n##### 4. Lock with recursion = 8\r\n\r\n- `NSRecursiveLock`:  3.233 sec. (1.800% STDEV)\r\n- `pthread_mutex_t`: 1.437 sec. (1.204% STDEV)\r\n\r\n---\r\n\r\nLike was expected, a POSIX's mutex configured as recursive shows it as being much more efficient than `NSRecursiveLock` especially when there's some recursion involved. \r\n\r\nI don't know the reasoning behind the adoption of `NSRecursiveLock` for `RecursiveAtomic` but considering results, adopting a POSIX's mutex seems a good option in terms of consistency and performance, unless there's some detail or limitation that I may not be aware.\r\n\r\nIn case of positive feedback about this change there's some documentation missing that should be added."},"comment":{"url":"https://api.github.com/repos/ReactiveCocoa/ReactiveSwift/issues/comments/270276894","html_url":"https://github.com/ReactiveCocoa/ReactiveSwift/pull/211#issuecomment-270276894","issue_url":"https://api.github.com/repos/ReactiveCocoa/ReactiveSwift/issues/211","id":270276894,"user":{"login":"andersio","id":11806295,"avatar_url":"https://avatars.githubusercontent.com/u/11806295?v=3","gravatar_id":"","url":"https://api.github.com/users/andersio","html_url":"https://github.com/andersio","followers_url":"https://api.github.com/users/andersio/followers","following_url":"https://api.github.com/users/andersio/following{/other_user}","gists_url":"https://api.github.com/users/andersio/gists{/gist_id}","starred_url":"https://api.github.com/users/andersio/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andersio/subscriptions","organizations_url":"https://api.github.com/users/andersio/orgs","repos_url":"https://api.github.com/users/andersio/repos","events_url":"https://api.github.com/users/andersio/events{/privacy}","received_events_url":"https://api.github.com/users/andersio/received_events","type":"User","site_admin":false},"created_at":"2017-01-04T01:56:23Z","updated_at":"2017-01-04T01:56:23Z","body":"Reentrant lock has a fundamentally different semantic, and I am not fond of having both under the same type. Places that actually need reentrant locks are limited either, so this is why it isn't part of the public API.\r\n\r\n+1 on pthread_mutex in release build. I think we should use `NSLock` in PosixThreadMutex for debug builds, heh, so that deadlocks would not be a silent halt."}}
{"actor":{"display_login":"andersio"}}