Fix ETM memory leak: m_Items hash table was never cleaned up

Review Request #106832 - Created Oct. 13, 2012 and submitted

Milian Wolff
kdepim, kdepimlibs
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.
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.

Volker Krause
Milian Wolff
Àlex Fiestas
Allen Winter
Stephen Kelly
Commit Hook
Commit Hook
Commit Hook
Milian Wolff
Review request changed

Status: Closed (submitted)