<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 December 17th, 2014, 12:17 p.m. UTC, <b>Milian Wolff</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;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@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 <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">and</em> 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.</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;">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.</p></pre>
<br />
<p>- Aleix</p>
<br />
<p>On December 17th, 2014, 2:32 a.m. UTC, 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 Dec. 17, 2014, 2:32 a.m.</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;">Kill util/spinlock.h, it is actually worse than QMutex in every case</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</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:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It spins using testAndSetOrdered() instead of testAndSetRelaxed().</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There is also no need to unlock using sequentially consistent order,
storeRelease() is enough.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<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<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">()) for the spinlock, instead of test-and-set
(QAtomicIn::fetchAndStore</em>()) which would be a lot faster.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<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>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<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>
</li>
</ul>
<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>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">REVIEW: 121570</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>