Fixed OSSpinLock usage.

[jcbertin]

Apple engineers have pointed out that OSSpinLocks are vulnerable to live locking on iOS in cases of priority inversion: • http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/ • https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html

You may also read this. It explains the problem quite well: • http://engineering.postmates.com/Spinlocks-Considered-Harmful-On-iOS/

But before merging we need to talk why I choose dispatch semaphores. There is an interesting discussion here: https://github.com/ReactiveCocoa/ReactiveCocoa/issues/2619 To sum up, there is mainly 2 possibilities: 1/ dispatch semaphores: quick but no support for priority inversion, it can be a penalty with POP which plays intensively with the main thread. 2/ pthread mutexes: slower especially with old OSes that do not support fast mutexes (they can be quite slow on 10.8 with heavy contention) but do support priority inversion.

So what do you think?

[nlutsenko]

This is a very good discussion to have.

Taking into account that semaphores do not donate priority - if we encounter a scenario of lock being held by a low-pri thread, we are going to hit a much more visible performance penalty compared to pthread_mutexes. Where the latter are actually tiny bit slower than spin locks (not as fast as semaphores though), but we can guarantee that we won't ever run into priority inversion.

Also did you consider using a dispatch_queue with a user interactive QoS (if available)? It's going to be slower than any previous option, though we might be able to move some of the logic that we have right now to be asynchronous.

cc @richardjrossiii @grp @kimon Feel free to jump on this issue, but I think the best option would be to actually go with pthread mutexes.

[jcbertin]

The right thing to do for Apple would have been to expose objc locks with a new public API! They know they have messed up but they leave us with no really good option… 😡

[jcbertin]

dispatch_queue could be a good alternative but it needs a good comprehension of the code (which I don't have) to avoid side effects.

[jcbertin]

You may also take a look at my first try to implement an adaptive mutex 😬 https://github.com/jcbertin/LockBenchmark

The first tests look pretty good… And you can have the best of both worlds (as we say in France): speed and priority inversion support! You can also remark that non fast mutexes are very slow with high contention scenario.

[jcbertin]

ping

[grp]

Another option would be std::mutex, which fits the idea more but is likely just wrapping pthread_mutex and so might not have the best performance.

[nlutsenko]

@jcbertin, let's circle back on the discussion here. dispatch_queue-based synchronization - let's postpone it to a better time, since it's a massive change and we can get to it later if we see the big performance decrease. dispatch_semaphore does not donate thread priority, which is the reason why we are doing this, right? pthread_mutex, while doing sys calls is technically not as slow and proven to be a good solution.

I vote for changing usage of spin locks here to pthread mutexes, unless there is a valid reason that I've missed on why we want to use dispatch semaphores instead.

[hfossli]

Is pop used from any other thread than main thread? Do we need locks at all?

[nlutsenko]

Please re-open or send a new PR that uses pthread_mutex instead of semaphores, or let's continue the discussion above about the priority donation and semaphores.