[Kde-pim] Review Request 119418: LIST: Non-Recursive listing

[Dan Vrátil]

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


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.


server/src/handler/list.cpp
<https://git.reviewboard.kde.org/r/119418/#comment43809>

    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?



server/src/handler/list.cpp
<https://git.reviewboard.kde.org/r/119418/#comment43810>

    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



server/src/handler/list.cpp
<https://git.reviewboard.kde.org/r/119418/#comment43812>

    You already have knownIds calculated from the mimetype filter above (at the "replace QHash by QSet" comment), so there's no need to create one here again - you could reuse it.



server/src/handler/list.cpp
<https://git.reviewboard.kde.org/r/119418/#comment43811>

    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.



server/src/handler/list.cpp
<https://git.reviewboard.kde.org/r/119418/#comment43814>

    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 ;-))



server/src/handler/list.cpp
<https://git.reviewboard.kde.org/r/119418/#comment43815>

    This is probably not needed.


- Dan Vrátil


On July 23, 2014, 2: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, 2: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/