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

[Alexander Richardson]

> On Dez. 17, 2014, 12:54 nachm., Kevin Funk wrote:
> > language/backgroundparser/urlparselock.cpp, line 42
> > <https://git.reviewboard.kde.org/r/121570/diff/1/?file=333559#file333559line42>
> >
> >     Unrelated + I have a patch ready that fixes the MSVC build.

This is needed because spinlock.h included unistd.h, which is now missing. If I don't add this change the code won't compile.


- Alexander


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


On Dez. 17, 2014, 2:32 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, 2:32 vorm.)
> 
> 
> 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/d767a7c4/attachment.html>