IRC - and spinlocks

[Milian Wolff]

Hey guys.

I'm moving this discussion over to kdevelop-devel, as it is not relevant to 
our users.

Also, I seem to be missing the original mails, i.e. the original context of 
this discussion.

On Thursday 20 November 2014 15:21:08 Kevin Funk wrote:
> On Wednesday 19 November 2014 20:04:41 René J.V. Bertin wrote:
> > On Wednesday November 19 2014 01:02:28 Kevin Funk wrote:
> > 
> > Hi,
> > 
> > > Good point, feel free to benchmark that. We already have benchmarks
> > > covering the repositories ('bench_itemrepository').
> > 
> > Benchmark how, a raw comparison of gettimeofday against a more lowlevel
> > function?
> 
> Nope, the whole interaction with the itemrepository. The
> 'bench_itemrepository' test does that.

Also note that you'll have to do proper profiling with e.g. "perf stat" or 
similar low-level performance counters. gettimeofday is not sufficient for 
that purpose. Also, when you touch locks, you'll have to check the lock 
contention as well. The only tool to properly investigate this I know of is 
VTune.

> > > Well, yes, I'd rather benchmark the itemrepository code first and check
> > > if
> > > just using a mutex would suffice. (We have quite a bit of legacy code
> > > which is overly complex and serves no good, just as the "SpinLock"
> > > class.)
> > 
> > I don't find anything called bench_itemrepository, neither among the
> > filenames nor in the code. That wouldn't happen to be in the KF5 branch,
> > perchance?
> 
> Right, it's in KF5.
> 
> > It's true that you're not likely to require realtime performance (whatever
> > that means :)) but I'm a firm believer in doing things as economically as
> > possible. If it doesn't pay today, it might do so later - and doubly so
> > (because you don't have to figure out the reason of a performance
> > bottleneck and how to remedy it).
> 
> No objections.

I have some objections: Please do proper profiling before changing code for 
the sake of performance. Also see below.

> > The code that uses the spinlocks isn't the easiest to assess in a glance
> > (esp, which operations are on basic types and which involve OO objects,
> > overloaded operators etc). But I see part of the code concerns reference
> > counting, and there it might be possible to protect the more frequently
> > called operations with something like QAtomicInt (i.e. only when exclusive
> > access is really required), and a standard mutex in other places? The
> > other
> > part of the code actually seems to use a spinlock PLUS a QMutex (e.g.
> > DuChain::addDocumentToChain), is that necessary?
> 
> Feel free to investigate and file a patch. I don't really have the time to
> go through it atm. There's lots of legacy code that could/should be
> improved!

Note that multiple mutexes are, sadly, required in some of our convoluted old 
code base. Think of stuff like this (with the variables indicating write 
operations on any kind of thread-shared data):

Bla::foo() { m_foo++; }
Bla::bar() { m_bar++; }
Bla::asdf() { m_foo++; m_bar++; }

In some parts of our code, we guard m_foo and m_bar with its own mutex. It 
could be worth it, if most of the time foo and bar are accessed independently 
of each other. But in ::asdf you'll need to lock both mutices.

So it's always a question of whether the old code is actually doing something 
good and the performance is worth it, or whether it was done blindly due to 
premature optimizations. We sadly lack quite some benchmarks on these parts of 
our codebase...

Anyhow, I'd urge you to not waste your time on any of this Rene. Seriously, 
there are so many more pressing issues that actually make a noticeable impact 
on the usage of KDevelop - please concentrate on these. And while I'll 
certainly not hold up anyone who wants to try to improve our core code, it is 
rather time consuming as you'll need to proof that a) your change is an 
improvment and b) no regression is introduced...

To wrap this up: I agree with both of you that there are many places in our 
code that do locking / multithreading wrong. But, most of the time, exchanging 
the lock types won't help. Rather a rewrite of some parts of the API will be 
required to give proper multi-core scalability. And also note that while e.g. 
the DUChain and ItemRepository with their locks are a huge source of 
scalabilty issues with oldcpp and the other language plugins, in kdev-clang 
this is not the case.

Bye
-- 
Milian Wolff
mail at milianw.de
http://milianw.de