Review Request: Dolphin - optimize use of PlacesItemModel

[Frank Reininghaus]

> 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.
> 
> Dan Vrátil wrote:
>     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 :-)
> 
> Frank Reininghaus wrote:
>     I agree that removing unnecessary overhead is a good idea :-) However, I'm not sure if I understand what exactly you propose now. Are you suggesting to construct a PlacesItemModel in DolphinMainWindow on startup and use that for the context menu, and change nothing else (which would mean that the Places Panel still has its own model)? Or do you want to share the model between the context menu and the Places Panel somehow?
>     
>     I agree that reusing data if the context menu is opened multiple times is good, but I would like to prevent that users who do not use the Places Panel at all suffer from any overhead on startup. Therefore, I still think that the context menu should only access the kfileplaces/bookmarks.xml file. How this is implemented is a different question.
>     
>     I still think that the 'asynchronous cache' idea is good, but as I said, I believe that it's wrong to have every app using Solid implement its own cache.
> 
> Dan Vrátil wrote:
>     I didn't find a way how to reach DolphinMainWindow from the Places Panel, so the PlacesItemModel owned by DolphinMainWindow is used only by the context menu, the Places panel has it's own model.
>     
>     If there is a way how to reach DMW from Places Panel, we could have the panel and the context menu using one shared PlacesItemModel. In order not to penalize users without Places Panel, the model in DolphinMainWindow could be created the first time someone asks for it, rather then in DolphinMainWindow constructor.
>     
>     I agree that the problem should be fixed in Solid, but as long as API is frozen, I'm afraid there's little we can do about it. We can only wait for libsolid2 :-(

> I didn't find a way how to reach DolphinMainWindow from the Places Panel

Yes, that's right. And I would prefer not to add hacks to make the main window accessible from the panel, considering that the context menu doesn't need a full PlacesItemModel at all.

I would really prefer to just have DolphinContextMenu interact with a KBookMarkManager that works on the file kfileplaces/bookmarks.xml. That would be quite straightforward to do, I think: to check if a 'Place' exists, we would initialise the KBookMarkManager and just loop over the KBookmarks and check the URL of each of them.
 
Yes, this would happen every time the context menu is opened. IMHO, we should only consider making the code more complex to get rid of this overhead if there really is a delay that is noticeable by the user if the context menu is opened.

> I agree that the problem should be fixed in Solid, but as long as API is frozen, I'm afraid there's little we can do about it. We can only wait for libsolid2 :-(

No, the kdelibs API is not frozen. You can easily find recent commits like

https://projects.kde.org/projects/kde/kdelibs/repository/revisions/e2e6e5ecb607b545a9154d368aba8f60776dd777

which add new API. And as I said, Solid's new udisks2 backend even looks like a new feature to my uninformed eye.


- Frank


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


On Nov. 6, 2012, 12:32 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. 6, 2012, 12:32 p.m.)
> 
> 
> Review request for Dolphin and Frank Reininghaus.
> 
> 
> 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/20121107/839746c5/attachment.htm>