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

[Milian Wolff]

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

Ship it!


Great work, and nice find ;-) I had that on my TODO for quite some time, cool to see you getting dirty with these internals of KDevelop code.

@Aleix: David is infamous, imo, for writing "efficient" ugly code that is not really much more efficient than clean code such as this one. I'd rather we clean up our code base and then apply properly profile-guided optimization techniques _and_ write benchmarks, instead of littering the code base with these "this has to be faster" uglyness which often is not faster at all. That fact that David never wrote a benchmark for this code shows how premature the spinlock application is, imo.

- Milian Wolff


On Dec. 17, 2014, 2:32 a.m., Alexander Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121570/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2014, 2:32 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> 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
> 
> 
> 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/7d168a9b/attachment.html>