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

Andras Mantia amantia at kde.org
Sat Feb 18 11:37:43 GMT 2012 


> On Feb. 12, 2012, 4:43 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/maildir.h, line 239
> > <http://git.reviewboard.kde.org/r/103947/diff/4/?file=49328#file49328line239>
> >
> >     QStringList?

Changed (I use QList<QString> as that was what we pass to it in RetrieveItemsJob)


> On Feb. 12, 2012, 4:43 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/maildir.h, line 243
> > <http://git.reviewboard.kde.org/r/103947/diff/4/?file=49328#file49328line243>
> >
> >     I know I was complaining about a const being removed, but that doesn't sound like a method that would not change anything :)

Technically it doesn't change the class, but yeah, it is contradictory with the method name. Removed.


> On Feb. 12, 2012, 4:43 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/maildir.cpp, line 21
> > <http://git.reviewboard.kde.org/r/103947/diff/4/?file=49329#file49329line21>
> >
> >     Looks like keycache.h and keycache.cpp are missing

Argh, I forgot to git add, and then git diff doesn't include them...


> On Feb. 12, 2012, 4:43 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/maildir.cpp, line 80
> > <http://git.reviewboard.kde.org/r/103947/diff/4/?file=49329#file49329line80>
> >
> >     Singletons in libraries always make me feel uneasy.
> >     Is this save enough when two threads work with the library?

I don't see this used in threads, the whole maildir lib was not really designed to be thread safe and it is not marked as being safe. I also don't see the code depending on any initalization or destruction order, so imo it is safe as it is now, but feel free to correct me. 


> On Feb. 12, 2012, 4:43 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/maildir.cpp, line 125
> > <http://git.reviewboard.kde.org/r/103947/diff/4/?file=49329#file49329line125>
> >
> >     Shouldn't the cache be updated in this case?

This is for debug only, so I can see if the cache works as it should. It is even protected with an ifdef.


> On Feb. 12, 2012, 4:43 p.m., Kevin Krammer wrote:
> > resources/maildir/libmaildir/maildir.cpp, line 589
> > <http://git.reviewboard.kde.org/r/103947/diff/4/?file=49329#file49329line589>
> >
> >     This looks weird from a code readability point of view.
> >     A key is removed and then immediately added again?

Hm, it removes all keys for that file and adds it as a "new" key, to be sure that the key is present in the cache only for the "new" subdirs. Any suggestion for naming? 


> On Feb. 12, 2012, 4:43 p.m., Kevin Krammer wrote:
> > resources/maildir/maildirresource.cpp, line 466
> > <http://git.reviewboard.kde.org/r/103947/diff/4/?file=49331#file49331line466>
> >
> >     Any specific reason why const got removed?

There was a reason while developing, I just forgot to add the const back after. Done now.


> On Feb. 12, 2012, 4:43 p.m., Kevin Krammer wrote:
> > resources/maildir/retrieveitemsjob.h, line 28
> > <http://git.reviewboard.kde.org/r/103947/diff/4/?file=49332#file49332line28>
> >
> >     maybe remove this debug stuff before committing?

Sure, as soon as the patch is approved, i will clean up this debug info.


- Andras


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


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 at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/

More information about the kde-pim mailing list