[Nepomuk] Nepomuk Core - Questions & Patches

[Sebastian Trüg]

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.

Cheers,
Sebastian

On 05/10/2010 02:00 PM, Vishesh Handa wrote:
> 
> On Sun, May 9, 2010 at 7:30 PM, Sebastian Trüg <trueg at kde.org
> <mailto:trueg at kde.org>> wrote:
> 
>     On 05/09/2010 02:37 PM, Vishesh Handa wrote:
>     > *4.* In *ResourceData::resetAll* - shouldn't *m_cacheDirty = false *be
>     > equal to true? Considering resetAll is called when the resource is
>     being
>     > Removed, it shouldn't make a any difference. Still, just to be on the
>     > safer side.
> 
>     This would not make much sense since the Resource becomes invalid and
>     has no URI to check for any data to load.
> 
>  
> Okay.
>  
> 
>     > *5. Error Checking: *There is considerable error checking in
>     > Nepomuk::ResouceFilterModel where /Soprano::Error/s are returned, but
>     > after that it all stops.
> 
>     This is a bad situation indeed. However, I am not sure how to fix that
>     at the moment. The only solution I see would be to emit signals from
>     ResourceManager reporting the problems - although that does not at all
>     fit with the rest of the sync api.
> 
> 
> How about for now just adding debug statements if something is wrong. At
> least one of the developers will notice. 
> 
>  
> 
>     > *6. Destructors: *Shouldn't ResourceManagerPrivate have a destructor
>     > where it clears out the remaining ResouceData in m_initializedData and
>     > other? I know ResourceData::resetAll removes them, but it feels wrong
>     > not to check them just in case.
> 
>     Maybe we should rather put a Q_ASSERT(m_initializedData.isEmpty())
>     in there.
> 
> 
> Please look at the patch. I have noticed that at times
> ResourceManagerPrivate::cleanUpCache doesn't clean up properly. More on
> that later.
> 
>     > *8.* *ResourceManager::allResourcesWithProperty*: They seem to be
>     using
>     > a QList internally and using the QList:contains() functions quite
>     > frequently. Considering QList isn't sorted, i think it would be quite
>     > costly -> O(n). How about using a QSet? The only problem is the added
>     > QSet::toList() in the end, which shouldn't be too slow. (Max
>     O(n)). I've
>     > provided a patch.
> 
>     please apply. :)
> 
> 
> Committed.
> 
> 9. *ResourceManagerPrivate::cleanUpCache()* : I don't get how this
> works, and for what it's worth it doesn't always work, and I have proof
> that it doesn't (backtrace + debug statements) Look at the added kcrash,
> it has debug statements initially and then the backtrace.
> 
> Try to bear with me while I write down a lengthy explanation.
> 
> RIP  = Resource which is a Proxy for other Resources.
> RHP = Resource which has a Proxy.
> RES = Normal Resource which isn't a RIP or RHP.
> 
> If you look at the source code cleanUpCache() -
> 
> QList<ResourceData*> rdl = m_uriKickoffData.values() +
> m_idKickoffData.values();
>     for( QList<ResourceData*>::iterator rdIt = rdl.begin();
>          rdIt != rdl.end(); ++rdIt ) {
>         ResourceData* data = *rdIt;
>         if ( !data->cnt() ) {
>             delete data;
>             if( num > 0 && --num == 0 )
>                 break;
>         }
>     }
> 
> when the loop is running thsese 2 cases can occur (there are many other
> cases, but I don't care about them) -
> 
> *1. RIP *followed by a *RHP* :
> Initially the RIP would come, and since its cnt() won't be 0, it would
> not get deleted. Then the RHP (who's proxy is the previous RIP) would
> come, and since its cnt() = 0, it should get deleted. As it's getting
> deleted ResourceData::resetAll() would be called.
> 
>  // reset proxy
>     bool hadProxy = false;
>     if( m_proxyData ) {
>         kDebug() << "Has proxy data : " << m_proxyData;
>         kDebug() << m_proxyData->cnt();
>         hadProxy = true;
>         if( !m_proxyData->deref() &&
>             rm()->dataCacheFull() ) {
>             kDebug() << "Deleting proxy data";
>             delete m_proxyData;
>         }
>         kDebug() << m_proxyData->cnt();
>         m_proxyData = 0;
>     }
>  
> (Please ignore the debug statements)
> 
> Here the RIP would be deref() and it should NOT get deleted as
> rm()->dataCacheFull() would most probably be false (Just assume it is.)
> The RHP would then get deleted and the RIP still exists, and will
> continue to exist even after cleanUpCache() has finished execution. Look
> at proof.
> 
> One way is to loop until m_uriKickoffData and m_idKickoffData are both
> empty.
> 
> *2. RHP *followed by a *RIP*:
> The RHP comes first, gets deleted and deletes the proxy (assume
> dataCacheFull() to be true). Then when the RIP comes along it would test
> itself with /!data->cnt()/, and since data doesn't point to anything
> valid anymore, BOOM!, segfault, and it should crash. Unfortunately I
> haven't been able to prove this!  
> 
> *Some other stuff :* (Complaints. Just ignore them!)
> 
> I find all this proxy stuff kinda confusing. I know it's super helpful,
> but it is confusing. I don't like the idea that every ResourceData first
> queries it's proxy, what if the proxy has been deleted? I know that
> can't happen because of the checks, but still. I wonder if QSharedData
> or something like that could be used. I'll try a couple of things, and
> come back later.
> 
> The ResouceData and ResourceMangerPrivate classes are tightly coupled,
> and it is difficult to track who's managing who's data. For example -
> ResourceManagerPrivate::data is responsible for adding ResourceData to
> the lists (m_initialized and all), however ResourceData:resetAll is
> responsible for removing them. It it wasn't for KDevelop's handy "Show
> uses" features, I would have been, well, frustrated.
> 
> I try to look for some kind of solution.
> 
> Sorry for the obscenely large mail, and part rant.
> 
> - Vishesh Handa 
> 
> 
> 
>