[Kde-pim] Review Request: Fix ETM memory leak: m_Items hash table was never cleaned up

[Commit Hook]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106832/#review22477
-----------------------------------------------------------


This review has been submitted with commit 886af66da176d9de7e3368ca83fb037e5c295858 by Milian Wolff to branch master.

- Commit Hook


On Oct. 15, 2012, 12:13 p.m., Milian Wolff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106832/
> -----------------------------------------------------------
> 
> (Updated Oct. 15, 2012, 12:13 p.m.)
> 
> 
> Review request for KDEPIM, KDEPIM-Libraries and Stephen Kelly.
> 
> 
> Description
> -------
> 
> As it turned out, the ETM kept filling its QHash<Item::Id, Item> m_items without ever cleaning it up. This resulted in increasing memory consumption of KMail until eventually all collections where fetched at least once by the ETM. Note: It's not a true leak, i.e. the data is still reachable and gets properly cleaned up on close. But during runtime the ever increasing memory consumption is easily noticeable.
> 
> Generally this patch ensures that the existing cache code is actually working. Maybe the old code was working once, but a regression due to the patch that moved some of that code from ETMPrivate to MonitorPrivate::PurgeBuffer crept in?
> 
> Anyhow, this patch contains basically of these parts:
> 
> a) Ensure that the SelectionProxyModel properly derefs root indices in its dtor. Should be a non-brainer, compare to the ctor e.g. If this is not done, collections will always stay reffed and thus never cleared up.
> 
> b) Fix the implementation of the PurgeBuffer. The old code was as far as I could see non functional. The safety calls to ::purge from ::buffer e.g. purged much more than just the one requested ID from the buffer. This in turn resulted in ::buffer never returning something besides "-1".
> 
> Instead, the code is now simplified by using a QQueue FIFO. Considering though that this change was mostly based on how I thought the buffer *should* behave like, this might need some reviewing. Note that I didn't find the documentation sufficient. The buffer, as I've written it now, is now working as following:
> 
> PurgeBuffer::purge(id) -> just remove id from FIFO
> PurgeBuffer::buffer(id) -> 
> - remove id if it already is tracked (i.e. purge(id))
> - ensure the FIFO is not bigger than X items, if it is, dequeue and purge first item
> - enqueue id to FIFO
> 
> c) Fix the implementation of ETMPrivate::removeItems and ::purgeItems such that it does not keep iterators around which get invalidated once we call .erase on the parent container. This would yield double deletion runtime errors and the like.
> 
> d) Fix the memory leak by actually removing items from m_items in ::removeItems. This requires us to remove the purged'd collection id from m_populatedCols.
> 
> Note: This patch should be the basis for evaluating the caching parameters (MAXITEMS=10000 and MAXBUFFERSIZE=10). Especially I think we should investigate whether the MAXITEMS should not take precedence over MAXBUFFERSIZE, such that e.g. opening 10 10k folders does not result in 100k of items in memory, but instead only 10k items.
> 
> Note: Favorited folders are always reffed and thus never cleared from the cache.
> 
> 
> Diffs
> -----
> 
>   akonadi/entitytreemodel_p.cpp bc2ca28 
>   akonadi/monitor_p.h b5d0984 
>   akonadi/monitor_p.cpp b2e85eb 
>   akonadi/selectionproxymodel.cpp 72ca71a 
> 
> Diff: http://git.reviewboard.kde.org/r/106832/diff/
> 
> 
> Testing
> -------
> 
> Ran it against my local setup. The memory consumption stays roughly constant over time now when reading all my folders, instead of increasing over time until all collections have been seen.
> 
> To reproduce: Open KMail, go from one collection to the next until you have visited all. Note the memory consumption intermittently. Before: Memory consumption increases until the end. After: Memory consumption will reach a peak and go up and down as you open your collectoins.
> 
> 
> Screenshots
> -----------
> 
> memory consumption before patch
>   http://git.reviewboard.kde.org/r/106832/s/774/
> memory consumption after patch
>   http://git.reviewboard.kde.org/r/106832/s/775/
> 
> 
> Thanks,
> 
> Milian Wolff
> 
>

_______________________________________________
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/