[Nepomuk] Review Request: Misc Filewatch fixes

[Simeon Bird]

> On Oct. 29, 2012, 11:18 a.m., David Faure wrote:
> > This assumes that QRegExp::exactMatch is re-entrant, i.e. that it doesn't update an internal cache on demand. I'm not so sure of that, the regexp parsing sounds like a very good candidate for on-demand parsing, unless you make extra sure that it's all done upfront.
> 
> Simeon Bird wrote:
>     The Qt documentation claims it is:
>     
>     http://qt-project.org/doc/qt-4.8/qregexp.html
>     "Note: All functions in this class are reentrant."
>     
>     (although it's true that I hadn't checked up to now, so that was still a good question)
> 
> David Faure wrote:
>     Never trust documentation :-)
>     
>     I had a look at the source code, and found indeed
>     1) on-demand initializations
>     2) no locking mechanism.
>     
>     So I wrote a test program, and guess what? It gives different output from one run to the next. It's memcheck-clean, but helgrind manages to spot the issue in some runs.
>     http://www.davidfaure.fr/2012/qregexp_race.cpp
>     http://www.davidfaure.fr/2012/qregexp-helgrind-log
>     The program is supposed to write out "true true true" but only does so 1 time out of 8 on average, in my tests. Nice, heh?
>     
>     => The documentation is wrong. I'll submit a docu fix to Qt...
> 
> David Faure wrote:
>     Wait, the documentation is right, the class is reentrant (i.e. different instances can be used in different threads).
>     
>     It's just not threadsafe (i.e. you can't use the same instance of QRegExp in different threads).
>     Therefore the documentation isn't wrong, but this patch is.
>     You must use one QRegExp per thread, or ensure exclusive use of the QRegExp instance.
>     
>     A QThreadStorage would make it possible to use one QRegExp per thread, but is rather difficult to handle if the regexp could change at runtime. So I think the only solution is "exclusive use of the QRegExp instance", i.e. QWriteLocker rather than QReadLocker (new conclusion: careful with what appears to be readonly).
> 
> Simeon Bird wrote:
>     Aha! Ok, you're quite right. I see also that I was confused about the difference between POSIX's definition of re-entrant and Qt's...
>     I'll make it a WriteLocker and add a comment in v2. Thanks for teaching me C++ once again :)
> 
> Sergey Borovkov wrote:
>     What do you mean difference in definition? If I am not mistaken re-entrant function can't use global data - and you are using just that. All class members have explicit 'this' parameter passed to to function.

Qt docs say:
"A reentrant function can also be called simultaneously from multiple threads, but only if each invocation uses its own data."
which QRegExp::exactMatch is documented to be, and is, as David pointed out.

POSIX says:
A "reentrant function" is defined as a "function whose effect, when called by two or more threads, is guaranteed to be as if the threads each executed the function one after another in an undefined order, even if the actual execution is interleaved" (http://www.unix.org/whitepapers/reentrant.html)

which it is not, no? Due, as you say, to the use of the 'this' parameter.

I thought the Qt docs were implying the latter when they really only meant the former. 


- Simeon


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


On Oct. 27, 2012, 6:29 p.m., Simeon Bird wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107082/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2012, 6:29 p.m.)
> 
> 
> Review request for Nepomuk, Vishesh Handa and Sebastian Trueg.
> 
> 
> Description
> -------
> 
> Some fairly trivial misc improvements to the filewatch service. Probably don't make a big difference, but probably a good idea. 
> 
> - Use QReadWriteLock instead of QMutex in FileIndexerConfig, thus allowing multiple threads to call shouldFolderBeIndexed at once (not that we really do that right now).
> 
> - Add the IN_EXCL_UNLINK inotify flag. On recent (2.6.36) kernels, this means we don't generate events for files once
> they have been unlinked from the directory we are watching. Prevents waking up for some already-deleted temporary files.
> 
> 
> Diffs
> -----
> 
>   services/fileindexer/fileindexerconfig.h 7debaf3 
>   services/fileindexer/fileindexerconfig.cpp 5226a79 
>   services/filewatch/kinotify.h 6e3f1c0 
>   services/filewatch/kinotify.cpp 9868b90 
> 
> Diff: http://git.reviewboard.kde.org/r/107082/diff/
> 
> 
> Testing
> -------
> 
> Compiled, ran for a bit, didn't seem to break anything. 
> 
> 
> Thanks,
> 
> Simeon Bird
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/nepomuk/attachments/20121029/60420b5b/attachment-0001.html>