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, [email protected], http://www.davidfaure.fr Working on KDE, in particular KDE Frameworks 5
>From 15d501c80db30e46ec6135008bf1b30a9ce43984 Mon Sep 17 00:00:00 2001 From: David Faure <[email protected]> Date: Thu, 14 Mar 2013 13:22:31 +0100 Subject: [PATCH 1/5] Don't hold the RM mutex during executeQuery(), so that other threads can progress. --- libnepomukcore/resource/resourcedata.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libnepomukcore/resource/resourcedata.cpp b/libnepomukcore/resource/resourcedata.cpp index 7a97974..971a46e 100644 --- a/libnepomukcore/resource/resourcedata.cpp +++ b/libnepomukcore/resource/resourcedata.cpp @@ -343,10 +343,6 @@ bool Nepomuk2::ResourceData::load() if ( !m_uri.isValid() ) return false; - lock.unlock(); - QMutexLocker rmlock(&m_rm->mutex); // for updateKickOffLists, but must be locked first - lock.relock(); // we must respect the lock ordering! - const QString oldNaoIdentifier = m_cache[NAO::identifier()].toString(); const QUrl oldNieUrl = m_cache[NIE::url()].toUrl(); @@ -365,6 +361,10 @@ bool Nepomuk2::ResourceData::load() m_cache[p].append( Variant::fromNode( it["o"] ) ); } + lock.unlock(); + QMutexLocker rmlock(&m_rm->mutex); // for updateKickOffLists, but must be locked first + lock.relock(); // we must respect the lock ordering! + const QString newNaoIdentifier = m_cache.value(NAO::identifier()).toString(); const QUrl newNieUrl = m_cache.value(NIE::url()).toUrl(); updateIdentifierLists( oldNaoIdentifier, newNaoIdentifier ); -- 1.7.10.4
>From d8ec8f29cb3864e927ae224acbcfc26e6cabf8c7 Mon Sep 17 00:00:00 2001 From: David Faure <[email protected]> Date: Thu, 14 Mar 2013 13:23:22 +0100 Subject: [PATCH 2/5] early return if nothing to do (noop, just makes the code clearer) --- libnepomukcore/resource/resourcedata.cpp | 97 +++++++++++++++--------------- 1 file changed, 49 insertions(+), 48 deletions(-) diff --git a/libnepomukcore/resource/resourcedata.cpp b/libnepomukcore/resource/resourcedata.cpp index 971a46e..351bf81 100644 --- a/libnepomukcore/resource/resourcedata.cpp +++ b/libnepomukcore/resource/resourcedata.cpp @@ -574,6 +574,9 @@ bool Nepomuk2::ResourceData::isValid() const Nepomuk2::ResourceData* Nepomuk2::ResourceData::determineUri() { QMutexLocker lock(&m_dataMutex); + if( !m_uri.isEmpty() ) { + return this; + } // We have the following possible situations: // 1. m_uri is already valid @@ -598,64 +601,62 @@ Nepomuk2::ResourceData* Nepomuk2::ResourceData::determineUri() // -> use r as m_uri // - if( m_uri.isEmpty() ) { - Soprano::Model* model = MAINMODEL; + Soprano::Model* model = MAINMODEL; - if( !m_naoIdentifier.isEmpty() ) { - // - // Not valid. Checking for nao:identifier - // - QString query = QString::fromLatin1("select distinct ?r where { ?r %1 %2. } LIMIT 1") - .arg( Soprano::Node::resourceToN3(Soprano::Vocabulary::NAO::identifier()) ) - .arg( Soprano::Node::literalToN3( m_naoIdentifier ) ); - - Soprano::QueryResultIterator it = model->executeQuery( query, Soprano::Query::QueryLanguageSparql ); - if( it.next() ) { - m_uri = it["r"].uri(); - it.close(); - } - } - else { - // - // In one query determine if the URI is already used as resource URI or as - // nie:url - // - QString query = QString::fromLatin1("select distinct ?r ?o where { " - "{ ?r %1 %2 . FILTER(?r!=%2) . } " - "UNION " - "{ %2 ?p ?o . } " - "} LIMIT 1") - .arg( Soprano::Node::resourceToN3( Nepomuk2::Vocabulary::NIE::url() ) ) - .arg( Soprano::Node::resourceToN3( m_nieUrl ) ); - Soprano::QueryResultIterator it = model->executeQuery( query, Soprano::Query::QueryLanguageSparql ); - if( it.next() ) { - QUrl uri = it["r"].uri(); - if( uri.isEmpty() ) { - // FIXME: Find a way to avoid this - // The url is actually the uri - legacy data - m_uri = m_nieUrl; - } - else { - m_uri = uri; - } - } + if( !m_naoIdentifier.isEmpty() ) { + // + // Not valid. Checking for nao:identifier + // + QString query = QString::fromLatin1("select distinct ?r where { ?r %1 %2. } LIMIT 1") + .arg( Soprano::Node::resourceToN3(Soprano::Vocabulary::NAO::identifier()) ) + .arg( Soprano::Node::literalToN3( m_naoIdentifier ) ); + + Soprano::QueryResultIterator it = model->executeQuery( query, Soprano::Query::QueryLanguageSparql ); + if( it.next() ) { + m_uri = it["r"].uri(); + it.close(); } - + } + else { // - // Move us to the final data hash now that the URI is known + // In one query determine if the URI is already used as resource URI or as + // nie:url // - if( !m_uri.isEmpty() ) { - m_cacheDirty = true; - ResourceDataHash::iterator it = m_rm->m_initializedData.find(m_uri); - if( it == m_rm->m_initializedData.end() ) { - m_rm->m_initializedData.insert( m_uri, this ); + QString query = QString::fromLatin1("select distinct ?r ?o where { " + "{ ?r %1 %2 . FILTER(?r!=%2) . } " + "UNION " + "{ %2 ?p ?o . } " + "} LIMIT 1") + .arg( Soprano::Node::resourceToN3( Nepomuk2::Vocabulary::NIE::url() ) ) + .arg( Soprano::Node::resourceToN3( m_nieUrl ) ); + Soprano::QueryResultIterator it = model->executeQuery( query, Soprano::Query::QueryLanguageSparql ); + if( it.next() ) { + QUrl uri = it["r"].uri(); + if( uri.isEmpty() ) { + // FIXME: Find a way to avoid this + // The url is actually the uri - legacy data + m_uri = m_nieUrl; } else { - return it.value(); + m_uri = uri; } } } + // + // Move us to the final data hash now that the URI is known + // + if( !m_uri.isEmpty() ) { + m_cacheDirty = true; + ResourceDataHash::iterator it = m_rm->m_initializedData.find(m_uri); + if( it == m_rm->m_initializedData.end() ) { + m_rm->m_initializedData.insert( m_uri, this ); + } + else { + return it.value(); + } + } + return this; } -- 1.7.10.4
>From 3ed0a7e33d9e4ec6f59a9e8764a868f81fac4077 Mon Sep 17 00:00:00 2001 From: David Faure <[email protected]> Date: Thu, 14 Mar 2013 13:23:53 +0100 Subject: [PATCH 3/5] Protect m_watcher with the RM mutex --- libnepomukcore/resource/resourcemanager.cpp | 2 ++ libnepomukcore/resource/resourcemanager_p.h | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/libnepomukcore/resource/resourcemanager.cpp b/libnepomukcore/resource/resourcemanager.cpp index 2b32be0..7f08367 100644 --- a/libnepomukcore/resource/resourcemanager.cpp +++ b/libnepomukcore/resource/resourcemanager.cpp @@ -410,6 +410,7 @@ void Nepomuk2::ResourceManagerPrivate::addToWatcher( const QUrl& uri ) if( uri.isEmpty() ) return; + QMutexLocker lock( &mutex ); if( !m_watcher ) { m_watcher = new ResourceWatcher(m_manager); // @@ -436,6 +437,7 @@ void Nepomuk2::ResourceManagerPrivate::addToWatcher( const QUrl& uri ) void Nepomuk2::ResourceManagerPrivate::removeFromWatcher( const QUrl& uri ) { + QMutexLocker lock( &mutex ); if( uri.isEmpty() || !m_watcher ) return; diff --git a/libnepomukcore/resource/resourcemanager_p.h b/libnepomukcore/resource/resourcemanager_p.h index 68479d5..7516992 100644 --- a/libnepomukcore/resource/resourcemanager_p.h +++ b/libnepomukcore/resource/resourcemanager_p.h @@ -54,7 +54,7 @@ namespace Nepomuk2 { /// used to protect the initialization QMutex initMutex; - /// used to protect all data in ResourceManager + /// used to protect all data in ResourceManagerPrivate QMutex mutex; /// contains all initialized ResourceData object, i.e. all those which -- 1.7.10.4
>From 5cda03c588f9f07c12fb7a90f6fa25511e217764 Mon Sep 17 00:00:00 2001 From: David Faure <[email protected]> Date: Thu, 14 Mar 2013 14:26:45 +0100 Subject: [PATCH 4/5] fix lock order in resetAll --- libnepomukcore/resource/resourcedata.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/libnepomukcore/resource/resourcedata.cpp b/libnepomukcore/resource/resourcedata.cpp index 351bf81..a9c28a9 100644 --- a/libnepomukcore/resource/resourcedata.cpp +++ b/libnepomukcore/resource/resourcedata.cpp @@ -159,9 +159,9 @@ QUrl Nepomuk2::ResourceData::type() void Nepomuk2::ResourceData::resetAll( bool isDelete ) { - QMutexLocker locker(&m_dataMutex); // remove us from all caches (store() will re-insert us later if necessary) - m_rm->mutex.lock(); + QMutexLocker rmMutexLocker(&m_rm->mutex); + QMutexLocker locker(&m_dataMutex); // IMPORTANT: // Remove from the kickOffList before removing from the resource watcher @@ -177,7 +177,6 @@ void Nepomuk2::ResourceData::resetAll( bool isDelete ) m_rm->m_initializedData.remove( m_uri ); removeFromWatcher(); } - m_rm->mutex.unlock(); // reset all variables m_uri.clear(); -- 1.7.10.4
>From 82680e8877867c263025d20837484273533a08a3 Mon Sep 17 00:00:00 2001 From: David Faure <[email protected]> Date: Thu, 14 Mar 2013 14:27:47 +0100 Subject: [PATCH 5/5] Rework determineUri so that the RM lock isn't held during the executeQuery --- libnepomukcore/resource/resource.cpp | 19 ++----------------- libnepomukcore/resource/resourcedata.cpp | 22 ++++++++++++++++------ libnepomukcore/resource/resourcedata.h | 8 ++------ 3 files changed, 20 insertions(+), 29 deletions(-) diff --git a/libnepomukcore/resource/resource.cpp b/libnepomukcore/resource/resource.cpp index 7158802..f019b6a 100644 --- a/libnepomukcore/resource/resource.cpp +++ b/libnepomukcore/resource/resource.cpp @@ -700,25 +700,10 @@ void Nepomuk2::Resource::determineFinalResourceData() const return; } - QMutexLocker lock( &m_data->rm()->mutex ); - // Get an initialized ResourceData instance ResourceData* oldData = m_data; - ResourceData* newData = m_data->determineUri(); - - Q_ASSERT(oldData); - Q_ASSERT(newData); - - // in case we get an already existing one we update all instances - // using the old ResourceData to avoid the overhead of calling - // determineUri over and over - if( newData != oldData ) { - Q_FOREACH( Resource* res, m_data->resources() ) { - res->m_data = newData; // one of these resources is "this", so this updates our m_data. - oldData->deref( res ); - newData->ref( res ); - } - } + + m_data->determineUri(); // note that this can change the value of m_data if ( !oldData->cnt() ) delete oldData; diff --git a/libnepomukcore/resource/resourcedata.cpp b/libnepomukcore/resource/resourcedata.cpp index a9c28a9..ff5e24e 100644 --- a/libnepomukcore/resource/resourcedata.cpp +++ b/libnepomukcore/resource/resourcedata.cpp @@ -569,12 +569,11 @@ bool Nepomuk2::ResourceData::isValid() const return !m_uri.isEmpty() || !m_nieUrl.isEmpty() || !m_naoIdentifier.isEmpty(); } -// Caller must hold the ResourceManager mutex (since the RM owns the returned ResourceData pointer) -Nepomuk2::ResourceData* Nepomuk2::ResourceData::determineUri() +void Nepomuk2::ResourceData::determineUri() { QMutexLocker lock(&m_dataMutex); if( !m_uri.isEmpty() ) { - return this; + return; } // We have the following possible situations: @@ -646,17 +645,27 @@ Nepomuk2::ResourceData* Nepomuk2::ResourceData::determineUri() // Move us to the final data hash now that the URI is known // if( !m_uri.isEmpty() ) { + lock.unlock(); // respect mutex order + QMutexLocker locker(&m_rm->mutex); + lock.relock(); m_cacheDirty = true; ResourceDataHash::iterator it = m_rm->m_initializedData.find(m_uri); if( it == m_rm->m_initializedData.end() ) { m_rm->m_initializedData.insert( m_uri, this ); } else { - return it.value(); + ResourceData* foundData = it.value(); + + // in case we get an already existing one we update all instances + // using the old ResourceData to avoid the overhead of calling + // determineUri over and over + Q_FOREACH (Resource* res, m_resources) { + res->m_data = foundData; // this can include our caller + this->deref( res ); + foundData->ref( res ); + } } } - - return this; } @@ -721,6 +730,7 @@ void Nepomuk2::ResourceData::updateIdentifierLists(const QString& oldIdentifier, // Caller must lock RM mutex first void Nepomuk2::ResourceData::updateKickOffLists(const QUrl& uri, const Nepomuk2::Variant& variant) { + Q_ASSERT(!m_rm->mutex.tryLock()); if( uri == NIE::url() ) updateUrlLists( m_cache.value(NIE::url()).toUrl(), variant.toUrl() ); else if( uri == NAO::identifier() ) diff --git a/libnepomukcore/resource/resourcedata.h b/libnepomukcore/resource/resourcedata.h index 3950876..58db9c8 100644 --- a/libnepomukcore/resource/resourcedata.h +++ b/libnepomukcore/resource/resourcedata.h @@ -138,13 +138,9 @@ namespace Nepomuk2 { * and add m_data into ResourceManagerPrivate::m_initializedData * or it will find another ResourceData instance in m_initializedData * which represents the same resource. The ResourceData that should be - * used is returned. - * - * \returns The initialized ResourceData object representing the actual resource. - * - * The resource manager mutex needs to be locked before calling this method + * used is set in all the associated resources. */ - ResourceData* determineUri(); + void determineUri(); void invalidateCache(); -- 1.7.10.4
_______________________________________________ Nepomuk mailing list [email protected] https://mail.kde.org/mailman/listinfo/nepomuk
