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

[Milian Wolff]

mwolff added inline comments.

INLINE COMMENTS

> 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.

The dir watcher **was not accessed concurrently** before this patch. As such, you are now responsible to ensure all the old code that wasn't written with concurrency in mind plays along.

> 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.

You can, and should, disable the background parser in the benchmark. I don't see why a file manager should be able to "forget about the project". That should only happen when the project is closed, so do close the project.

Also, please for the love of $deity stop saying "you don't have time". I also have other things to do than to review your code. If you don't have time we can and should stop this whole process here and there. Either you attend to the code review, or you don't. If the latter, don't contribute to KDevelop.

> rjvbb wrote in filemanagerlistjob.cpp:108
> Did I misunderstand, doesn't this loop add the contents of the folder at `path`?

Yes, you did. This is a `std::transform`. It just creates an `UDSEntry` from a `QFileInfo`. The former is then handled from `handleResults`.

> rjvbb wrote in abstractfilemanagerpluginimportbenchmark.cpp:138
> The enumerator doesn't specify values; is there a standard that guarantees that such an enum always counts from 0 by 1?

yes

> 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?

the steps should be (all separate patches and reviews):

1. cleanup code (i.e. the smaller stuff you found here and there that is unrelated to the performance issue)
2. add benchmark
3. improve performance

so once we have added the benchmark upstream, we can easily compare the impact of the performance patch by running the benchmark with and without your patch. This also allows you to add actual hard numbers to your performance patch, along the lines of "this patch improves the runtime of the benchmark: Before, the numbers measured where X, Y and Z. With this patch, the numbers decrease to X', Y' and Z'".

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

one is a qt-ism, the other is modern c++

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/1867a2f0/attachment.html>