Review Request 121570: Kill util/spinlock.h, it is actually worse than QMutex in every case

[Alexander Richardson]

> On Dez. 17, 2014, 1:42 vorm., Aleix Pol Gonzalez wrote:
> > While I would really like to see that happening (droping code is good) I know David Nolden added this because he found it was more performant. Given you claim it's as performant, I would appreciate some numbers.

I definitively believe that this code was better than QMutex at the time, but QMutex has changed a lot since this was introduced (http://woboq.com/blog/internals-of-qmutex-in-qt5.html)
I looked at the implemenation of QMutex and the fastpath (http://code.woboq.org/qt5/qtbase/src/corelib/thread/qmutex.cpp.html#209) and the slowpath (http://code.woboq.org/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp.html#155) is exactly the same as the logic from SpinLock.
QMutex does not have the unnecessarily strong memory odering semantics used in SpinLock and does not have two CAS operations in each loop iteration (QMutex::lock() + QWaitcondition::wait())
The only thing that is worse is the additional comparison to check whether the lock is recursive that we now have and extra call+ret instruction in the fast case (direct lock acquisition), which can be fixed by using QBasicMutex.
If one more condition test + 1 call + 1 return instruction is really such a problem we should consider using this (non-documented) class instead of maintaining our own locking code.
Writing good locking code is very hard and relying on something well tested is IMHO very preferable to maintaining our own in KDevelop.
Just look at how many articles there are on LWN.net about improving locking in the Linux kernel (e.g. http://lwn.net/Articles/531254/ or http://lwn.net/Articles/590243/)


If we have some code with extremely short critical sections and high contention, QMutex could be a problem and using a MCS lock or a ticket lock could be a good idea. However, what would most likely gain a lot more performance than changing the locking code is using finer grained locking.


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121570/#review72155
-----------------------------------------------------------


On Dez. 17, 2014, 1:39 vorm., Alexander Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121570/
> -----------------------------------------------------------
> 
> (Updated Dez. 17, 2014, 1:39 vorm.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> QMutex has the same kind of logic internally, but implements it
> a lot more efficiently. Instead of requiring a QMutex::lock() call and
> a QWaitCondition::wait(), it directly uses the futex() syscall.
> 
> There are also a few problems with the spinlock code:
> It spins using testAndSetOrdered() instead of testAndSetRelaxed().
> - There is also no need to unlock using sequentially consistent order,
> storeRelease() is enough.
> 
> - Even worse is the fact that it uses compare-and-swap
> (QAtomicInt::testAndSet*()) for the spinlock, instead of test-and-set
> (QAtomicIn::fetchAndStore*()) which would be a lot faster.
> 
> - For handling locking with timeout Spinlock uses gettimeofday() which is
> not availableon e.g. Windows.
> 
> - Finally, the class is poorly named, it isn't a spinlock since it sleeps
> in every loop iteration instead of continuously attempting to acquire
> the lock.
> 
> A class that actually tries spinning on the lock for a few iterations
> before taking the slow path that sleeps MIGHT improve the throughput.
> However, finding the right count is hard and will decrease performance
> if not chosen correctly.
> 
> The easiest way to gain a bit of performance in the locking code would
> be to use QBasicMutex for all non-recursive QMutexes, since that would
> allow inlining of the fast path lock() code. But since the class is not
> documented as public we should probably not do that.
> 
> 
> Diffs
> -----
> 
>   language/backgroundparser/urlparselock.cpp d113b66b3802818805f87c5b68b0b6f0c933f1f0 
>   language/duchain/duchain.h 9180cf13e9970b7dd3347ef757b741adab01b733 
>   util/spinlock.cpp 9b4deeee16ba15e1a9515189a441bc9bd4c1e30f 
>   util/spinlock.h cbdb904e39679f136348cb5d6aa8af064c811baf 
>   serialization/referencecounting.h 3aa0244377f5cbcb128575585beefa98d7f41626 
>   serialization/referencecounting.cpp 6091c4003fea3573cef779b08b1d4d187c779a7d 
>   util/CMakeLists.txt a2287bd2ae10d5e7fa64820e46aa514fa6d18cfe 
>   language/duchain/duchain.cpp 7ed779d37bcc7075011a901ab09c24522fb651d4 
> 
> Diff: https://git.reviewboard.kde.org/r/121570/diff/
> 
> 
> Testing
> -------
> 
> Builds
> 
> 
> Thanks,
> 
> Alexander Richardson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20141217/bd016207/attachment.html>