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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    [Kde-pim] Review Request: Fix ETM memory leak: m_Items hash table was never cleaned up
From:       "Milian Wolff" <mail () milianw ! de>
Date:       2012-10-13 18:04:50
Message-ID: 20121013180450.10961.51510 () vidsolbach ! de
[Download RAW message or body]

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

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.

Thanks,

Milian Wolff

_______________________________________________
KDE PIM mailing list kde-pim@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

[prev in list] [next in list] [prev in thread] [next in thread] 