[Nepomuk] Review Request 109747: ResourceManager cleanup merge

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109747/#review29917
-----------------------------------------------------------


Thanks for picking this up again, I like where it's going overall. Found one refactoring bug though, plus I have a few questions.


libnepomukcore/resource/resourcedata.cpp
<http://git.reviewboard.kde.org/r/109747/#comment22252>

    Is there a risk another thread could modify m_uri at this point? Or are we sure it's never empty when this is called?



libnepomukcore/resource/resourcedata.cpp
<http://git.reviewboard.kde.org/r/109747/#comment22253>

    This uses m_cache.value() outside of the m_dataMutex lock.
    Plus, at that point, m_cache.value(uri) is always 'value', isn't it? Should this use an "oldValue" local variable instead, set before m_cache is updated, and from within the lock?



libnepomukcore/resource/resourcedata.cpp
<http://git.reviewboard.kde.org/r/109747/#comment22254>

    (this one looks correct, for instance)



libnepomukcore/resource/resourcemanager.cpp
<http://git.reviewboard.kde.org/r/109747/#comment22255>

    What do you mean by "take" here? Lock? Surely if the calling function was locking the rm mutex before calling addToWatcher, this line would deadlock... Not sure what this comment means.


- David Faure


On March 26, 2013, 7:54 p.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109747/
> -----------------------------------------------------------
> 
> (Updated March 26, 2013, 7:54 p.m.)
> 
> 
> Review request for Nepomuk, David Faure and Vishesh Handa.
> 
> 
> Description
> -------
> 
> Am I ok to merge this to KDE 4.10? I would like to merge the actual bugfixes, 
> but if you want I could omit the performance fixes/cleanups, which are
> 1e76eedb82ee363c9caf333fe221962db81d67c6  75028c00ec71ccd67a046c0d71072623c2e5af74  464590095981e7ded537901d56f84ca49aa49d74
> 
> . 
> 
> Thanks
> 
> commit dbbea65fb00c14852034be867396aa055041d848
> Author: Simeon Bird <bladud at gmail.com>
> Date:   Sat Mar 16 19:11:19 2013 -0400
> 
>     ResourceWatcher: switch to using KDbusConnectionPool
> 
> commit 1e76eedb82ee363c9caf333fe221962db81d67c6
> Author: Simeon Bird <bladud at gmail.com>
> Date:   Thu Mar 14 23:17:31 2013 -0400
> 
>     Tidy up ResourceData::propertyAdded.
> 
>     If an entry is not found in a QHash, it will return an empty list,
>     so we only need to check for contains one way.
> 
> commit 75028c00ec71ccd67a046c0d71072623c2e5af74
> Author: David Faure <faure at kde.org>
> Date:   Thu Mar 14 14:27:47 2013 +0100
> 
>     Rework determineUri so that the RM lock isn't held during the executeQuery
> 
> commit cc8a925989193b813dc50090c8b4a05e46fbfbdc
> Author: David Faure <faure at kde.org>
> Date:   Thu Mar 14 14:26:45 2013 +0100
> 
>     fix lock order in resetAll
> 
> commit aec508539182f7bed2819cd23d82ba9baa120c8c
> Author: David Faure <faure at kde.org>
> Date:   Thu Mar 14 13:23:22 2013 +0100
> 
>     early return if nothing to do (noop, just makes the code clearer)
> 
> commit 877b40f1916a64916d0869be5744dbc525931edd
> Author: Simeon Bird <bladud at gmail.com>
> Date:   Sat Mar 16 16:27:44 2013 -0400
> 
>     Fix bad threading in ResourceManagerPrivate::addToWatcher dbus usage.
> 
>     Instead of doing MoveToThread, take the resourceManager mutex before
>     talking to the ResourceWatcher. This is a lot clearer and less racy.
>     It should not be too contended now the rm mutex is not held over the
>     socket communication.
> 
> commit 464590095981e7ded537901d56f84ca49aa49d74
> Author: Simeon Bird <bladud at gmail.com>
> Date:   Fri Mar 8 22:08:17 2013 -0500
> 
>     ResourceData: Change updateKickOffLists to take three arguments, the
>     uri, old and new values.
> 
>     This means that it does not need to be under the dataMutex anymore,
>     which in turn means that we no longer need to remember to lock the RM
>     mutex before calling it, just to remember to unlock the dataMutex.
>     This in turn means that we can add a property that isn't NIE::url() or
>     NAO::identifier to the Resource without taking the mutex at all.
> 
> commit 1b82506d609866e87b9d49b47de473766b01f3d6
> Author: Simeon Bird <bladud at gmail.com>
> Date:   Sat Mar 9 03:25:09 2013 -0500
> 
>     Remove unneeded parameter from ResourceData::resetAll
> 
> 
> This addresses bug 305024.
>     http://bugs.kde.org/show_bug.cgi?id=305024
> 
> 
> Diffs
> -----
> 
>   libnepomukcore/datamanagement/resourcewatcher.cpp f394ae8705fa882b9f4329f93ea7d18c1cfabc73 
>   libnepomukcore/resource/resource.cpp 7158802cf1e1ba48da86f103aa9311c7acbe24fe 
>   libnepomukcore/resource/resourcedata.h 39508764682b798290c1ae5b74a2384f9cbcbf58 
>   libnepomukcore/resource/resourcedata.cpp 7a979745ca22c88a3a73921d9c0e64b51e064c38 
>   libnepomukcore/resource/resourcemanager.cpp 2b32be01176059932cc1c2cf3b1f348ed91e982b 
> 
> Diff: http://git.reviewboard.kde.org/r/109747/diff/
> 
> 
> Testing
> -------
> 
> Compiled, ran, used activities for a while.
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20130326/9db01521/attachment-0001.html>