On Tue, May 18, 2010 at 4:07 PM, Sebastian Trüg <[email protected]> wrote:
> On 05/18/2010 12:30 PM, Vishesh Handa wrote:
> > 2. Don't we need to protect m_resources via a QMutex?
> >
> >
> > I'm currently blissfully unaware about concurrent programming. I assure
> > you I'm trying to learn, but till then could you take care of it?
>
> ok, will do.
>
> > [3. Wouldn't shoudBeDeleted() make more sense in
> > ResourceManagerPrivate?]
> >
> > :-/ It checks if a ResourceData should be deleted. I considered making
> > it static but this approach had less clutter. Could you please explain
> > why it should be in ResourceManagerPrivate?
>
> I thought that conceptually it is the resourcemanager maintaining the
> resourcedata objects. So it should also be the one to decide when one
> has to be deleted.
>
Okay. I've changed it.
The shouldBeDeleted function is currently called twice. Two times in
Resource (destructor and operator==), and once in ResourceData::replaceWith.
- Vishesh Handa
> > As for ResourceManagerPrivate: this is just how it is done in KDE: it
> is
> > a convention to name private classes that way. You can look in KDE
> and
> > Qt code all over the place and find these private classes. Their main
> > purpose is to keep as much member variables and methods out of the
> main
> > class as possible to make it possible to add and remove those without
> > breaking binary compatibility.
> >
> >
> > Yea. I know. The "pimpl" idiom. :)
> >
> > I will apply the patch and test it.
> >
> > Okay. I'll do the same.
> >
> > - Vishesh Handa
> >
> >
> > Cheers,
> > Sebastian
> >
> > On 05/16/2010 04:11 PM, Vishesh Handa wrote:
> > > Cleaned up the code a little bit.
> > > - fixed a small bug in Resource::~Resource(). (my fault)
> > > - m_resources is now managed along with reference counting.
> > > - Added a function ResourceData::shouldBeDeleted() to avoid
> > duplication
> > > of code.
> > >
> > > I think the shouldBeDeleted() function should be in another patch.
> > >
> > > Btw, this code still hasn't been extensively tested. I'll do the
> > testing
> > > later on. I just though I should share the patch so that I can get
> > your
> > > comments.
> > >
> > > - Vishesh Handa
> >
> >
>
Index: resourcemanager_p.h
===================================================================
--- resourcemanager_p.h (revision 1126675)
+++ resourcemanager_p.h (working copy)
@@ -97,7 +97,7 @@ namespace Nepomuk {
*/
ResourceData* data( const QUrl& uri, const QUrl& type );
- bool dataCacheFull();
+ bool dataCacheFull() const;
/**
* Delete unused ResourceData objects from the cache.
@@ -115,6 +115,8 @@ namespace Nepomuk {
*/
void determineAllUris();
+ bool shouldBeDeleted( ResourceData * rd ) const;
+
QList<ResourceData*> allResourceData();
QList<ResourceData*> allResourceDataOfType( const QUrl& type );
QList<ResourceData*> allResourceDataWithProperty( const QUrl& _uri, const Variant& v );
Index: resourcedata.cpp
===================================================================
--- resourcedata.cpp (revision 1126675)
+++ resourcedata.cpp (working copy)
@@ -60,7 +60,6 @@ Nepomuk::ResourceData::ResourceData( con
m_kickoffUri( uri ),
m_mainType( type ),
m_modificationMutex(QMutex::Recursive),
- m_proxyData(0),
m_cacheDirty(true),
m_pimoThing(0),
m_groundingOccurence(0),
@@ -103,16 +102,12 @@ bool Nepomuk::ResourceData::isFile()
QUrl Nepomuk::ResourceData::uri() const
{
- if( m_proxyData )
- return m_proxyData->uri();
return m_uri;
}
QUrl Nepomuk::ResourceData::type()
{
- if( m_proxyData )
- return m_proxyData->type();
load();
return m_mainType;
}
@@ -120,8 +115,6 @@ QUrl Nepomuk::ResourceData::type()
QList<QUrl> Nepomuk::ResourceData::allTypes()
{
- if( m_proxyData )
- return m_proxyData->allTypes();
load();
return m_types;
}
@@ -129,10 +122,7 @@ QList<QUrl> Nepomuk::ResourceData::allTy
void Nepomuk::ResourceData::setTypes( const QList<QUrl>& types )
{
- if( m_proxyData ) {
- m_proxyData->setTypes( types );
- }
- else if ( store() ) {
+ if ( store() ) {
QMutexLocker lock(&m_modificationMutex);
// reset types
@@ -155,19 +145,9 @@ void Nepomuk::ResourceData::setTypes( co
void Nepomuk::ResourceData::resetAll( bool isDelete )
{
- // reset proxy
- bool hadProxy = false;
- if( m_proxyData ) {
- hadProxy = true;
- if( !m_proxyData->deref() &&
- rm()->dataCacheFull() )
- delete m_proxyData;
- m_proxyData = 0;
- }
-
// remove us from all caches (store() will re-insert us later if necessary)
m_rm->mutex.lock();
- if( !m_uri.isEmpty() && !hadProxy ) // if we had a proxy we were not in m_initializedData ourselves
+ if( !m_uri.isEmpty() )
m_rm->m_initializedData.remove( m_uri );
if( !m_kickoffUri.isEmpty() )
m_rm->m_uriKickoffData.remove( m_kickoffUri );
@@ -198,20 +178,13 @@ void Nepomuk::ResourceData::resetAll( bo
QHash<QUrl, Nepomuk::Variant> Nepomuk::ResourceData::allProperties()
{
- if( m_proxyData )
- return m_proxyData->allProperties();
-
load();
-
return m_cache;
}
bool Nepomuk::ResourceData::hasProperty( const QUrl& uri )
{
- if( m_proxyData )
- return m_proxyData->hasProperty( uri );
-
if( !load() )
return false;
@@ -225,9 +198,6 @@ bool Nepomuk::ResourceData::hasProperty(
bool Nepomuk::ResourceData::hasProperty( const QUrl& p, const Variant& v )
{
- if( m_proxyData )
- return m_proxyData->hasProperty( p, v );
-
if( !load() )
return false;
@@ -247,9 +217,6 @@ bool Nepomuk::ResourceData::hasProperty(
bool Nepomuk::ResourceData::hasType( const QUrl& uri )
{
- if( m_proxyData )
- return m_proxyData->hasType( uri );
-
load();
return constHasType( uri );
}
@@ -275,9 +242,6 @@ bool Nepomuk::ResourceData::constHasType
Nepomuk::Variant Nepomuk::ResourceData::property( const QUrl& uri )
{
- if( m_proxyData )
- return m_proxyData->property( uri );
-
if ( load() ) {
// we need to protect the reading, too. load my be triggered from another thread's
// connection to a Soprano statement signal
@@ -477,9 +441,6 @@ void Nepomuk::ResourceData::setProperty(
{
Q_ASSERT( uri.isValid() );
- if( m_proxyData )
- return m_proxyData->setProperty( uri, value );
-
// step 0: make sure this resource is in the store
if ( store() ) {
QMutexLocker lock(&m_modificationMutex);
@@ -532,9 +493,6 @@ void Nepomuk::ResourceData::removeProper
{
Q_ASSERT( uri.isValid() );
- if( m_proxyData )
- return m_proxyData->removeProperty( uri );
-
QMutexLocker lock(&m_modificationMutex);
if ( determineUri() ) {
@@ -558,20 +516,14 @@ void Nepomuk::ResourceData::remove( bool
// That way the proxy handling will take care of the rest.
m_rm->determineAllUris();
- if( m_proxyData ) {
- m_proxyData->remove( recursive );
- }
- else {
QMutexLocker lock(&m_modificationMutex);
-
if ( determineUri() ) {
MAINMODEL->removeAllStatements( Statement( m_uri, Node(), Node() ) );
if ( recursive ) {
MAINMODEL->removeAllStatements( Statement( Node(), Node(), m_uri ) );
}
}
- }
resetAll();
}
@@ -579,9 +531,6 @@ void Nepomuk::ResourceData::remove( bool
bool Nepomuk::ResourceData::exists()
{
- if( m_proxyData )
- return m_proxyData->exists();
-
if( determineUri() ) {
return MAINMODEL->containsAnyStatement( Statement( m_uri, Node(), Node() ) );
}
@@ -592,19 +541,12 @@ bool Nepomuk::ResourceData::exists()
bool Nepomuk::ResourceData::isValid() const
{
- if( m_proxyData )
- return m_proxyData->isValid();
-
return( !m_mainType.isEmpty() && ( !m_uri.isEmpty() || !m_kickoffUri.isEmpty() || !m_kickoffId.isEmpty() ) );
}
bool Nepomuk::ResourceData::determineUri()
{
- if( m_proxyData )
- return m_proxyData->determineUri();
-
- else {
//
// Preconditions:
// 1. m_kickoffId is not a local file path or URL (use m_kickoffUri for that)
@@ -744,14 +686,29 @@ bool Nepomuk::ResourceData::determineUri
m_rm->m_initializedData.insert( m_uri, this );
}
else {
- m_proxyData = it.value();
- m_proxyData->ref();
+ // As the ResourceData already exists in m_initializedData, we simply
+ // make all the the Resources which contain "this" ResourceData, point
+ // to the initialized ResourceData instead.
+ replaceWith( it.value() );
}
}
}
return !m_uri.isEmpty();
+}
+
+
+void Nepomuk::ResourceData::replaceWith(Nepomuk::ResourceData* rd)
+{
+ foreach( Resource * res, m_resources) {
+ ResourceData * oldData = res->m_data;
+ if ( oldData && !oldData->deref( res ) && rm()->shouldBeDeleted( oldData ) )
+ delete oldData;
+
+ res->m_data = rd;
+ res->m_data->ref( res );
}
+ delete this;
}
@@ -796,17 +753,13 @@ Nepomuk::Thing Nepomuk::ResourceData::pi
bool Nepomuk::ResourceData::operator==( const ResourceData& other ) const
{
- const ResourceData* that = this;
- if( m_proxyData )
- that = m_proxyData;
-
- if( that == &other )
+ if( this == &other )
return true;
- return( that->m_uri == other.m_uri &&
- that->m_mainType == other.m_mainType &&
- that->m_kickoffUri == other.m_kickoffUri &&
- that->m_kickoffId == other.m_kickoffId );
+ return( m_uri == other.m_uri &&
+ m_mainType == other.m_mainType &&
+ m_kickoffUri == other.m_kickoffUri &&
+ m_kickoffId == other.m_kickoffId );
}
@@ -818,8 +771,7 @@ QDebug Nepomuk::ResourceData::operator<<
m_uri.url(),
m_mainType.toString())
.arg(m_ref);
- if(m_proxyData)
- dbg << QLatin1String(", proxy: " ) << *m_proxyData;
+
return dbg << QLatin1String("]");
}
Index: resourcemanager.cpp
===================================================================
--- resourcemanager.cpp (revision 1126675)
+++ resourcemanager.cpp (working copy)
@@ -197,7 +197,7 @@ QList<Nepomuk::ResourceData*> Nepomuk::R
}
-bool Nepomuk::ResourceManagerPrivate::dataCacheFull()
+bool Nepomuk::ResourceManagerPrivate::dataCacheFull() const
{
return dataCnt >= 1000;
}
@@ -230,6 +230,22 @@ void Nepomuk::ResourceManagerPrivate::de
}
+bool Nepomuk::ResourceManagerPrivate::shouldBeDeleted( ResourceData * rd ) const
+{
+ if ( rd->cnt() )
+ return false;
+
+ //
+ // We delete data objects in one of two cases:
+ // 1. They are not valid and as such not in one of the ResourceManagerPrivate kickoff lists
+ // 2. The cache is already full and we need to clean up
+ //
+ if ( !rd->isValid() || dataCacheFull() ) {
+ return true;
+ }
+}
+
+
void Nepomuk::ResourceManagerPrivate::_k_storageServiceInitialized( bool success )
{
if( success ) {
Index: resourcedata.h
===================================================================
--- resourcedata.h (revision 1126675)
+++ resourcedata.h (working copy)
@@ -37,6 +37,7 @@
namespace Nepomuk {
+ class Resource;
class ResourceManagerPrivate;
class ResourceData
@@ -45,11 +46,14 @@ namespace Nepomuk {
explicit ResourceData( const QUrl& uri, const QString& kickoffId_, const QUrl& type_, ResourceManagerPrivate* rm );
~ResourceData();
- inline bool ref() {
+ inline bool ref(Nepomuk::Resource* res) {
+ m_resources.push_back( res );
return m_ref.ref();
}
- inline bool deref() {
+
+ inline bool deref(Nepomuk::Resource* res) {
+ m_resources.removeAll( res );
return m_ref.deref();
}
@@ -155,7 +159,7 @@ namespace Nepomuk {
ResourceManagerPrivate* rm() const { return m_rm; }
- ResourceData* proxy() const { return m_proxyData; }
+ bool shouldBeDeleted() const;
private:
void loadType( const QUrl& type );
@@ -187,13 +191,14 @@ namespace Nepomuk {
QMutex m_determineUriMutex;
mutable QMutex m_modificationMutex;
+ /// Contains a list of resources which contain this ResourceData
+ QList<Resource *> m_resources;
+
/**
- * Used to virtually merge two data objects representing the same
- * resource. This happens if the resource was once created using its
- * actual URI and once via its ID. To prevent early loading we allow
- * this scenario.
+ * Makes the m_data pointer of all the Resources in m_resources point to
+ * rd, instead of 'this'. Handles refernce counting as well.
*/
- ResourceData* m_proxyData;
+ void replaceWith( ResourceData * rd );
QHash<QUrl, Variant> m_cache;
bool m_cacheDirty;
Index: resource.cpp
===================================================================
--- resource.cpp (revision 1126675)
+++ resource.cpp (working copy)
@@ -47,14 +47,16 @@
Nepomuk::Resource::Resource()
{
m_data = ResourceManager::instance()->d->data( QUrl(), QUrl() );
- m_data->ref();
+ if ( m_data )
+ m_data->ref( this );
}
Nepomuk::Resource::Resource( ResourceManager* manager )
{
m_data = manager->d->data( QUrl(), QUrl() );
- m_data->ref();
+ if ( m_data )
+ m_data->ref( this );
}
@@ -62,7 +64,7 @@ Nepomuk::Resource::Resource( const Nepom
{
m_data = res.m_data;
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -70,7 +72,7 @@ Nepomuk::Resource::Resource( const QStri
{
m_data = ResourceManager::instance()->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -78,7 +80,7 @@ Nepomuk::Resource::Resource( const QStri
{
m_data = manager->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -86,7 +88,7 @@ Nepomuk::Resource::Resource( const QStri
{
m_data = ResourceManager::instance()->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -94,7 +96,7 @@ Nepomuk::Resource::Resource( const QUrl&
{
m_data = ResourceManager::instance()->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -102,7 +104,7 @@ Nepomuk::Resource::Resource( const QUrl&
{
m_data = manager->d->data( uri, type );
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
@@ -110,39 +112,29 @@ Nepomuk::Resource::Resource( Nepomuk::Re
{
m_data = data;
if ( m_data )
- data->ref();
+ m_data->ref( this );
}
Nepomuk::Resource::~Resource()
{
if ( m_data ) {
- if ( !m_data->deref() ) {
- //
- // We delete data objects in one of three cases:
- // 1. They are not valid and as such not in one of the ResourceManagerPrivate kickoff lists
- // 2. They have a proxy which is the actual thing to reuse later on
- // 3. The cache is already full and we need to clean up
- //
- if ( !m_data->isValid() ||
- m_data->proxy() ||
- m_data->rm()->dataCacheFull() ) {
+ m_data->deref( this );
+ if ( m_data->rm()->shouldBeDeleted( m_data ) )
delete m_data;
}
- }
- }
}
Nepomuk::Resource& Nepomuk::Resource::operator=( const Resource& res )
{
if( m_data != res.m_data ) {
- if ( m_data && !m_data->deref() && !m_data->isValid() ) {
+ if ( m_data && !m_data->deref( this ) && m_data->rm()->shouldBeDeleted( m_data ) ) {
delete m_data;
}
m_data = res.m_data;
if ( m_data )
- m_data->ref();
+ m_data->ref( this );
}
return *this;
_______________________________________________
Nepomuk mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/nepomuk
