[Nepomuk] High mutex contention in Nepomuk2::Resource

[David Faure]

On Monday 11 March 2013 21:33:21 Simeon Bird wrote:
> > Yes, I need to take the time to write a blog post on how to set up Qt for
> > helgrind (and helgrind for Qt). Ping me back about this in a few days if I
> > don't move forward on it :)

(OK, working on this, but I'm fixing data races in Qt and bugs in helgrind 
along the way, so it will take a bit more time.)

> > Which caller, and which lock? I see no lock in the watcher -- and if you
> > mean the ResourceManager lock, that one is already too much of a
> > contention point, we should definitely not re-use it for objects that are
> > independent from ResourceManager.
> 
> Originally I was thinking the big lock, which, most of the time, was
> already held by
> the ResourceData method that called addToWatcher, but then I thought of a
> way to avoid the lock being taken in the ResourceData method, so we can't
> do that anymore :)

"the ResourceData method" -- did you mean load()?

> So I added an extra mutex for the ResourceWatcher, but I'll switch it
> to the single-thread stuff you suggest.

OK.

In general I'm backing down a bit from "let's not use the big lock", though:
I'm working on fixes so that it's not held for long (i.e. not during socket 
communication), so locking it for fast operations is fine.

> >> I guess there are two possible fixes:
> >> 1) Just protect the ResourceWatcher with the rm mutex.
> >> 2) Call all methods via a QAutoConnection
> >> 
> >> I like the first, because I don't really understand what is happening
> >> with
> >> the moveToThread stuff, but is there some reason I am missing why the
> >> mutex
> >> is not enough and this is really necessary?
> > 
> > I'd prefer solution 2, so that the watcher lives in a single thread
> > entirely, which is important for libdbus usage -- and which will give
> > better performance than locking the big mutex even more than today.
> 
> hmm, this is more tricky - we need to safely access
> m_watcher->resourceCount() somehow,
> although I think I see a way around that...

I think start and stop should simply be done inside the implementation of 
addResource/removeResource. This would solve the problem completely.
Anything else is racy.


On my side, I have 5 patches for you to review...

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE, in particular KDE Frameworks 5
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Don-t-hold-the-RM-mutex-during-executeQuery-so-that-.patch
Type: text/x-patch
Size: 1526 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130314/c0f91636/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-early-return-if-nothing-to-do-noop-just-makes-the-co.patch
Type: text/x-patch
Size: 5540 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130314/c0f91636/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Protect-m_watcher-with-the-RM-mutex.patch
Type: text/x-patch
Size: 1711 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130314/c0f91636/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-fix-lock-order-in-resetAll.patch
Type: text/x-patch
Size: 1225 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130314/c0f91636/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Rework-determineUri-so-that-the-RM-lock-isn-t-held-d.patch
Type: text/x-patch
Size: 4832 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130314/c0f91636/attachment-0004.patch>