<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/121570/">https://git.reviewboard.kde.org/r/121570/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Dezember 17th, 2014, 1:42 vorm. GMT, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While I would really like to see that happening (droping code is good) I know David Nolden added this because he found it was more performant. Given you claim it's as performant, I would appreciate some numbers.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I definitively believe that this code was better than QMutex at the time, but QMutex has changed a lot since this was introduced (http://woboq.com/blog/internals-of-qmutex-in-qt5.html)
I looked at the implemenation of QMutex and the fastpath (http://code.woboq.org/qt5/qtbase/src/corelib/thread/qmutex.cpp.html#209) and the slowpath (http://code.woboq.org/qt5/qtbase/src/corelib/thread/qmutex_linux.cpp.html#155) is exactly the same as the logic from SpinLock.
QMutex does not have the unnecessarily strong memory odering semantics used in SpinLock and does not have two CAS operations in each loop iteration (QMutex::lock() + QWaitcondition::wait())
The only thing that is worse is the additional comparison to check whether the lock is recursive that we now have and extra call+ret instruction in the fast case (direct lock acquisition), which can be fixed by using QBasicMutex.
If one more condition test + 1 call + 1 return instruction is really such a problem we should consider using this (non-documented) class instead of maintaining our own locking code.
Writing good locking code is very hard and relying on something well tested is IMHO very preferable to maintaining our own in KDevelop.
Just look at how many articles there are on LWN.net about improving locking in the Linux kernel (e.g. http://lwn.net/Articles/531254/ or http://lwn.net/Articles/590243/)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If we have some code with extremely short critical sections and high contention, QMutex could be a problem and using a MCS lock or a ticket lock could be a good idea. However, what would most likely gain a lot more performance than changing the locking code is using finer grained locking.</p></pre>
<br />
<p>- Alexander</p>
<br />
<p>On Dezember 17th, 2014, 1:39 vorm. GMT, Alexander Richardson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Alexander Richardson.</div>
<p style="color: grey;"><i>Updated Dez. 17, 2014, 1:39 vorm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QMutex has the same kind of logic internally, but implements it
a lot more efficiently. Instead of requiring a QMutex::lock() call and
a QWaitCondition::wait(), it directly uses the futex() syscall.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">- 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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">- For handling locking with timeout Spinlock uses gettimeofday() which is
not availableon e.g. Windows.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">- 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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Builds</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>language/backgroundparser/urlparselock.cpp <span style="color: grey">(d113b66b3802818805f87c5b68b0b6f0c933f1f0)</span></li>
<li>language/duchain/duchain.h <span style="color: grey">(9180cf13e9970b7dd3347ef757b741adab01b733)</span></li>
<li>util/spinlock.cpp <span style="color: grey">(9b4deeee16ba15e1a9515189a441bc9bd4c1e30f)</span></li>
<li>util/spinlock.h <span style="color: grey">(cbdb904e39679f136348cb5d6aa8af064c811baf)</span></li>
<li>serialization/referencecounting.h <span style="color: grey">(3aa0244377f5cbcb128575585beefa98d7f41626)</span></li>
<li>serialization/referencecounting.cpp <span style="color: grey">(6091c4003fea3573cef779b08b1d4d187c779a7d)</span></li>
<li>util/CMakeLists.txt <span style="color: grey">(a2287bd2ae10d5e7fa64820e46aa514fa6d18cfe)</span></li>
<li>language/duchain/duchain.cpp <span style="color: grey">(7ed779d37bcc7075011a901ab09c24522fb651d4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121570/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>