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

[Alexander Richardson]

-----------------------------------------------------------
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 (updated)
-------

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/c8acdc77/attachment.html>