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

[René J.V. Bertin]

rjvbb marked 8 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

> mwolff wrote in abstractfilemanagerplugin.cpp:409
> yes, direct commit is fine, but now that you access the watcher multiple times, ensure you cleanup the code:
> 
>   auto watcher = m_watchers.value(folder->project());
>   Q_ASSERT(watcher);
>   if (!watcher->restartDirScan(path)) {
>       // warning + comment
>       watcher->addDir(path);
>   }

So, what branch (this shouldn't break anything in 5.2) ?

> rjvbb wrote in abstractfilemanagerplugin.h:106
> I don't think it's any more unsafe than it already was; I don't see anything that suggests that using a KDirWatch once from another thread makes it unsafe to use from its main thread afterwards. Or rather, concurrently from multiple threads because that's what the mutex protects against.
> Maybe David can tell us otherwise, but a priori it should be enough to add a warning in the comment that KDirWatch isn't thread-safe.
> 
> I've tried my benchmark app without the mutex locally, and managed to reproduce the double-free crash quite regularly (on Linux). I think it would have been observed in the wild by now if the result from `projectWatcher` was used concurrently.

You are concerned that the existing code that uses the getter might end up using the dirwatcher at the same time the filemanagerlistjob uses it?

In that case we'll need to export the ProjectWatcher class, and upgrade all code where the getter is used.

This would be much less of a concern if KDirWatch::addDir and KDirWatch::removeDir were virtual, right? That might be an easier change to get through upstream.

> rjvbb wrote in abstractfilemanagerplugin.h:114
> Why do what?
> 
> It's part of what happens when closing a project in KDevelop, but it isn't actually closing the project. It's telling the file manager to forget about the project. There is no current use case for the method in KDevelop itself, it's only for the benchmark. I tried to get the signal sent through the ProjectController (`projectController()->closeProject(m_project)`) but that seems to need more setting up. I don't have time to figure out what and if that is going to add noise to the benchmark (parser overhead, for instance).
> 
> The `detach` method could be useful in a mode where you want to keep a project open for quick reference (or snatching some code snippets) only and don't want to waste resources on parsing etc.

If someone wants to take this over, by all means, feel free.

I've renamed the method to `projectClosing`. If you don't want to export the method at all we'll just do without benchmarking the dirwatcher dtor.

> rjvbb wrote in abstractfilemanagerpluginimportbenchmark.cpp:158
> Well, you didn't clarify that so I did what cost me the least time (for the initial implementation). I'm ok with adding a benchmark first as long as that can also go into the 5.2 branch, along with any API additions like `AbstractFileManagerPlugin::detach()` (or however we'll be calling it).
> 
> I was kind of hoping that everyone who actually uses the benchmark would see the obvious performance improvement when using `KDIRWATCH_METHOD=QFSWatch` during this review process, which would remove the need to commit the BENCHMARK_ORIGINAL_DIRWATCHER feature.
> 
> It *was* the idea to use the benchmark before committing it, no?

OK, then let's get this ripe for cutting up and proceed from there.

> rjvbb wrote in abstractfilemanagerpluginimportbenchmark.cpp:172
> Sorry, what's the difference?

Not exactly the question I asked so I hope this is what you want.

REPOSITORY
  R32 KDevelop

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

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