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

[Aleix Pol]

On Wed, Dec 17, 2014 at 2:11 PM, Alexander Richardson <
arichardson.kde at gmail.com> wrote:
>
>    This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121570/
>   This change has been marked as submitted.
>   Review request for KDevelop.
> By Alexander Richardson.
>
> *Updated Dec. 17, 2014, 1:11 p.m.*
>  *Repository: * kdevplatform
> Description
>
> Kill util/spinlock.h, it is actually worse than QMutex in every case
>
> QMutex has changed a lot since this code was introduced and now has the
> same kind of logic as SpinLock internally, but implements it a lot more
> efficiently. Instead of requiring a QMutex::lock() call and a call to
> QWaitCondition::wait() (which is two more compare-and-swap instructions)
> 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.
>
> REVIEW: 121570
>
>   Testing
>
> Builds
>
>   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)
>
> View Diff <https://git.reviewboard.kde.org/r/121570/diff/>
>

FWIW, I'm having huge UI freezes since I last upgraded. Have you tried
using it for a while?

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