Review Request: Delay initialization of PowerDevil and DeviceAutomounter

[Sebastian Sauer]

> On 2010-03-22 09:28:26, Lubos Lunak wrote:
> > Why don't you change X-KDE-Kded-phase=1 to 2 instead of this?
> >
> 
> Sebastian Sauer wrote:
>     because;
>     1. they are not the same
>     2. the one does not exclude the other
>     
>     So, you suggest to additionally change them both from X-KDE-Kded-phase=1 to X-KDE-Kded-phase=2? Sounds good for me :)
>
> 
> Lubos Lunak wrote:
>     I'm not familiar with the modules enough to suggest, I'm asking. If you change the phase to 2, your change is needless, as such modules are loaded late enough to for delayed class initialization to not matter, and if the modules would be needed sooner, they'd be loaded and initialized on-demand. So my question was if it really is necessary for the modules to be loaded in phase 1 (and, also, are you sure it doesn't just make it seemingly faster by making phase 1 loading faster but only moving the work done to a different time?).
>
> 
> Sebastian Sauer wrote:
>     > If you change the phase to 2, your change is needless
>     
>     No, it is not. KDED will block till those modules are loaded what includes calling the ctors. This is the case in any hase without any exception. The patch does decrease that wait-time by close to a second and does therefore decrease the time needed to pass the phase the modules are in.
>     
>     An exception are modules loaded on-demand (which none of the both are btw) cause they are not loaded together with n other modules which are all together blocking KDED and with it the whole startup process (ok, except the loading-on-demand happens on startup). But even in the load-on-demand case it would make sense to apply that patch cause;
>     
>     1. both modules are not expected to be 'direct available' on calling like other modules but are only expected to be 'running now'. This means that we do not have to wait till the modules are fully setup.
>     2. we do not block whole KDED any longer for unrelated setup work. The only important aspect here is that the both modules are loaded.
>     
>     The important part here is that I did not pick this both modules only cause they are started so early compared to other modules but also cause each of those both modules (especially powerdevil) needs *way* longer then any other module that is loaded here per default. To compare none of the other modules comes over 0.07 seconds (what doesn't mean that cannot be the reason for other bottlenecks not found/investigated yet).
>     
>     > if the modules would be needed sooner, they'd be loaded and initialized on-demand.
>     
>     I don't think that loading-on-demand is a real option for Powerdevil and DeviceAutomounter but I see what you mean. My idea here was that the reason those both are started so early is that they should be up and running before the other modules. But I agree that there is probably no reason for this. Maybe someone should really investigate this? For my goal the 0.1 seconds to save here (that is the remaining time the both are needing now to be loaded and the total time KDED spends in phase1 now compared to the ~seconds it was needing before) are not that important cause I am more looking for seconds to save and there are still lot of areas where we can get them from (e.g. starting Plasma and KWin in parallel would bring us up to 3 seconds).
>     
>     > are you sure it doesn't just make it seemingly faster by making phase 1 loading faster but only moving the work done to a different time?
>     
>     yes, I am testing the overall startup time and that's how I identified those both modules as optimization candidates that are worth to be improved to enhance the total outcome.
> 
> Lubos Lunak wrote:
>     > 2. we do not block whole KDED any longer for unrelated setup work.
>     
>     But that is the point of phase 1 kded modules. They are loaded and fully initialized before following startup handling is done. Let me say what I said differently: Either the modules don't need to be loaded before all the other things that follow kded phase 1 - then it's just phase 2 and your change doesn't make a difference. Or it is needed for the modules to be loaded soon enough in the startup, and then your change breaks it.
>     
>     > The only important aspect here is that the both modules are loaded.
>     
>     Why? What is the point of a loaded module that is not initialized? Either it needs to be ready soon in the startup process, then it should be completely ready, or it doesn't need to be, and then just phase 2 should be enough.
>     
>
> 
> Sebastian Sauer wrote:
>     Then maybe just change them to phase2?
>

re this patch and

> then it's just phase 2 and your change doesn't make a difference

If I change them to phase=2 then Kded::loadSecondPhase() is left one second earlier with this patch. The saved time stays the same but phase=2 would be even better.


- Sebastian


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


On 2010-03-21 17:49:17, Sebastian Sauer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/3331/
> -----------------------------------------------------------
> 
> (Updated 2010-03-21 17:49:17)
> 
> 
> Review request for kdelibs and Dario Freddi.
> 
> 
> Summary
> -------
> 
> The patch delays the initialization of PowerDevil and DeviceAutomounter and improves that way KDED startup phase 1 by close to a second (out of 15, coldstart).
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdebase/runtime/solid-device-automounter/kded/DeviceAutomounter.h 1105732 
>   trunk/KDE/kdebase/runtime/solid-device-automounter/kded/DeviceAutomounter.cpp 1105732 
>   trunk/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.h 1105732 
>   trunk/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.cpp 1105732 
> 
> Diff: http://reviewboard.kde.org/r/3331/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sebastian
> 
>