'Re: [Kde-pim] Review Request: Optimize file access in maildir'

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

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request: Optimize file access in maildir
From:       "Andras Mantia" <amantia () kde ! org>
Date:       2012-02-18 20:06:48
Message-ID: 20120218200648.29107.3735 () vidsolbach ! de
[Download RAW message or body]

> On Feb. 18, 2012, 3:24 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/keycache.h, line 41
> > <http://git.reviewboard.kde.org/r/103947/diff/5/?file=50380#file50380line41>
> > 
> > I know this is already inconsistent in the maildir.h file, but I think KDEPIM \
> > coding style favors QString &dir
> > over
> > QString& dir
> > 

No idea, I know I'm also sometime inconsistent with myself when it comes about \
pointer and reference declarations. :) Most of maildir is using QString& dir, \
probably that was also my previous contribution. So I'd rather not change it here.

> On Feb. 18, 2012, 3:24 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/keycache.h, line 69
> > <http://git.reviewboard.kde.org/r/103947/diff/5/?file=50380#file50380line69>
> > 
> > Hmm, I think KDEPIM coding style suggest using members in this form:
> > mNewKeys

Changed.

> On Feb. 18, 2012, 3:24 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/keycache.cpp, line 31
> > <http://git.reviewboard.kde.org/r/103947/diff/5/?file=50381#file50381line31>
> > 
> > Won't newKeys.size() always be 0 here?
> > dir is not in m_newKeys, so m_newKeys[dir] will create and return an empty set, \
> > no? 
> > In case this is true, could this not just be written as
> > m_newKeys[dir] = listNew(dir)
> > 

Hm, you're right, since I've added the "if" check (first version didn't have it), the \
complicated addition is not needed.

> On Feb. 18, 2012, 3:24 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/keycache.cpp, line 73
> > <http://git.reviewboard.kde.org/r/103947/diff/5/?file=50381#file50381line73>
> > 
> > What about
> > return m_curKeys.value( dir ).contains( key );

In general I don't like that, as the default constructed value when dir is not yet in \
the hash is not explicitely visible what it can be. Here yes, will be an empty set \
and the code will work, it is just not that readble imo.

> On Feb. 18, 2012, 3:24 p.m., Kevin Krammer wrote:
> > resources/maildir/maildirresource.cpp, line 748
> > <http://git.reviewboard.kde.org/r/103947/diff/5/?file=50385#file50385line748>
> > 
> > I know that it not something you have changed, but this should probably be
> > setFlag( flag )
> > i.e. spaces around function argument

Corrected.

- Andras

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

On Feb. 18, 2012, 11:36 a.m., Andras Mantia wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103947/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2012, 11:36 a.m.)
> 
> 
> Review request for KDEPIM and Kevin Krammer.
> 
> 
> Description
> -------
> 
> This patch reduces the number of file stats (QFile::exist) in the maildir library \
> by caching the content of the cur/new directory listing and using QMap lookup to \
> search for file existance. The patch also optimizes the mapping between collections \
> and Maildir objects in the resource. 
> 
> Diffs
> -----
> 
> resources/maildir/libmaildir/CMakeLists.txt b8cfb92 
> resources/maildir/libmaildir/keycache.h PRE-CREATION 
> resources/maildir/libmaildir/keycache.cpp PRE-CREATION 
> resources/maildir/libmaildir/maildir.h b038f02 
> resources/maildir/libmaildir/maildir.cpp d61ac0b 
> resources/maildir/maildirresource.h 233732c 
> resources/maildir/maildirresource.cpp b5aa3e2 
> resources/maildir/retrieveitemsjob.h dfc7095 
> resources/maildir/retrieveitemsjob.cpp 97f22b3 
> 
> Diff: http://git.reviewboard.kde.org/r/103947/diff/
> 
> 
> Testing
> -------
> 
> 1) make test in libmaildir passes
> 2) I run a variant of the patch for quite some time (~2 months) without problem. \
> The only difference was using a QStringList instead of QMap for caching, which make \
> the code even slower. :) 3) You can see the poor man's profiling with the QTime: \
> with a 18K folder subsequent synching (when nothing changes, just click on the \
> folder in KMail that triggers a sync), is around 1100ms. With the patch it is \
> around 670ms. With a 30K folder it was above 2000ms, now it is below 1200ms. This \
> is the time for "ENTRIESPROCESSED", the actual amount of time needed to compare the \
> items in the database with the files on the disk. 
> Here are the full numbers (3 runs for each folder, numbers in ms):
> 
> 18K folder, original code:
> LOCALLISTDONE AT 1856 
> ENTRIESPROCESSED AT 1101 
> 
> LOCALLISTDONE AT 1879 
> ENTRIESPROCESSED AT 1098 
> 
> LOCALLISTDONE AT 1809 
> ENTRIESPROCESSED AT 1097 
> 
> 
> 18K - with patch:
> LOCALLISTDONE AT 1923 
> ENTRIESPROCESSED AT 675 
> 
> LOCALLISTDONE AT 2002 
> ENTRIESPROCESSED AT 674 
> 
> LOCALLISTDONE AT 2045 
> ENTRIESPROCESSED AT 669 
> 
> Net gain is 300-400ms.
> 
> 32K-original:
> LOCALLISTDONE AT 3627 
> ENTRIESPROCESSED AT 2041 
> 
> LOCALLISTDONE AT 3826 
> ENTRIESPROCESSED AT 2009 
> 
> LOCALLISTDONE AT 3698 
> ENTRIESPROCESSED AT 2024 
> 
> 
> 32K -with patch:
> LOCALLISTDONE AT 3447 
> ENTRIESPROCESSED AT 1304 
> 
> LOCALLISTDONE AT 3728 
> ENTRIESPROCESSED AT 1216 
> 
> 
> LOCALLISTDONE AT 3723 
> ENTRIESPROCESSED AT 1207 
> 
> Net gain is 900-1000ms.
> 
> 
> Thanks,
> 
> Andras Mantia
> 
> 

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