[Kde-pim] Review Request 119418: LIST: Non-Recursive listing
Christian Mollekopf
chrigi_1 at fastmail.fm
Thu Jul 24 16:48:01 BST 2014
> On July 24, 2014, 3:38 p.m., Dan Vrátil wrote:
> > I have one concern regarding this LIST implementation: in retrieveChildren() this code iterates over the entire set of collections (which can be a huuge number) up to 5 times. I'm convinced that it could be optimized, for instance by merging the mimetype filter, referenced/enabled collections filter and missigCollections check into one loop.
Right, my test show though that performance potentially increases over the old implementation (with 1k enabled collections and 200k disabled collections). This doesn't even take into account that the old implementation did not find enabled collections in disabled collections, so if the features would be the same I would expect the original implementation to be even worse.
Anyways, I agree we can optimize this.
> On July 24, 2014, 3:38 p.m., Dan Vrátil wrote:
> > server/src/handler/list.cpp, line 213
> > <https://git.reviewboard.kde.org/r/119418/diff/1/?file=292095#file292095line213>
> >
> > On deeply-nested structures, this might be a performance issue. It will almost degradate the code to the original recursive listing.
> >
> > The entire parent chain should already be held in the collections list you got from database, so maybe just quickly trying to find it in the list would be faster?
>
> Dan Vrátil wrote:
> Hmm, maybe if you move the code that builds the "childMap" QHash in the mimetype filter here, then you would have a fast lookup without querying the database?
Perhaps thats faster, or I'll just use that childMap that I anyways need for later on.
> On July 24, 2014, 3:38 p.m., Dan Vrátil wrote:
> > server/src/handler/list.cpp, line 225
> > <https://git.reviewboard.kde.org/r/119418/diff/1/?file=292095#file292095line225>
> >
> > Since the childMap is only used few lines below just to check whether the collection has any children, it could just be a QSet<qint64> instead of a QHash.
> >
> > Actually if you build the map before the
a part of this sentence got lost ;-)
> On July 24, 2014, 3:38 p.m., Dan Vrátil wrote:
> > server/src/handler/list.cpp, line 287
> > <https://git.reviewboard.kde.org/r/119418/diff/1/?file=292095#file292095line287>
> >
> > This has to be col.id() - otherwise collections get sometimes listed multiple times in the output, which leads to the assert in CollectionSync that I was getting. Apparently it's only reproducible on more complicated hierarchies, and also dependso n the order in which you get collections from the database.
Could you perhaps come up with a test or supply me with a tree that exposes the bug? Otherwise I'll try to figure it out.
> On July 24, 2014, 3:38 p.m., Dan Vrátil wrote:
> > server/src/handler/list.cpp, line 310
> > <https://git.reviewboard.kde.org/r/119418/diff/1/?file=292095#file292095line310>
> >
> > The purpose of this code was to insert the fetched missing collections into the right position in the overall collections list in order to keep the ordering same as the original implementation.
> >
> > However it is quite expensive, as it requires iterating over the full list of collections many times (std::find_if is just a nicely written nested Q_FOREACH :-))
> >
> > We could remove this and only prepend the fetched ancestors at the beginning of the list - that would ensure they are available on the client side before their descendants, which is always better. I tested it locally and everything seems to still work (except for LIST unittest ;-))
Fine with me, my try to preserve the order anyways only partially worked.
- Christian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119418/#review63078
-----------------------------------------------------------
On July 23, 2014, 12:01 p.m., Christian Mollekopf wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119418/
> -----------------------------------------------------------
>
> (Updated July 23, 2014, 12:01 p.m.)
>
>
> Review request for Akonadi.
>
>
> Repository: akonadi
>
>
> Description
> -------
>
> CollectonReferenceTest: Fixed the tests after fixing the reference manager
>
> This test couldn't work because the referenced state is reset after
> each scenario. I adapated the test now accordingly.
>
> LIST: Non-recursive listing
>
> Instead of recursively query for children,
> we query for a list of all collections we're interested in,
> and then assemble a tree.
>
> This potentially results in a small overhead with few collections,
> but should scale much better when we have many collections while most
> are getting filtered by the initial query (i.e. disabled collections).
>
> Additionally this patch ensures enabled collections that are nested in
> a disabled collection are correctly found.
>
> Share DbInitializer for other tests and make use in listhandlertest.
>
> use dbinitializer
>
> remove the collections manually since sqlite doesn't deal with constraints.
>
>
> Diffs
> -----
>
> server/tests/unittest/listhandlertest.cpp b25b6a85538cec786c09a2f2cc629b2be5c82fec
> server/src/handler/list.h 56401b3164e5035518d63ed39e5a048481808560
> server/src/handler/list.cpp b891d10d2ceb63482a4453695dc38ee625b8c768
> server/tests/unittest/CMakeLists.txt b9744d93a3b0cb9e895141c10ddaf2703f12acf0
> server/tests/unittest/collectionreferencetest.cpp 808227f9771a33dc1c77d960160770ca919ea2fd
> server/tests/unittest/dbinitializer.h PRE-CREATION
> server/tests/unittest/dbinitializer.cpp PRE-CREATION
>
> Diff: https://git.reviewboard.kde.org/r/119418/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Christian Mollekopf
>
>
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list