Review Request: Load Solid devices in KFilePlacesModel asynchronously

[Dan Vrátil]

> On Nov. 29, 2012, 10:27 p.m., Mark Gaiser wrote:
> > Ehm, this just seems odd to me. Because kdelibs is frozen you add a completely new class and public even. Thus you already change the kdelibs api which should not be allowed.
> > 
> > Second argument, if this udisks2 is so important to have, why don't we just make an exception to the frozen api and allow some changes in. I'm sure the kde folks couldn't have known that udisks2 was going to be widely used within the "kdelibs is frozen, bug fixes only" timeline. This is just out of the kdelibs hands. Again, an exception would better.
> > 
> > Third argument, adding this as public API and wanting other applications to use this till frameworks is done will just add a lot more work on the app side. First to change the current stuff and add the cache class then later on to revert to how the code looks now because that class is temporary and won't be in frameworks. Really, seriously? 
> > 
> > Why is udisks2 slower anyway?

Well this is the exception I'm trying to get in :) If you are thinking about a long-term solution, that would be Solid2 with completely asynchronous API. That would be a very cool thing to have, but I'm afraid pretty much impossible to design, write, test and port all existing code to within less than three weeks.

Thanks to the cache API being based on Solid::DeviceNotifier API, porting to it requires changing 3 lines of code (so no major rewrites there). If we come with Solid2 for Frameworks, all of the Solid related code will have to be revisited and ported anyway, so it does not matter that you would have to revert this patch, just call it Solid->Solid2 transition.

udisks2 is slower, because first you need to enumerate all devices (via DBus), filter them to get the ones you are interested in and then retrieve all properties (again via DBus) of each device you want to use (the list can be quite large, it's cached for later use though). When building the devices model however, the amount of data() calls is very high and somewhat overwhelms DBus and the udisks daemon. udisks1 had different API which required less traffic to get all the info you needed (Lukas could explain better) so there were no problems with this. Because of Solid's synchronous API, the Udisks2 backend has to use QDBusPendingCall::waitForReply() which I think blocks any other DBus traffic in the main thread until the blocking call is finished, which impacts speed notably (especially in apps trying to use DBus for other things too).


- Dan


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


On Nov. 29, 2012, 2:55 p.m., Dan Vrátil wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107513/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2012, 2:55 p.m.)
> 
> 
> Review request for kdelibs, Lukáš Tinkl and David Faure.
> 
> 
> Description
> -------
> 
> With KDE 4.10 Solid will provide udisks2 backend. Unfortunately udisks2 is somewhat slower then udisks1 and listing all the devices when starting Gwenview or file dialog takes more time then is acceptable for users. I only have a single optical drive and one HDD with 4 partitions and it takes ~2 seconds for the file dialog to appear and udisks2d causes high CPU spikes during that. And it gets much worse with slow, cheap USB sticks etc.
> 
> This patch adds an asynchronous cache, which will call Solid::Devices::listFromQuery() in a thread and then notify listeners through deviceAdded() signal about all the devices received. The cache keeps itself up-to-date (via Solid::DeviceNotifier) and notifies listeners about these changes via deviceAdded() and deviceRemoved() signals. This patch also ports the KFilePlacesModel to use the cache which results in KFileDialog appearing immediately. Gwenview also starts faster.
> 
> The KFilePlacesDeviceCache class is exported as public because of Dolphin, which has it's own implementation of places model similar to KFilePlacesModel and which suffers from the same problems (and possibly there are other apps). We have already verified that porting Dolphin's model to this cache resolves the issue. Better solution would be for libsolid to have asynchronous API, however that's not possible now with kdelibs being feature-frozen. With libsolid2, this class can go away. Meanwhile however it's the only way to fix this performance regression that will hit every distro that will decide to ditch udisks1 with KDE 4.10 and switch to udisks2.
> 
> 
> Diffs
> -----
> 
>   kfile/CMakeLists.txt ceae140 
>   kfile/kfileplacesdevicecache.h PRE-CREATION 
>   kfile/kfileplacesdevicecache.cpp PRE-CREATION 
>   kfile/kfileplacesmodel.cpp 0192926 
> 
> Diff: http://git.reviewboard.kde.org/r/107513/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Dan Vrátil
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20121130/8207cb53/attachment.htm>