[Kde-hardware-devel] Review Request 126101: allow loading backends in-process

[Sebastian Kügler]

> On Nov. 17, 2015, 9:55 p.m., Daniel Vrátil wrote:
> > Thanks a lot for the patch! I like the general approach and I just have two objections regarding the implementation: 
> > 
> > 1) I don't like the split of the ConfigOperations for In- and Out-Of-Process cases. Please just implement both ways in the existing Operation classes. The way you implemented it now requires that all code using Operations effectively needs to be ported to use the factory methods you added, which is not a desirable change. You can hide this equally well into the Operation implementations. (you can then ignore some of the review comments in the Operations below)
> > 
> > 2) BackendManager::loadBackend() vs. BackendManager::requestBackend() vs. BackendLoader::loadBackend() is confusing and all mixed up. What about moving the actual plugin loading code back to BackendLoader::loadBackend() and adding BackendLoader::unload() method to unload it when necessary? This way we can avoid having another QPluginLoader in BackendManager. Then BackendManager::loadBackend() can become BackendManager::requestInProcessBackend() and be nicely in pair with BackendManager::requestBackend(). It would make the API cleaner IMO and less mixed up.

First, thanks for the review. :)

As to 2), I've had a look into that, but I'm not convinced it's a good idea to move the plugin loading logic back into BackendLoader, mostly for practical reasons: BackendLoader is currently not used or built in the libkscreen binary, it's only in the backendlauncher binary. Moving this (static) method into BackendLoader means that we have to build the BackendLoader and the DBus adapter into the libkscreen library(while ironically the goal here is to avoid this logic with the in-process method).
On the other hand, we would save the QPluginLoader* in BackendManager, but in exchange we have to carry a BackendLoader instance, which we'd only need for that one method and the contained QPluginLoader*.
I think that's a bad trade-off and a net loss.

I'll have a look how I can make the loadBackend logic a bit clearer in the code, otherwise.

As to the other comments, they make sense to me. I'm working on it.


- Sebastian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126101/#review88502
-----------------------------------------------------------


On Nov. 17, 2015, 8:35 p.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126101/
> -----------------------------------------------------------
> 
> (Updated Nov. 17, 2015, 8:35 p.m.)
> 
> 
> Review request for Plasma, Solid, Aleix Pol Gonzalez, Daniel Vrátil, and Martin Gräßlin.
> 
> 
> Repository: libkscreen
> 
> 
> Description
> -------
> 
> This patchset adds in-process operation to libkscreen
> 
> purpose:
> - allow easier debugging
> - for some backends (qscreen, upcoming kwayland) the out of process operation is not necessary since these backends are well-shielded
> 
> This implementation is backwards compatible and should mean only minimal changes to running setups.
> 
> If the user exports KSCREEN_BACKEND_INPROCESS=1 before running, all operations will be done in process. Otherwise, the out-of-process mode is used.
> 
> The idea is that we use 
> The changes in the clients to use the in-process mode are to use ConfigOperation::create() and ConfigOperation::setOperation() to retrieve the get or set config jobs. The rest will be handled inside libkscreen.
> 
> Autotests should cover all the cases (and actually a few currently unsupported ones, such as using different backends in the same process).
> 
> Details on performance, etc.: http://vizzzion.org/blog/2015/11/wayland-and-libkscreen-benchmarks/
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 86a0965 
>   autotests/CMakeLists.txt 69af7f0 
>   autotests/testconfigmonitor.cpp a051226 
>   autotests/testinprocess.cpp PRE-CREATION 
>   autotests/testqscreenbackend.cpp da4dbae 
>   autotests/testscreenconfig.cpp ecbcedf 
>   backends/fake/fake.cpp 60264dd 
>   src/CMakeLists.txt 4b56b61 
>   src/backendlauncher/backendloader.cpp 52051df 
>   src/backendmanager.cpp ca9c746 
>   src/backendmanager_p.h c6418e2 
>   src/config.cpp 75d947d 
>   src/configmonitor.h b6f1189 
>   src/configmonitor.cpp a14bc70 
>   src/configoperation.h 2405d79 
>   src/configoperation.cpp 87fe141 
>   src/getconfigoperation.h c85bfaa 
>   src/inprocessconfigoperation.h PRE-CREATION 
>   src/inprocessconfigoperation.cpp PRE-CREATION 
>   src/output.cpp bd381fa 
>   src/setconfigoperation.cpp 6ea944f 
>   src/setinprocessoperation.h PRE-CREATION 
>   src/setinprocessoperation.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/126101/diff/
> 
> 
> Testing
> -------
> 
> Added a ton of autotests, made sure all existing ones pass.
> 
> tried "KSCREEN_BACKEND_INPROCESS=1 kcmshell5 kscreen", all working as expected.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-hardware-devel/attachments/20151117/ed0831fc/attachment.html>