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

[David Nolden]

> On Dez. 17, 2014, 12:17 nachm., Milian Wolff wrote:
> > 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.
> 
> Aleix Pol Gonzalez wrote:
>     Fair enough, I withdraw my comment. I'm pretty sure he had some reasons to do so, but for now let's assume that future structural optimizations will require proper benchmarks, then.
> 
> David Nolden wrote:
>     The benchmark is called 'duchainify'. Yes, SpinLock was much faster for this. SpinLock was used in some places which are accessed on an extremely high frequency, and the lock does not encapsulate much code, so that there should not be much lock contention. The sheer overhead of some non-inlined function calls already plays a role on this frequency. That said, maybe QMutex has improved in Qt5. But I recommend to try out duchainify to compare the performance.
> 
> Milian Wolff wrote:
>     Even in Qt4, there is lockInline, which is being used by QMutexLocker. In Qt5, it just became the default. And QMutex got further improvements internally.
>     
>     Anyhow, I'll do a before/after duchainify comparison sometime later this week to verify this does not give us a noticeable performance penalty.
>     
>     Nice to see you still reading KDevelop related review requests David! Any plans to get your hands dirty again? :)

I guess the profiling is not necessary, because the new QMutex and QMutexLocker should have the same overhead as SpinLock when there is no lock contention (I took a look at the code, it's all inlined, and it uses an atomic counter as fast-path).

I'm interested in the current development, and I hope that you guys will soon get the clang support to a similar feature-set and efficiency as the old C++ support, then I might consider getting my hands dirty again. ;)


- David


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


On Dez. 17, 2014, 1:11 nachm., Alex Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121570/
> -----------------------------------------------------------
> 
> (Updated Dez. 17, 2014, 1:11 nachm.)
> 
> 
> 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,
> 
> Alex Richardson
> 
>

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