Review Request 121570: Kill util/spinlock.h, it is actually worse than QMutex in every case
aleixpol at kde.org
Wed Dec 17 16:55:08 UTC 2014
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:
> This change has been marked as submitted.
> Review request for KDevelop.
> By Alexander Richardson.
> *Updated Dec. 17, 2014, 1:11 p.m.*
> *Repository: * kdevplatform
> 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
> - language/backgroundparser/urlparselock.cpp
> - language/duchain/duchain.h (9180cf13e9970b7dd3347ef757b741adab01b733)
> - util/spinlock.cpp (9b4deeee16ba15e1a9515189a441bc9bd4c1e30f)
> - util/spinlock.h (cbdb904e39679f136348cb5d6aa8af064c811baf)
> - serialization/referencecounting.h
> - serialization/referencecounting.cpp
> - util/CMakeLists.txt (a2287bd2ae10d5e7fa64820e46aa514fa6d18cfe)
> - language/duchain/duchain.cpp
> 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?
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the KDevelop-devel