Hi Sebastian
I sat down and thought of possible solutions, and in the end I came up with
3. The first was the keep the proxy as the Resource and not ResourceData,
but this adds more problems than it solves. The second was for every
ResourceData to have a link to the Resource that it contains. I thought this
would solve all our problems. Then I realized that a ResourceData can be
contained by multiple Resources. And there came the third, your, solution -
On Tue, May 11, 2010 at 1:50 PM, Sebastian Trüg <[email protected]> wrote:
> One solution to the proxy situation would be to not use proxies but let
> ResourceData have backlinks to all its Resource instances. Then one
> ResourceData could be replaced with another like so:
>
> void ResourceData::replaceWith( ResourceData* other )
> {
> foreach( Resource* res, m_resourceWrappers ) {
> res->m_data = other;
> other->m_resourceWrappers.append(res);
> }
> m_resourceWrappers.clear();
> delete this;
> }
>
> m_resourceWrappers is a bad name though.
>
>
It seems to solves almost all our problems. Just so that everyone is on the
same page, I'm going to state all the problems -
1. Proxies are confusing, and clutter up the ResourceData code.
2. CleanUpCache currently does not handle proxies correctly (read prev
emails)
3. Reference counting is wacky with proxies - ResourceData's are reference
counted as long as they don't have a proxy, and after they have one, they
use the same reference count instead of using the proxies. I find this
confusing.
4. The Metadata mover service currently cleans up the cache on every move.
This coupled with the fact that Metadata mover currently tracks hidden files
( your .kde directory ) makes it clean up the cache very frequently. (Look
at the metadata mover code)
There may be more problems which I'm not aware of.
Okay, now please look at the patch provided. It's a huge change, and patch
is kinda messy. It will be improved. One minor thing -
*Reference Counting - *Every time a Resource is created it needs to do
reference counting and manage the ResourceData's m_resources list (I didn't
like the name m_resourceWrappers) Because of this m_resources is currently
public (temp). One possible solution is to handle the m_resources along with
ResourceData's ref() and deref() functions. This would be a lot more
intuitive.
Can/Should we remove reference counting via the AtomicInt variable entirely?
Reference counting can still be done via m_resources.size(). But then there
is the thing that all operations on an AtomicInt are guaranteed to be
atomic.
This patch doesn't solve problem 4.
*Other stuff not related to this proxy patch -*
*tools.h - *This has an overloaded function called
*convertResourceList*,* *which
in one case converts into into a ResourceList and in the other case converts
it from a ResourceList. Can we depreciate this function and add more
appropriate *convertToResourceList* and *convertFromResourceList* function?
*ResourceManagerPrivate - *This is a really stupid thing. The name
ResourceManagerPrivate is a little misleading. Couldn't we name is
ResourceDataManger instead? The class isn't accessible to the outside world,
and we wouldn't be invalidating any APIs.
*
Resource::operator== - *Shouldn't it check if the data Cache is full and
accordingly delete? Just like Resource's destructor.
*Resource Class -* This is another really really stupid thing, and probably
has no solution, but I'm still voicing it. The ResouceClass has too many
dependencies. By dependencies I mean ontologies. It cannot function without
these ontologies, and in a way that's the point. It provides convenience
functions for many of those ontologies, but it lands up creating
dependencies. I would typically want the core to be as isolated as possible
and then have helper classes add extra dependencies. That way it would be a
simple task to port it to another platform, say Meego, which doesn't
need/want all those ontologies.
This is just something we should think about.
*
Random Idea - *When clearing up the cache instead of just deleting the
unused ones, maybe we could delete them based on some timestamps. This
currently isn't required.
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,35 @@ 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() && !oldData->isValid() )
+ delete oldData;
+ oldData->m_resources.removeAll( res );
+
+ res->m_data = rd;
+
+ // vHanda : Can reference counting be eliminated totally?
+ // It could be replaced with m_resources.size()
+ rd->ref();
+ rd->m_resources.append( res );
}
+ delete this;
}
@@ -796,17 +759,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 +777,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: resourcedata.h
===================================================================
--- resourcedata.h (revision 1126675)
+++ resourcedata.h (working copy)
@@ -37,6 +37,7 @@
namespace Nepomuk {
+ class Resource;
class ResourceManagerPrivate;
class ResourceData
@@ -155,7 +156,8 @@ namespace Nepomuk {
ResourceManagerPrivate* rm() const { return m_rm; }
- ResourceData* proxy() const { return m_proxyData; }
+ /// Contains a list of resources which contain this ResourceData
+ QList<Resource *> m_resources;
private:
void loadType( const QUrl& type );
@@ -187,13 +189,7 @@ namespace Nepomuk {
QMutex m_determineUriMutex;
mutable QMutex m_modificationMutex;
- /**
- * 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.
- */
- 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,70 +47,90 @@
Nepomuk::Resource::Resource()
{
m_data = ResourceManager::instance()->d->data( QUrl(), QUrl() );
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
Nepomuk::Resource::Resource( ResourceManager* manager )
{
m_data = manager->d->data( QUrl(), QUrl() );
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
Nepomuk::Resource::Resource( const Nepomuk::Resource& res )
{
m_data = res.m_data;
- if ( m_data )
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
Nepomuk::Resource::Resource( const QString& uri, const QUrl& type )
{
m_data = ResourceManager::instance()->d->data( uri, type );
- if ( m_data )
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
Nepomuk::Resource::Resource( const QString& uri, const QUrl& type, ResourceManager* manager )
{
m_data = manager->d->data( uri, type );
- if ( m_data )
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
Nepomuk::Resource::Resource( const QString& uri, const QString& type )
{
m_data = ResourceManager::instance()->d->data( uri, type );
- if ( m_data )
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
Nepomuk::Resource::Resource( const QUrl& uri, const QUrl& type )
{
m_data = ResourceManager::instance()->d->data( uri, type );
- if ( m_data )
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
Nepomuk::Resource::Resource( const QUrl& uri, const QUrl& type, ResourceManager* manager )
{
m_data = manager->d->data( uri, type );
- if ( m_data )
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
Nepomuk::Resource::Resource( Nepomuk::ResourceData* data )
{
m_data = data;
- if ( m_data )
- data->ref();
+ if ( m_data ) {
+ m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
@@ -119,16 +139,17 @@ Nepomuk::Resource::~Resource()
if ( m_data ) {
if ( !m_data->deref() ) {
//
- // We delete data objects in one of three cases:
+ // 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. 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
+ // 2. The cache is already full and we need to clean up
//
if ( !m_data->isValid() ||
- m_data->proxy() ||
m_data->rm()->dataCacheFull() ) {
delete m_data;
}
+
+ // Remove it from the m_resources list from ResourceData
+ m_data->m_resources.removeAll( this );
}
}
}
@@ -141,8 +162,10 @@ Nepomuk::Resource& Nepomuk::Resource::op
delete m_data;
}
m_data = res.m_data;
- if ( m_data )
+ if ( m_data ) {
m_data->ref();
+ m_data->m_resources.push_back( this );
+ }
}
return *this;
_______________________________________________
Nepomuk mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/nepomuk
