Re: [RFC] [kservice] KPluginMetadata indexing
On Thu, Nov 6, 2014 at 3:44 AM, Sebastian Kügler se...@kde.org 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, since it's orthogonal to KPluginTrader. That aside, I've run the same queries through
Re: [RFC] [kservice] KPluginMetadata indexing
On Thursday 06 November 2014 10:09:51 Mark Gaiser wrote: On Thu, Nov 6, 2014 at 3:44 AM, Sebastian Kügler se...@kde.org 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
Re: Review Request 120198: KPluginInfo: use KPluginMetaData instead of a QVariantMap for storage
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120198/#review69928 --- src/services/kplugininfo.cpp https://git.reviewboard.kde.org/r/120198/#comment48918 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 https://git.reviewboard.kde.org/r/120198/#comment48920 use QString for the key here as well src/services/kplugininfo.cpp https://git.reviewboard.kde.org/r/120198/#comment48919 !json.contains src/services/kplugininfo.cpp https://git.reviewboard.kde.org/r/120198/#comment48921 QString src/services/kplugininfo.cpp https://git.reviewboard.kde.org/r/120198/#comment48923 this is more or less the same code as above, can you share it? src/services/kplugininfo.cpp https://git.reviewboard.kde.org/r/120198/#comment48922 !contains src/services/kplugininfo.cpp https://git.reviewboard.kde.org/r/120198/#comment48924 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()
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120199/#review69929
---
src/plugin/kplugintrader.cpp
https://git.reviewboard.kde.org/r/120199/#comment48925
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
--- 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
--- 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
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
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 your_benchmark_app 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
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.gita=blobh=ea772hb=d8ff8f=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?
On Sat, Oct 25, 2014 at 3:06 PM, David Narvaez david.narv...@computer.org 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
On Thu, Nov 6, 2014 at 4:43 PM, Sebastian Kügler se...@kde.org 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 your_benchmark_app 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
--- 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()
--- 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
--- 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
On Thursday 06 November 2014 17:06:38 Mark Gaiser wrote: On Thu, Nov 6, 2014 at 4:43 PM, Sebastian Kügler se...@kde.org 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 your_benchmark_app 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
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120198/#review69947 --- src/services/kplugininfo.cpp https://git.reviewboard.kde.org/r/120198/#comment48935 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()
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/120199/#review69948 --- src/plugin/kplugintrader.cpp https://git.reviewboard.kde.org/r/120199/#comment48936 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
2014-11-05 23:44 GMT-03:00 Sebastian Kügler se...@kde.org: 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
--- 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=285t=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
--- 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=285t=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
--- 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=285t=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.
--- 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
