Review Request: Dolphin - optimize use of PlacesItemModel

[Dan Vrátil]

> On Nov. 2, 2012, 2:29 p.m., Frank Reininghaus wrote:
> > Thanks for the patch! The context menu is one of the areas that I haven't worked on much until now - I wasn't even aware that an operation as expensive as creating a PlacesItemModel is performed every time a context menu is requested for a directory.
> > 
> > However, I think that the context menu issue can be solved in a much easier way. If we want to determine if a directory is already present in the "Places", we are only interested in the "Places" in kfileplaces/bookmarks.xml, i.e., those Places that the user created or the predefined ones like "Home". The devices that Solid knows about are irrelevant in this context.
> > 
> > Therefore, one could just query the Places in kfileplaces/bookmarks.xml (possibly using a new static method in PlacesItemModel), rather than setting up a full PlacesItemModel, in DolphinContextMenu::placeExists(const KUrl& url).
> > 
> > I see that your proposal to add a PlacesCache might still have the benefit that Dolphin starts up faster (if the Places Panel is enabled) by making the Solid stuff asynchronous. I'm not really familiar with Solid, but I assume that the call that takes so long is Solid::Device::listFromQuery(m_predicate)? Wouldn't it be much better to resolve this problem in Solid and provide some API to get this information asynchronously? That would prevent that every app using Solid has to implement its own 'PlacesCache'.
> 
> Àlex Fiestas wrote:
>     kdelibs is frozen until Frameworks, we can't add async api :/
>     
>     libsolid2 will be totally async.
> 
> Frank Reininghaus wrote:
>     Hm, it seems that I haven't quite understood yet what exactly is frozen in kdelibs. If it is feature frozen, why could a new backend be added to Solid at all? And it's not exactly API frozen either. You can easily find examples for recent commits in kdelibs which added API or even new classes if it was considered necessary to fix a bug.
>     
>     I must admit that I don't know much about udisks/udisks2, but if the new backend really does cause trouble for applications (like make them block for a few seconds), I would consider anything that can help to resolve this a bug fix. Adding some way to access device info asynchronously in Solid (could be something similar to Dan's PlacesCache) makes IMHO more sense than requiring apps to add hacks to work around udisks2-related problems which result from the limitations of the synchronous API. It's not just Dolphin. The "open/save file" dialogs will suffer from the same problem, and probably some more applications.
>     
>     Is the new asynchronous API for libsolid2 already being worked on?
>     
>     @Dan: I didn't say at all in my first comment that I did have a close look at your code, and I see that you spent a lot of time on it and that it is of high quality. But it's based on the assumption that we do need a full PlacesItemModel for the context menu, which is wrong IMHO. I think that you should always talk to people *before* you start working on such a massive code change to see if there is an easier solution.

The asynchronous cache was made mainly to improve start up time (that's what users notice, so I think that's something worth spending time on).

The only fix regarding context menu is not to create a new model every time the context menu is created. I didn't realize that we don't need the Solid data for this, however constructing the model every time even to just read the bookmarks from hard disk is IMO still an unnecessary overhead. If you remove the cache-code from the patch, you get like 10 lines of code changed just to have the model owned by DolphinMainWidget and context menu using that - which I think is possibly the simplest solution :-)


- Dan


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


On Nov. 1, 2012, 1:13 p.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107168/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2012, 1:13 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> This patch optimizes PlacesItemModel in two places. The optimizations are needed when using udisks2 Solid backend (which will be shipped in KDE 4.10, but it's already used in KDE 4.9 at least in Fedora). udisks2 is a bit slower because it only can enumerate all devices (unlike udisks where you can ask for specific device) - and that takes time, especially when there are slow devices (cheap USB sticks, slow optical drive).
> 
> 1) Solid devices are stored in a static cache, which takes care that on startup Solid is only queried once (this eliminates CPU spikes by udisks2d daemon and limits DBus traffic). The Solid query is run asynchronously, which has a positive impact on Dolphin startup time.
> 
> 2) Don't create new PlacesItemModel on every context menu popup. This effectively eliminates ~4 seconds delay between right click and the context menu appearing on screen. The persistent model is owned by DolphinMainView.
> 
> 
> Diffs
> -----
> 
>   dolphin/src/CMakeLists.txt 8f7f4db 
>   dolphin/src/dolphincontextmenu.cpp bb26c7a 
>   dolphin/src/dolphinmainwindow.h ab79fb0 
>   dolphin/src/dolphinmainwindow.cpp babaf14 
>   dolphin/src/panels/places/placescache.h PRE-CREATION 
>   dolphin/src/panels/places/placescache.cpp PRE-CREATION 
>   dolphin/src/panels/places/placesitemmodel.h 463e564 
>   dolphin/src/panels/places/placesitemmodel.cpp 1789f18 
> 
> Diff: http://git.reviewboard.kde.org/r/107168/diff/
> 
> 
> Testing
> -------
> 
> Start up time and opening context menu is faster, no CPU spikes from udisks2d deamon
> 
> 
> Thanks,
> 
> Dan Vrátil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20121104/845f4c49/attachment.htm>