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

[Milian Wolff]

mwolff added inline comments.

INLINE COMMENTS

> rjvbb wrote in abstractfilemanagerplugin.cpp:297
> 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?

This patch should target master, not 5.2. It's highly experimental.

Then see above, remove the dead code in master in a separate patch, but keep this part of the loop. Then rebase your patch remove the rest of this loop, too.

> rjvbb wrote in abstractfilemanagerplugin.cpp:409
> 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.

Well, then do it in a separate patch - I'm not going to oppose it. But it's not related to the bigger change set here at all.

> rjvbb wrote in projectwatcher.cpp:33
> 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.

Can you please clarify this:

Are you saying different KDirWatcher instances share the same QFileSystemWatcher? If so, that needs to be changed upstream, but I'd be OK with working around that for now.

> rjvbb wrote in projectwatcher.h:38
> 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.

Sure, but it's against our policy. We only pimpl public stuff.

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/89667a0d/attachment.html>