[RFC] [kservice] KPluginMetadata indexing

[Sebastian Kügler]

Hi Milian,

Thanks for looking at my patches. Since the branch, as you noticed, is quite 
contaminated with unrelated changes, merging in and out stuff, I'll push these 
changes to a cleaned branch, and will address the issues you pointed out. I'll 
post a new RR then.

On Sunday, November 16, 2014 14:13:39 Milian Wolff wrote:
> On Thursday 06 November 2014 03:44:58 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.
> 
> <snip>
> 
> Hey Sebas,
> 
> I just took a look at your code. What comes to my mind you'll find below.
> 
> a) you added a "return;" in the middle of a pluginlocatortest - why? no
> comment either. If you expect failures, use QEXPECT_FAIL. I've added this
> also to PluginTest::findPackageStructure now.
> 
> b) adding code in a merge commit is a very bad idea. Never do that. (see
> 3c9f33251708c6d935ed35fbfb6130c4389e69a7). One reason is that merge commits
> are "special" and often omitted when looking at code. See e.g. "git log -u"
> which won't show the code you added in the merge...
> 
> c) The code seems to contain tons of debug code, commented out code etc.
> pp.. Will you please clean that up before you merge this in? I.e. squash it
> locally with the cleanup commits to not uglify the git history.
> 
> d) Overall I find it extremely hard to review the code since there is so
> much unrelated stuff going on. Now that Alexander's work is in master (it
> is, no?) could you maybe cleanup your branch and do a force-push rebased on
> top of current master? If you do that together with c) I could actually see
> whats supposed to be going on without being confused by tons of new code
> that's only there for debug purposes.
> 
> e) // Less than two plugin means it's not worth indexing
> 
> I'd argue this code path can be removed. In a normal session there will
> always be more than two plugins.
> 
> f) I urge you (and everybody else) to rethink some code structures around
> conditionals. Most of the time one can greatly simplify functions (imo) by
> the following (inspired by KPluginIndexer::removeIndex):
> 
> bool ok = true;
> if (foo) {
>   if (bar) {
>     if (!asdf) {
>       ok = false;
>       qWarning()
>     } else {
>       // success
>     }
>   } else {
>     qWarning();
>     ok = false;
>   }
> }
> return ok;
> 
> rewrite this as:
> 
> if (!foo) {
>   return true;
> }
> if (!bar) {
>   return false;
> }
> if (!asdf) {
>   return false;
> }
> // success!
> 
> g) I still don't know how to benchmark your code.
> 
> Bye

-- 
sebas

http://www.kde.org | http://vizZzion.org | GPG Key ID: 9119 0EF9