Re: [RFC] [kservice] KPluginMetadata indexing

[Mark Gaiser]

On Thu, Nov 6, 2014 at 3:44 AM, Sebastian Kügler  wrote:
> Hi all,  especially Alex and David,
>
> tl;dr:
> I've done a proof-of-concept implementation of a metadata index for
> KPluginTrader::query(), the main entry point when it comes to finding binary
> plugins. This index considerably speeds up all current use cases, but comes at
> the cost of having to maintain the index. Code is in
> kservice[sebas/kpluginindex], speeds up plugin quering a few times.
>
> The Slightly Longer Story...
>
> During Akademy's frameworks and plasma bofs, we talked about indexing plugins
> for faster lookups. One of the things we wanted to try in Plasma is to index
> packages, and thereby speeding up package metadata lookups and plugin queries.
>
> I have done a naive implementation of such an indexing mechanism, and have
> implemented this as a proof of concept in KService, specifically in
> KPluginTrader::query(). This is using Alex Richardson's recent work on
> KPluginMetadata, which I found very useful (
> https://git.reviewboard.kde.org/r/120198/ and
> https://git.reviewboard.kde.org/r/120199/ ). I've put these patches in my
> branch kservice[sebas/kpluginindex].
>
> Basic Mechanism
>
> - a small tool called kplugin-update-index collects the json metadata from the
> plugins, and puts the list of plugins in a given plugin directory into a
> QJsonArray, and dumps that in Qt's json binary format to disk
> - KPluginTrader::query checks if an index file exists in a given plugin
> directory
> -- if the index file exists, it reads it and creates a list of KPluginMetaData
> objects from it
> -- if the index file doesn't exist, it walks over each plugin to read its
> metadata, it basically falls back to the old code path
>
> Performance Measurement Method
>
> I've created a new autotest, kpluginmetadatatest, which runs two subsequent
> queries and measure the time it takes to return the results. I've instrumented
> the code in kplugintrader.cpp with QElapsedTimers. The autotest runs on an
> environment on rotation metal and ssd in separate test runs. Before cold cache
> tests, I've dropped page cache, dentries and inodes from memory using
> echo 3 > /proc/sys/vm/drop_caches
> Tests are running on Qt's 5.4 branch, they're fairly consistent with what I've
> seen on Qt 5.3.
>
> Performance Improvements
>
> Performance tests are promising:
> http://vizzzion.org/blog/wp-content/uploads/2014/11/performance-comparison-charts.png
>  (note that the metal's left-most bar is truncated by /10 in the
> picture).
>
> In short, the indexed queries are roughly:
> * 60 times faster on a rotational medium with cold caches
> * 3 times faster on an SSD with cold caches
> * 7 times faster on  a rotational disk with warm caches
> * 5 times faster on a SSD with warm caches
>
> More Observations
> - on ssds, we save most of the time in directory traversal and (de)serializing
> the json metadata
> - the index lookups spends almost all of its time in disk reads, deserializing
> the binary metadata is almost free (Qt's json binary representation is really
> fast to read)
> - I haven't seen any tests in which the indexed queries have been slower.
>
> These results can be explained as follows:
> - the bottleneck is reading the files from disk
> - on rotational media, expectedly we get huge performance penalties for every
> seek we cause, the more files we read, the more desastrous lookups times get.
> - Expectedly, warm pagecaches help a lot in all cases
>
> Cost: Maintaining the Cache
>
> These speedups do come at a cost, of course, and that is the added complexity
> of maintaining the caches. The idea from the bof sessions had been to update
> the caches at install time, this is essentially what can be done with kplugin-
> update-index (it needs some added logic to give the index files sensible
> permissions when run as root). That means that packagers will have to run the
> index updater in their postinstall routine. Not doing this at all means slower
> queries (or rather, no speedier queries), worse is if they forget to update
> once in a while, in which case newly installed or removed plugins might be
> missing or dangling in the index files. This will need at least some packaging
> discipline.
>
> Index File Location
>
> The indexer creates the index files in the plugin directories itself, not in
> $CACHE or $TMP. This seems the most straight-forward way to do it, since if a
> plugin is installed into a specific directory, the "installer" will have write
> permission there to update the index as well. One might consider putting these
> index files in the cache directory, like ksycoca does, but in that case, we
> need to be smarter to actually update the index files correctly, since at that
> point, it depends on the environment of the user and the plugin paths (which
> means, it can't sensibly be done at install-time).
>
> KServiceTyperTrader Comparison
>
> First off, for the current situation, the comparison to KServiceTypeTrader is
> not of much use

Re: [RFC] [kservice] KPluginMetadata indexing

[Milian Wolff]

On Thursday 06 November 2014 10:09:51 Mark Gaiser wrote:
> On Thu, Nov 6, 2014 at 3:44 AM, Sebastian Kügler  wrote:
> > Hi all,  especially Alex and David,
> > 
> > tl;dr:
> > I've done a proof-of-concept implementation of a metadata index for
> > KPluginTrader::query(), the main entry point when it comes to finding
> > binary plugins. This index considerably speeds up all current use cases,
> > but comes at the cost of having to maintain the index. Code is in
> > kservice[sebas/kpluginindex], speeds up plugin quering a few times.
> > 
> > The Slightly Longer Story...
> > 
> > During Akademy's frameworks and plasma bofs, we talked about indexing
> > plugins for faster lookups. One of the things we wanted to try in Plasma
> > is to index packages, and thereby speeding up package metadata lookups
> > and plugin queries.
> > 
> > I have done a naive implementation of such an indexing mechanism, and have
> > implemented this as a proof of concept in KService, specifically in
> > KPluginTrader::query(). This is using Alex Richardson's recent work on
> > KPluginMetadata, which I found very useful (
> > https://git.reviewboard.kde.org/r/120198/ and
> > https://git.reviewboard.kde.org/r/120199/ ). I've put these patches in my
> > branch kservice[sebas/kpluginindex].
> > 
> > Basic Mechanism
> > 
> > - a small tool called kplugin-update-index collects the json metadata from
> > the plugins, and puts the list of plugins in a given plugin directory
> > into a QJsonArray, and dumps that in Qt's json binary format to disk
> > - KPluginTrader::query checks if an index file exists in a given plugin
> > directory
> > -- if the index file exists, it reads it and creates a list of
> > KPluginMetaData objects from it
> > -- if the index file doesn't exist, it walks over each plugin to read its
> > metadata, it basically falls back to the old code path
> > 
> > Performance Measurement Method
> > 
> > I've created a new autotest, kpluginmetadatatest, which runs two
> > subsequent
> > queries and measure the time it takes to return the results. I've
> > instrumented the code in kplugintrader.cpp with QElapsedTimers. The
> > autotest runs on an environment on rotation metal and ssd in separate
> > test runs. Before cold cache tests, I've dropped page cache, dentries and
> > inodes from memory using echo 3 > /proc/sys/vm/drop_caches
> > Tests are running on Qt's 5.4 branch, they're fairly consistent with what
> > I've seen on Qt 5.3.
> > 
> > Performance Improvements
> > 
> > Performance tests are promising:
> > http://vizzzion.org/blog/wp-content/uploads/2014/11/performance-comparison
> > -charts.png (note that the metal's left-most bar is truncated by /10 in
> > the picture).
> > 
> > In short, the indexed queries are roughly:
> > * 60 times faster on a rotational medium with cold caches
> > * 3 times faster on an SSD with cold caches
> > * 7 times faster on  a rotational disk with warm caches
> > * 5 times faster on a SSD with warm caches
> > 
> > More Observations
> > - on ssds, we save most of the time in directory traversal and
> > (de)serializing the json metadata
> > - the index lookups spends almost all of its time in disk reads,
> > deserializing the binary metadata is almost free (Qt's json binary
> > representation is really fast to read)
> > - I haven't seen any tests in which the indexed queries have been slower.
> > 
> > These results can be explained as follows:
> > - the bottleneck is reading the files from disk
> > - on rotational media, expectedly we get huge performance penalties for
> > every seek we cause, the more files we read, the more desastrous lookups
> > times get. - Expectedly, warm pagecaches help a lot in all cases
> > 
> > Cost: Maintaining the Cache
> > 
> > These speedups do come at a cost, of course, and that is the added
> > complexity of maintaining the caches. The idea from the bof sessions had
> > been to update the caches at install time, this is essentially what can
> > be done with kplugin- update-index (it needs some added logic to give the
> > index files sensible permissions when run as root). That means that
> > packagers will have to run the index updater in their postinstall
> > routine. Not doing this at all means slower queries (or rather, no
> > speedier queries), worse is if they forget to update once in a while, in
> > which case newly installed or removed plugins might be missing or
> > dangling in the index files. This will need at least some packaging
> > discipline.
> > 
> > Index File Location
> > 
> > The indexer creates the index files in the plugin directories itself, not
> > in $CACHE or $TMP. This seems the most straight-forward way to do it,
> > since if a plugin is installed into a specific directory, the "installer"
> > will have write permission there to update the index as well. One might
> > consider putting these index files in the cache directory, like ksycoca
> > does, but in that case, we need to be smarter to actually update the
> > index files correctly, 

Re: Review Request 120198: KPluginInfo: use KPluginMetaData instead of a QVariantMap for storage

[Milian Wolff]

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



src/services/kplugininfo.cpp


http://qt-project.org/doc/qt-5/qjsonobject.html#operator-5b-5d

these should be 

static const QString s_fooKey = QStringLiteral(...);

better yet, wrap it all in an anonymous namespace and safe yourself the 
trouble of adding static everywhere



src/services/kplugininfo.cpp


use QString for the key here as well



src/services/kplugininfo.cpp


!json.contains



src/services/kplugininfo.cpp


QString



src/services/kplugininfo.cpp


this is more or less the same code as above, can you share it?



src/services/kplugininfo.cpp


!contains



src/services/kplugininfo.cpp


inline this as above, use QString and take the service smart ptr by const&


- Milian Wolff


On Sept. 14, 2014, 2:05 p.m., Alexander Richardson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120198/
> ---
> 
> (Updated Sept. 14, 2014, 2:05 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> A series of 4 commits:
> 
> 
> 
> KPluginInfo: use KPluginMetaData instead of a QVariantMap for storage
> 
> This means that KPluginInfo can now simply reuse the QJsonObject
> returned by QPluginLoader.metaData() (by storing it in a 
> KPluginMetaData object instead of having to convert the JSON to a
> QVariantMap first.
> 
> Additionally this allows very efficient conversion between KPluginInfo
> and KPluginMetaData.
> 
> ---
> Add compatibility key names to KPluginInfo::property()
> 
> ---
> 
> KPluginInfo: Fix loading JSON metadata that only has compatibility keys
> 
> This can be removed in KF6, but for now allows loading all both old
> style as well as new style metadata
> 
> 
> 
> kplugininfotest: also test objects constructed from JSON
> 
> This tests both new style JSON as well as JSON using the old key names
> 
> 
> Diffs
> -
> 
>   autotests/CMakeLists.txt 913e848ba5d1754ef7726f92604d1aaa398fa107 
>   autotests/kplugininfotest.cpp 34f87028ce08f2db1e5f57edbc6f99a237bf90ac 
>   src/services/kplugininfo.h dea07e6e63baf2483afc4a6d43d0892efc485903 
>   src/services/kplugininfo.cpp 50a6564edbbb1890c0b91badad69db967035231f 
> 
> Diff: https://git.reviewboard.kde.org/r/120198/diff/
> 
> 
> Testing
> ---
> 
> All unit tests still work
> 
> 
> Thanks,
> 
> Alexander Richardson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120199: Implement KPluginTrader::query() using KPluginLoader::findPlugins()

[Milian Wolff]

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



src/plugin/kplugintrader.cpp


here and below, the conversions are costly just to check whether something 
is contained? rewrite this please:

{
const auto& types = md.serviceTypes();
if (!types.isEmpty() && types.contains(serviceType)) {
return true;
}
}
const auto& data = md.rawData();
{
const auto& types = data.value(s_XKdeServiceTypes).toArray();
if (!types.isEmpty() && types.contains(serviceType)) {
return true;
}
}
return data.value(s_serviceTypesKey).toArray().contains(serviceType);


- Milian Wolff


On Sept. 14, 2014, 2:06 p.m., Alexander Richardson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120199/
> ---
> 
> (Updated Sept. 14, 2014, 2:06 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Implement KPluginTrader::query() using KPluginLoader::findPlugins()
> 
> 
> Diffs
> -
> 
>   src/plugin/kplugintrader.cpp 024d74ca5341a2960161b55e4efde956e80f7c76 
> 
> Diff: https://git.reviewboard.kde.org/r/120199/diff/
> 
> 
> Testing
> ---
> 
> Unit test still passes after applying RR 120198, not sure if it works without 
> it.
> 
> 
> Thanks,
> 
> Alexander Richardson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120854: KPassivePopup - Set default hide delay

[Martin Klapetek]

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

(Updated Nov. 6, 2014, 11:36 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Bugs: 340238
https://bugs.kde.org/show_bug.cgi?id=340238


Repository: knotifications


Description
---

If delay "-1" is passed, it means "server default", but in KPassivePopup the 
default was never set.

Fixes bug 340238.


Diffs
-

  src/kpassivepopup.cpp d253898 

Diff: https://git.reviewboard.kde.org/r/120854/diff/


Testing
---


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120728: Install public header for KNotifyPlugin and rename it to KNotificationPlugin

[Martin Klapetek]

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

(Updated Nov. 6, 2014, 11:36 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Repository: knotifications


Description
---

...to allow custom KNotification plugins. This class is being exported since 
ever, but the public header was missing.

Also, I'd like to rename this class to KNotificationPlugin rather than 
KNotifyPlugin as there is no KNotify anymore, but renaming already exported 
class is not allowed in frameworks as that would break BC, right?


Diffs
-

  src/knotificationplugin.cpp PRE-CREATION 
  src/knotifyplugin.h 248a66f 
  src/knotifyplugin.cpp e2efab9 
  src/notifybyaudio.h 767f1ce 
  src/notifybyaudio.cpp 99b8027 
  src/notifybyexecute.h 92781ef 
  src/notifybyexecute.cpp 254341a 
  src/notifybyktts.h a05eebf 
  src/notifybyktts.cpp 71f9ae5 
  src/notifybylogfile.h 32a8ae5 
  src/notifybylogfile.cpp fa0c103 
  src/notifybypopup.h 36aac1d 
  src/notifybypopup.cpp c7add40 
  src/notifybysound.h 44f6463 
  src/notifybysound.cpp f005b99 
  src/notifybytaskbar.h 83d46ce 
  src/notifybytaskbar.cpp 173bbb8 
  src/CMakeLists.txt 5b109c8 
  src/knotification.h 456e84b 
  src/knotificationmanager.cpp f44c660 
  src/knotificationmanager_p.h 19bb823 
  src/knotificationplugin.h PRE-CREATION 

Diff: https://git.reviewboard.kde.org/r/120728/diff/


Testing
---


Thanks,

Martin Klapetek

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: [RFC] [kservice] KPluginMetadata indexing

[Marco Martin]

On Thursday 06 November 2014, Sebastian Kügler wrote:
> Basic Mechanism
> 
> - a small tool called kplugin-update-index collects the json metadata from
> the plugins, and puts the list of plugins in a given plugin directory into
> a QJsonArray, and dumps that in Qt's json binary format to disk

since I'll end to use the mechanism for KPackage as well, it will be needed 
also some api to access this, since we have there api for install() and 
uninstall()
I'm also wondering, besides regenerating the whole cache, if would be some use 
an incremental update as well that would just add a single plugin to the index 
(may introduce new places in which the process may go wrong tough, such as not 
searching for duplicates)

> These speedups do come at a cost, of course, and that is the added
> complexity of maintaining the caches. The idea from the bof sessions had
> been to update the caches at install time, this is essentially what can be
> done with kplugin- update-index (it needs some added logic to give the
> index files sensible permissions when run as root). That means that
> packagers will have to run the index updater in their postinstall routine.
> Not doing this at all means slower queries (or rather, no speedier
> queries), worse is if they forget to update once in a while, in which case
> newly installed or removed plugins might be missing or dangling in the
> index files. This will need at least some packaging discipline.

hmm, maybe the build process with some cmake magic could generate a script in 
the build dir with the proper command so the process would be eased at least a 
little?

> Index File Location
> 
> The indexer creates the index files in the plugin directories itself, not
+1

-- 
Marco Martin
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: [RFC] [kservice] KPluginMetadata indexing

[Sebastian Kügler]

On Thursday, November 06, 2014 10:09:51 Mark Gaiser wrote:
> I'm curious about one thing. Have you done some profiling on the
> current KPluginMetaData to see where the actual hot spot is?
> In case you don't know how to do that, here are some tips:
> 1. Recompile Qt with debug symbols (not debug mode, just with the debug
> symbols) 2. Run a benchmark application via valgrind like so: valgrind
> --tool=callgrind 
> 3. Open the output file of the line above in KCacheGrind and hunt for
> those pesky hot spots.
> 
> Perhaps there is nothing to optimize and then having an index (and the
> cost of maintaining it) is worth it, but it would be best to first
> determine if the current code path can be optimized.

I've focused on reducing the I/O, since that's where we spend by far most of 
the time, somewhere beyond 90% of the whole time it takes to run the query 
(and in worse cases with cold caches and on rotational media, even more). 

The next bottleneck would be the deserialization of json data, which is for 
the binary format we use for storage (and I think which is also used in 
QPluginLoader to read the plugins json metadata). As fas as I can see, that is 
also pretty much entirely I/O bound.

There may be something to be gained in the conversion from the KPluginMetaData 
list to the KPluginInfo::List by making the query runner 'understand' 
KPluginMetaData (it knows KService and KPluginInfo currently) but that's 
definitely not a bottleneck currently. 

In the whole picture, KPluginMetaData is not a concern right now, reducing the 
I/O is what we need to do first. 

Cheers,
-- 
sebas

http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Feedback on KDBusService::activateRequested

[David Narvaez]

Hi,

After working on the implementation of the unique mode in Rekonq[0], I
have some (unsolicited) feedback about the activateRequested slot that
I would like to share in case these issues can be addressed:

Whenever I use libraries that deal with command line arguments my
first question is whether they expect the first argument to be the
name of the executable or not. For KDBusServices, it turns out that
the answer is "it depends", and I think that is almost never a good
answer. For some context, the current behavior[1] is:

- line 119: If the executable was called with more than one argument,
send them all to the instance already running through a call to the
CommandLine signal.
- line 126: If the executable was called with just one argument (the
executable name) call the Activate signal of the instance already
running (effectively sending an empty list of arguments).

At the other end, the receiver (which we can assume is using
QCommandLineParser because this is Qt5 etc) always has to deal with
the two cases because QCLP requires at least one argument and will
crash if you pass an empty list of arguments. IMO, whenever all
applications need to write the same  boilerplate code - in this case
if(arguments.size() > 0) - that is already an indication of a bad API
(see, e.g., [2]).

In addition to that, one can argue that some info is lost: suppose you
have a binary and several symlinks to that binary, and this binary
decides what to do depending on the symlink used to call it (think of
busybox). If this application was using KDBusService to implement
Unique mode, then it would no longer know what the caller binary was.

So my question woud be: is this behavior mandated by some standard? or
can it be modified to always call CommandLine with the list of
arguments?

Thanks.

David E. Narvaez

[0] https://git.reviewboard.kde.org/r/120794/
[1] 
http://quickgit.kde.org/?p=kdbusaddons.git&a=blob&h=ea772&hb=d8ff8&f=src%2Fkdbusservice.cpp
[2] 
http://lxr.kde.org/source/kde/workspace/plasma-workspace/plasma-windowed/plasmawindowedcorona.cpp?v=kf5-qt5#0103
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Split kde-baseapps?

[David Narvaez]

On Sat, Oct 25, 2014 at 3:06 PM, David Narvaez
 wrote:
> It's also used by Rekonq and anything building a web browser using KDE
> software, it should really go to kioslave-extras.

Any feedback on this? If it will not be moved to kioslave-extras I
need to patch Rekonq to stop annoying me about the missing about
protocol.

Thanks.

David E. Narvaez
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: [RFC] [kservice] KPluginMetadata indexing

[Mark Gaiser]

On Thu, Nov 6, 2014 at 4:43 PM, Sebastian Kügler  wrote:
> On Thursday, November 06, 2014 10:09:51 Mark Gaiser wrote:
>> I'm curious about one thing. Have you done some profiling on the
>> current KPluginMetaData to see where the actual hot spot is?
>> In case you don't know how to do that, here are some tips:
>> 1. Recompile Qt with debug symbols (not debug mode, just with the debug
>> symbols) 2. Run a benchmark application via valgrind like so: valgrind
>> --tool=callgrind 
>> 3. Open the output file of the line above in KCacheGrind and hunt for
>> those pesky hot spots.
>>
>> Perhaps there is nothing to optimize and then having an index (and the
>> cost of maintaining it) is worth it, but it would be best to first
>> determine if the current code path can be optimized.
>
> I've focused on reducing the I/O, since that's where we spend by far most of
> the time, somewhere beyond 90% of the whole time it takes to run the query
> (and in worse cases with cold caches and on rotational media, even more).
>
> The next bottleneck would be the deserialization of json data, which is for
> the binary format we use for storage (and I think which is also used in
> QPluginLoader to read the plugins json metadata). As fas as I can see, that is
> also pretty much entirely I/O bound.
>
> There may be something to be gained in the conversion from the KPluginMetaData
> list to the KPluginInfo::List by making the query runner 'understand'
> KPluginMetaData (it knows KService and KPluginInfo currently) but that's
> definitely not a bottleneck currently.
>
> In the whole picture, KPluginMetaData is not a concern right now, reducing the
> I/O is what we need to do first.
>
> Cheers,

Ah right, i think i missed the I/O reducing goal in your initial post.

I haven't checked your draft implementation in detail, but what you
might want to do is:
- Monitor all plugin folders for changes
- If a change in any of the folders is detected: rebuild cache.

That should keep the cache updated in a fairly easy way and remove the
need to have a tool at all.
You could do it more fine grained and only update the cache for the
plugin that changed. It will be a bit more tricky to implement.

Just an idea, but i hope it helps.

Cheers,
Mark
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120825: Fix KPluginInfo::entryPath() being empty when not loaded from .desktop

[Alexander Richardson]

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

(Updated Nov. 6, 2014, 4:09 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, David Faure and Sebastian Kügler.


Repository: kservice


Description
---

Fix KPluginInfo::entryPath() being empty when not loaded from .desktop


Diffs
-

  autotests/kplugininfotest.cpp 34f87028ce08f2db1e5f57edbc6f99a237bf90ac 
  src/services/kplugininfo.cpp 50a6564edbbb1890c0b91badad69db967035231f 

Diff: https://git.reviewboard.kde.org/r/120825/diff/


Testing
---


Thanks,

Alexander Richardson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120199: Implement KPluginTrader::query() using KPluginLoader::findPlugins()

[Alexander Richardson]

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

(Updated Nov. 6, 2014, 6:49 nachm.)


Review request for KDE Frameworks.


Changes
---

Addressed issues


Repository: kservice


Description
---

Implement KPluginTrader::query() using KPluginLoader::findPlugins()


Diffs (updated)
-

  src/plugin/kplugintrader.cpp 024d74ca5341a2960161b55e4efde956e80f7c76 

Diff: https://git.reviewboard.kde.org/r/120199/diff/


Testing
---

Unit test still passes after applying RR 120198, not sure if it works without 
it.


Thanks,

Alexander Richardson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120198: KPluginInfo: use KPluginMetaData instead of a QVariantMap for storage

[Alexander Richardson]

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

(Updated Nov. 6, 2014, 6:50 nachm.)


Review request for KDE Frameworks.


Repository: kservice


Description
---

A series of 4 commits:



KPluginInfo: use KPluginMetaData instead of a QVariantMap for storage

This means that KPluginInfo can now simply reuse the QJsonObject
returned by QPluginLoader.metaData() (by storing it in a 
KPluginMetaData object instead of having to convert the JSON to a
QVariantMap first.

Additionally this allows very efficient conversion between KPluginInfo
and KPluginMetaData.

---
Add compatibility key names to KPluginInfo::property()

---

KPluginInfo: Fix loading JSON metadata that only has compatibility keys

This can be removed in KF6, but for now allows loading all both old
style as well as new style metadata



kplugininfotest: also test objects constructed from JSON

This tests both new style JSON as well as JSON using the old key names


Diffs (updated)
-

  autotests/kplugininfotest.cpp 9d4ee046db1e5d0b9f30a9a68929147763ee1cfa 
  src/services/kplugininfo.h 871d6a2ead5a9b358d864372152cbfa0c43d8a68 
  src/services/kplugininfo.cpp 54593e57ca2b898a7d68de2915b7e40c3aa96f5f 
  autotests/CMakeLists.txt 913e848ba5d1754ef7726f92604d1aaa398fa107 

Diff: https://git.reviewboard.kde.org/r/120198/diff/


Testing
---

All unit tests still work


Thanks,

Alexander Richardson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: [RFC] [kservice] KPluginMetadata indexing

[Milian Wolff]

On Thursday 06 November 2014 17:06:38 Mark Gaiser wrote:
> On Thu, Nov 6, 2014 at 4:43 PM, Sebastian Kügler  wrote:
> > On Thursday, November 06, 2014 10:09:51 Mark Gaiser wrote:
> >> I'm curious about one thing. Have you done some profiling on the
> >> current KPluginMetaData to see where the actual hot spot is?
> >> In case you don't know how to do that, here are some tips:
> >> 1. Recompile Qt with debug symbols (not debug mode, just with the debug
> >> symbols) 2. Run a benchmark application via valgrind like so: valgrind
> >> --tool=callgrind 
> >> 3. Open the output file of the line above in KCacheGrind and hunt for
> >> those pesky hot spots.
> >> 
> >> Perhaps there is nothing to optimize and then having an index (and the
> >> cost of maintaining it) is worth it, but it would be best to first
> >> determine if the current code path can be optimized.
> > 
> > I've focused on reducing the I/O, since that's where we spend by far most
> > of the time, somewhere beyond 90% of the whole time it takes to run the
> > query (and in worse cases with cold caches and on rotational media, even
> > more).
> > 
> > The next bottleneck would be the deserialization of json data, which is
> > for
> > the binary format we use for storage (and I think which is also used in
> > QPluginLoader to read the plugins json metadata). As fas as I can see,
> > that is also pretty much entirely I/O bound.
> > 
> > There may be something to be gained in the conversion from the
> > KPluginMetaData list to the KPluginInfo::List by making the query runner
> > 'understand' KPluginMetaData (it knows KService and KPluginInfo
> > currently) but that's definitely not a bottleneck currently.
> > 
> > In the whole picture, KPluginMetaData is not a concern right now, reducing
> > the I/O is what we need to do first.
> > 
> > Cheers,
> 
> Ah right, i think i missed the I/O reducing goal in your initial post.
> 
> I haven't checked your draft implementation in detail, but what you
> might want to do is:
> - Monitor all plugin folders for changes
> - If a change in any of the folders is detected: rebuild cache.
> 
> That should keep the cache updated in a fairly easy way and remove the
> need to have a tool at all.
> You could do it more fine grained and only update the cache for the
> plugin that changed. It will be a bit more tricky to implement.
> 
> Just an idea, but i hope it helps.

Most linux distros have a strict limit of file watches. Baloo, KDevelop and 
other projects already battle for that resource. Adding one more watcher won't 
simplify that situation. Generally, there will be occasions where adding a 
watcher failed. You cannot rely on that feature to work reliably.

bye
-- 
Milian Wolff
m...@milianw.de
http://milianw.de
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120198: KPluginInfo: use KPluginMetaData instead of a QVariantMap for storage

[Milian Wolff]

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



src/services/kplugininfo.cpp


ugh. why not an anon namespace? I'd still use `const foo QString = 
QStringLiteral(...)` personally...


- Milian Wolff


On Nov. 6, 2014, 5:50 p.m., Alexander Richardson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120198/
> ---
> 
> (Updated Nov. 6, 2014, 5:50 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> A series of 4 commits:
> 
> 
> 
> KPluginInfo: use KPluginMetaData instead of a QVariantMap for storage
> 
> This means that KPluginInfo can now simply reuse the QJsonObject
> returned by QPluginLoader.metaData() (by storing it in a 
> KPluginMetaData object instead of having to convert the JSON to a
> QVariantMap first.
> 
> Additionally this allows very efficient conversion between KPluginInfo
> and KPluginMetaData.
> 
> ---
> Add compatibility key names to KPluginInfo::property()
> 
> ---
> 
> KPluginInfo: Fix loading JSON metadata that only has compatibility keys
> 
> This can be removed in KF6, but for now allows loading all both old
> style as well as new style metadata
> 
> 
> 
> kplugininfotest: also test objects constructed from JSON
> 
> This tests both new style JSON as well as JSON using the old key names
> 
> 
> Diffs
> -
> 
>   autotests/kplugininfotest.cpp 9d4ee046db1e5d0b9f30a9a68929147763ee1cfa 
>   src/services/kplugininfo.h 871d6a2ead5a9b358d864372152cbfa0c43d8a68 
>   src/services/kplugininfo.cpp 54593e57ca2b898a7d68de2915b7e40c3aa96f5f 
>   autotests/CMakeLists.txt 913e848ba5d1754ef7726f92604d1aaa398fa107 
> 
> Diff: https://git.reviewboard.kde.org/r/120198/diff/
> 
> 
> Testing
> ---
> 
> All unit tests still work
> 
> 
> Thanks,
> 
> Alexander Richardson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 120199: Implement KPluginTrader::query() using KPluginLoader::findPlugins()

[Milian Wolff]

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



src/plugin/kplugintrader.cpp


here and below: QStringLiteral


- Milian Wolff


On Nov. 6, 2014, 5:49 p.m., Alexander Richardson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120199/
> ---
> 
> (Updated Nov. 6, 2014, 5:49 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Implement KPluginTrader::query() using KPluginLoader::findPlugins()
> 
> 
> Diffs
> -
> 
>   src/plugin/kplugintrader.cpp 024d74ca5341a2960161b55e4efde956e80f7c76 
> 
> Diff: https://git.reviewboard.kde.org/r/120199/diff/
> 
> 
> Testing
> ---
> 
> Unit test still passes after applying RR 120198, not sure if it works without 
> it.
> 
> 
> Thanks,
> 
> Alexander Richardson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: [RFC] [kservice] KPluginMetadata indexing

[Nicolás Alvarez]

2014-11-05 23:44 GMT-03:00 Sebastian Kügler :
> So, this code is in a bit of a draft stage, I'd very much welcome feedback
> about the approach, and of course the code itself. It can be found in
> kservice[sebas/kpluginindex]. the kpluginmetadata autotest gives a useful
> testing target. I didn't submit it to reviewboard yet, because I want to nail
> down the further direction, and provide a base to discuss on.

I tried it on Windows :)

Needed a minor tweak to build on MSVC2010 (have to specify the return
type of lambdas). Can I commit this?

findPluginSubdirectories is checking if the filename ends with ".so",
I had to change it to ".dll". But why is the check there, if
QDirIterator is already filtering?

After those changes, kplugin-update-index -a successfully generated 14
.json files. I think .json isn't a good file extension for them if
they aren't text JSON.

I ran the benchmark a few times and having the index made it about 24x
faster, with warm caches (I have no idea if there is anything like
drop_caches on Windows or if I'd have to reboot).

-- 
Nicolás
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 121020: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

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

Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: plasma-desktop


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285&t=123587
https://git.reviewboard.kde.org/r/121019/
https://git.reviewboard.kde.org/r/121021/


Diffs
-

  kcms/krdb/krdb.cpp 8452aa5 
  kcms/style/kcmstyle.cpp 9a13f45 

Diff: https://git.reviewboard.kde.org/r/121020/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 121021: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

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

Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: kdelibs4support


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285&t=123587
https://git.reviewboard.kde.org/r/121019/
https://git.reviewboard.kde.org/r/121020/


Diffs
-

  src/kdeui/k4style.cpp a1a2ab1 
  src/kdeui/kglobalsettings.h d63ac69 

Diff: https://git.reviewboard.kde.org/r/121021/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 121019: reverse ShowIconsOnPushButtons default

[Thomas Lübking]

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

Review request for KDE Frameworks, kdelibs and Thomas Pfeiffer.


Repository: frameworkintegration


Description
---

sumamrized, also see
https://forum.kde.org/viewtopic.php?f=285&t=123587
https://git.reviewboard.kde.org/r/121020/
https://git.reviewboard.kde.org/r/121021/


Diffs
-

  autotests/kdeplatformtheme_changed_kdeglobals 910e0e3 
  autotests/kdeplatformtheme_kdeglobals df52410 
  src/kstyle/kstyle.cpp b5f7363 
  src/platformtheme/khintssettings.cpp 8799216 

Diff: https://git.reviewboard.kde.org/r/121019/diff/


Testing
---


Thanks,

Thomas Lübking

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 121007: Fix warning when using newer upower backend.

[Kevin Funk]

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

Ship it!


Finally!

- Kevin Funk


On Nov. 6, 2014, 1:05 a.m., Milian Wolff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121007/
> ---
> 
> (Updated Nov. 6, 2014, 1:05 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: solid
> 
> 
> Description
> ---
> 
> "No such signal org::freedesktop::UPower::DeviceAdded(...)"
> 
> The signature change can be detected at runtime using Qt's QMetaObject
> introspection mechanism. That prevents us from emitting the two
> pesky warnings at runtime, polluting our konsoles.
> 
> Google is full of that warning, and there is also: 
> https://bugzilla.redhat.com/show_bug.cgi?id=1056769
> 
> 
> Diffs
> -
> 
>   src/solid/devices/backends/upower/upowermanager.cpp 
> 53c858093a1c439f0faca0c956a51199f4e882e4 
> 
> Diff: https://git.reviewboard.kde.org/r/121007/diff/
> 
> 
> Testing
> ---
> 
> warning gone!
> 
> 
> Thanks,
> 
> Milian Wolff
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel