[Kde-pim] Review Request 109186: work around a file descriptor leak when using QFileSystemWatcher

[Andras Mantia]

> On Feb. 27, 2013, 9:56 a.m., Andras Mantia wrote:
> > resources/maildir/maildirresource.cpp, line 841
> > <http://git.reviewboard.kde.org/r/109186/diff/1/?file=116066#file116066line841>
> >
> >     I prefer caching the path here( like it was in the places you have replaced), although probably the difference is not too big.
> 
> Wolfgang Rohdewald wrote:
>     dir.path() does not do much. Only "return d->path". Since everything is const, the compiler should be able to cache it automatically. Allocating a local QString also needs time, I suppose. And for full caching as it was, I would not be able to pass dir to the new methods, I would have to pass the path. So I would always have to cache path before calling stopDirScan. Too much additional code, IMHO

Calling dir.path() is a call to a function (dir.path()), a pointer indirection (d->path) to get the path. You do this twice.
If you cache, you have an extra QString copy constructor calling (const QString path = dir.path();), while you save the call to function and the pointer indirection (d->path).
I'm not sure how much the compiler optimizes this automatically, but the caching is what I like better, anytime the same method needs to be called more than one time. It also makes debugging easier, as you see the path variable in the debugger, you don't have to jump into the path() method.


- Andras


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


On Feb. 27, 2013, 9:32 a.m., Wolfgang Rohdewald wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109186/
> -----------------------------------------------------------
> 
> (Updated Feb. 27, 2013, 9:32 a.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Description
> -------
> 
> this cleans up some code and avoids warnings from QFileSystemWatcher when many mails are moved between folders. QFileSystemWatcher leaks four file descriptors for every moved mail because removeDir does not close the fd (as can be seen in /proc/X/fd where X is the process id of akonadi_agent_launcher akonadi_maildir_resource). On my system (kubuntu 12.10) warnings start when 1024 file descriptors are open.
> 
> I did not yet investigate which notifying method QFileSystemWatcher actually uses.
> 
> This has 3 commits:
> 
> 1. factor out common code
> 2. add a missing restore command in an error path
> 3. do not use removeDir/addDir but stopDirScan/restartDirScan
> 
> 
> Diffs
> -----
> 
>   resources/maildir/maildirresource.h f792b23f49cd493f128602649384d2a790ec3b47 
>   resources/maildir/maildirresource.cpp 6485f99d8f81cc46913cc473978ace818cbd114d 
> 
> Diff: http://git.reviewboard.kde.org/r/109186/diff/
> 
> 
> Testing
> -------
> 
> moving mails between folders. Needed time is about the same as before (like 50 seconds for 900 mails)
> 
> 
> Thanks,
> 
> Wolfgang Rohdewald
> 
>

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