[prev in list] [next in list] [prev in thread] [next in thread]
List: kfm-devel
Subject: Re: Review Request: Dolphin - optimize use of PlacesItemModel
From: Àlex_Fiestas <afiestas () kde ! org>
Date: 2012-11-08 9:32:41
Message-ID: 20121108093241.14560.60306 () vidsolbach ! de
[Download RAW message or body]
> 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 expe=
nsive 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 ea=
sier 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.x=
ml, i.e., those Places that the user created or the predefined ones like "H=
ome". 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 setti=
ng up a full PlacesItemModel, in DolphinContextMenu::placeExists(const KUrl=
& url).
> > =
> > I see that your proposal to add a PlacesCache might still have the bene=
fit that Dolphin starts up faster (if the Places Panel is enabled) by makin=
g the Solid stuff asynchronous. I'm not really familiar with Solid, but I a=
ssume that the call that takes so long is Solid::Device::listFromQuery(m_pr=
edicate)? Wouldn't it be much better to resolve this problem in Solid and p=
rovide some API to get this information asynchronously? That would prevent =
that every app using Solid has to implement its own 'PlacesCache'.
> =
> =C3=80lex 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 froz=
en 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 bloc=
k for a few seconds), I would consider anything that can help to resolve th=
is 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 th=
an requiring apps to add hacks to work around udisks2-related problems whic=
h 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 proba=
bly 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 i=
t is of high quality. But it's based on the assumption that we do need a fu=
ll 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 massi=
ve code change to see if there is an easier solution.
> =
> Dan Vr=C3=A1til 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 ever=
y 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 jus=
t read the bookmarks from hard disk is IMO still an unnecessary overhead. I=
f you remove the cache-code from the patch, you get like 10 lines of code c=
hanged just to have the model owned by DolphinMainWidget and context menu u=
sing 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 sugges=
ting 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 mode=
l between the context menu and the Places Panel somehow?
> =
> I agree that reusing data if the context menu is opened multiple time=
s 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 fil=
e. How this is implemented is a different question.
> =
> I still think that the 'asynchronous cache' idea is good, but as I sa=
id, I believe that it's wrong to have every app using Solid implement its o=
wn cache.
> =
> Dan Vr=C3=A1til wrote:
> I didn't find a way how to reach DolphinMainWindow from the Places Pa=
nel, 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 t=
he panel and the context menu using one shared PlacesItemModel. In order no=
t to penalize users without Places Panel, the model in DolphinMainWindow co=
uld be created the first time someone asks for it, rather then in DolphinMa=
inWindow 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 :-(
> =
> Frank Reininghaus wrote:
> > 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 ma=
in window accessible from the panel, considering that the context menu does=
n'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 wo=
uld 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, w=
e should only consider making the code more complex to get rid of this over=
head if there really is a delay that is noticeable by the user if the conte=
xt menu is opened.
> =
> > I agree that the problem should be fixed in Solid, but as long as A=
PI is frozen, I'm afraid there's little we can do about it. We can only wai=
t 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/e2=
e6e5ecb607b545a9154d368aba8f60776dd777
> =
> which add new API. And as I said, Solid's new udisks2 backend even lo=
oks like a new feature to my uninformed eye.
> =
> =C3=80lex Fiestas wrote:
> I wonder if this, would apply to KFilePlacesView since the code in do=
lphin is an improved/copypaste from it, Dan can you check? is performance a=
n issue with udisk2 and KFIlePlacesView? (you can check that using kdialog,=
or any open/close dialog)
kdelibs is frozen, that's the statement of the people working hard on KDE F=
rameworks and everybody else adding or doing things in kdelibs other than f=
ixing bugs are not doing a favor to those people.
About udisk2, what happen is that Luka worte it in KDE-Frameworks and distr=
ibutions started to pick it up from there, messing the code and doing nast=
y things, so I guess that he asked permission to merge it to kdelibs to avo=
id that.
Definitively libsolid2 is KDE Frameworks material.
- =C3=80lex
-----------------------------------------------------------
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=C3=A1til 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 sl=
ower 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 s=
tartup Solid is only queried once (this eliminates CPU spikes by udisks2d d=
aemon and limits DBus traffic). The Solid query is run asynchronously, whic=
h has a positive impact on Dolphin startup time.
> =
> 2) Don't create new PlacesItemModel on every context menu popup. This eff=
ectively eliminates ~4 seconds delay between right click and the context me=
nu 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 udis=
ks2d deamon
> =
> =
> Thanks,
> =
> Dan Vr=C3=A1til
> =
>
[Attachment #3 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107168/">http://git.reviewboard.kde.org/r/107168/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 2nd, 2012, 2:29 p.m., <b>Frank \
Reininghaus</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px \
solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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'.</pre> </blockquote>
<p>On November 3rd, 2012, 5:32 p.m., <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">kdelibs is frozen until \
Frameworks, we can't add async api :/
libsolid2 will be totally async.</pre>
</blockquote>
<p>On November 3rd, 2012, 6:24 p.m., <b>Frank Reininghaus</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre> </blockquote>
<p>On November 4th, 2012, 4:19 p.m., <b>Dan Vrátil</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 :-)</pre> </blockquote>
<p>On November 5th, 2012, 9:43 p.m., <b>Frank Reininghaus</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre> </blockquote>
<p>On November 6th, 2012, 12:32 p.m., <b>Dan Vrátil</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 \
:-(</pre> </blockquote>
<p>On November 7th, 2012, 8:02 p.m., <b>Frank Reininghaus</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre> </blockquote>
<p>On November 8th, 2012, 9:30 a.m., <b>Àlex Fiestas</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I wonder if this, would \
apply to KFilePlacesView since the code in dolphin is an improved/copypaste from it, \
Dan can you check? is performance an issue with udisk2 and KFIlePlacesView? (you can \
check that using kdialog, or any open/close dialog)</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">kdelibs is frozen, \
that's the statement of the people working hard on KDE Frameworks and everybody \
else adding or doing things in kdelibs other than fixing bugs are not doing a favor \
to those people.
About udisk2, what happen is that Luka worte it in KDE-Frameworks and distributions \
started to pick it up from there, messing the code and doing nasty things, so I \
guess that he asked permission to merge it to kdelibs to avoid that.
Definitively libsolid2 is KDE Frameworks material.</pre>
<br />
<p>- Àlex</p>
<br />
<p>On November 6th, 2012, 12:32 p.m., Dan Vrátil wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for Dolphin and Frank Reininghaus.</div>
<div>By Dan Vrátil.</div>
<p style="color: grey;"><i>Updated Nov. 6, 2012, 12:32 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">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.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">Start up time and opening context menu is faster, no CPU spikes from \
udisks2d deamon</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>dolphin/src/CMakeLists.txt <span style="color: grey">(8f7f4db)</span></li>
<li>dolphin/src/dolphincontextmenu.cpp <span style="color: \
grey">(bb26c7a)</span></li>
<li>dolphin/src/dolphinmainwindow.h <span style="color: grey">(ab79fb0)</span></li>
<li>dolphin/src/dolphinmainwindow.cpp <span style="color: \
grey">(babaf14)</span></li>
<li>dolphin/src/panels/places/placescache.h <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>dolphin/src/panels/places/placescache.cpp <span style="color: \
grey">(PRE-CREATION)</span></li>
<li>dolphin/src/panels/places/placesitemmodel.h <span style="color: \
grey">(463e564)</span></li>
<li>dolphin/src/panels/places/placesitemmodel.cpp <span style="color: \
grey">(1789f18)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107168/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic