D7995: KDevelop: address dirwatching inefficiency (WIP/PoC)

[René J.V. Bertin]

rjvbb added a comment.


  About that benchmark: if a properly adapted fork of the abstractfilemanagerprojectimport test is NOT what you want you'll have to describe what you do want in a much more detailed fashion, so I can see if it's an effort I can accept to make. Not because I'm lazy, this really has been keeping me from doing more urgent things already.
  
  @Gleb Popov: maybe you have ideas and some been-there-done-that experience to help out here? I'm getting the impression I'm doing this only for my own benefit but I'd like to hope that's not the case ...

INLINE COMMENTS

> mwolff wrote in abstractfilemanagerplugin.cpp:297
> this one needs to stay in master, it added new files to the folder

What does "needs to stay in master" mean? (I'm working on the 5.2 branch.) And why would it (this method) go adding new files to the folder?

New files that are created while the project is open are signalled as a "dirty" parent folder, which this method handles by queuing the folder for re-reading. That handles new files, as can easily be proven in real-world usage. What scenario am I missing according to you?

> mwolff wrote in abstractfilemanagerplugin.cpp:409
> it worked before, is this really required now? if not, you can make that a separate patch if you think it solves an issue. to me it looks like a blind change that has nothing to do with the actual thing you are trying to fix here

As stated in the comment, I don't know if we (read I) can be certain the directory to restart will always be monitored already.

I suspect there is a slightly bigger chance with the proposed dirwatching model that we end here for a new directory that somehow hasn't been processed by the dirty handler yet. I'll put a qWarning and run that locally for a bit to see if it ever triggers.

I made this change because it seemed like a simple and cheap failsafe, using a KDW feature that was undoubtedly implemented for a reason.

> mwolff wrote in projectwatcher.cpp:33
> a static mutex? why? make it one mutex per dirwatch
> 
> also call it m_mutex, not barrier

No, that doesn't work properly across all platforms. Remember how I said KDirWatch uses a single QFileSystemWatcher instance to handle all KDirWatch instances using the QFSW method?

Because of that this code should never be executed in parallel, and I wanted to use a name that explains a bit better what the thing does than just `m_mutex`.

KDW could use a static mutex internally to protect its QFSW instance but we'd still be needing a mutex here so I'd propose to stick with a single, static external mutex for now. `KDW::addDir()` of a singe directory is cheap and fast compared to the other things going on, and chances that this method is being called concurrently are slim anyway so the potential performance penalty should be negligible.

As mentioned elsewhere: I've only seen the effects of not using a static mutex on Mac. Without mutex or with non-static mutex instances I get a significant increase in import times (variable but up to about 4x) and sometimes deadlocks occur (deep in the FSEvents backend to QFileSystemWatcher). This happens only when loading multiple projects that have a large number of almost empty directories, IOW, that call KDW::addDir() in rapid succession from multiple threads.

The only possible effect I've seen on Linux of not using a static mutex is an almost anecdotal double free deep in the KDW implementation. I haven't even been able to reproduce it but I'd never seen that happen before in all the years that I've been using KDevelop so I think that was some kind of race condition.

> mwolff wrote in projectwatcher.h:38
> don't pimple, this class is not going to be exported

No, but this approach also allows to tinker with the ProjectWatching internals without having to rebuild more than ProjectWatcher.cpp . I'll get rid of it in due time.

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D7995

To: rjvbb, #kdevelop, mwolff
Cc: dfaure, mwolff, brauch, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170927/23d407d9/attachment-0001.html>