KDevelop: address dirwatching inefficiency (WIP/PoC)
AbandonedPublic

Authored by rjvbb on Sep 26 2017, 4:40 PM.

Details

Reviewers
mwolff
Group Reviewers
KDevelop
Summary

Cf. https://bugs.kde.org/show_bug.cgi?id=384880

This provides a PoC/WIP patch that shows a possible way to improve the current inefficiency in the dirwatching approach. That current approach consists of calling KDirWatch::addDir(<project source dir>) with the instruction to add all files and directories recursively.

This happens on the main thread and atomically, thus blocking the event loop for large projects. See D7742 for timing examples, and D7745 for a companion patch that adds temporary code for project import timing.

This patch installs watchers only on directories, and "online" during the import process itself, by FileManagerListJob. It still contains a bit of code allowing to turn off dirwatching; please note that this is only to allow evaluating the overhead of this new approach.

There's a mutex acting as a barrier to serialise access to the underlying QFileSystemWatcher object used by KDirWatch (when it uses QFSW); this change can of course be proposed upstream. It seems to be required despite Qt's claim that the class is reentrant. On Mac import times can be longer without it and on Linux I've seen sporadic memory allocation (and freeing) errors without it.

Test Plan

Works as intended: the time overhead of creating and populating a dirwatcher is much smaller - and dirwatching now actually works on Mac.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
mwolff added inline comments.Sep 28 2017, 9:20 AM
kdevplatform/project/tests/abstractfilemanagerpluginimportbenchmark.cpp
48

are you only changing visibility here? if so, use using instead, or better yet - add the AFMPBenchmark as friend class to the AFMP

76

de-inline most of this by returning early

if (!m_project) {
    return;
}
158

I think you misunderstood me. I don't want to keep the benchmark of the old implementation around. What I want is a simple benchmark (separate patch!) that can be run on the current implementation. Then when one applies your optimization patch, it should show a performance improvement of the benchmark.

172

range-based for

This revision now requires changes to proceed.Sep 28 2017, 9:20 AM

I also think by the way that this shouldn't go to 5.2, sorry ... 5.2 is fragile enough as it is on the platforms we already claim to be stable on. Let's put it in master and instead try to keep the time to 5.3 short-ish.

rjvbb added inline comments.Sep 28 2017, 10:16 AM
kdevplatform/project/abstractfilemanagerplugin.cpp
127

fine with me, someone suggested I add the conditional ;)

kdevplatform/project/abstractfilemanagerplugin.h
106 ↗(On Diff #20192)

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.

111 ↗(On Diff #20192)

I agree it's lacking from KDirWatch, but can't we develop the change here first and then see what needs to be upstreamed?

114 ↗(On Diff #20192)

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.

kdevplatform/project/filemanagerlistjob.cpp
106

Did I misunderstand, doesn't this loop add the contents of the folder at path?

kdevplatform/project/tests/abstractfilemanagerpluginimportbenchmark.cpp
48

Yes, just visibility change. I didn't think it was OK to add a (private) test class as a friend to public API. Will do that, then.

138

The enumerator doesn't specify values; is there a standard that guarantees that such an enum always counts from 0 by 1?

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?

172

Sorry, what's the difference?

Yeah, and I'm using it on such a platform because I can no longer remain on 5.1 if I want to keep up with development ...
I don't mind if this gets committed to master, as long as I can develop and test against the branch I'm using (and preferably, if any smaller, preparatory changes that don't affect the current behaviour *can* be committed in 5.2 too).

mwolff added inline comments.Sep 28 2017, 1:22 PM
kdevplatform/project/abstractfilemanagerplugin.h
106 ↗(On Diff #20192)

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.

114 ↗(On Diff #20192)

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.

kdevplatform/project/filemanagerlistjob.cpp
106

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

kdevplatform/project/tests/abstractfilemanagerpluginimportbenchmark.cpp
138

yes

158

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

172

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

rjvbb marked 8 inline comments as done.Sep 28 2017, 2:08 PM
rjvbb added inline comments.
kdevplatform/project/abstractfilemanagerplugin.cpp
407

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

kdevplatform/project/abstractfilemanagerplugin.h
106 ↗(On Diff #20192)

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.

114 ↗(On Diff #20192)

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.

kdevplatform/project/tests/abstractfilemanagerpluginimportbenchmark.cpp
158

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

172

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

rjvbb updated this revision to Diff 20039.Sep 28 2017, 2:33 PM
rjvbb marked an inline comment as done.

revision mark N+1

Hopefully there remain only a few trivial things (or rewinds) before we can 1) cut this up and 2) commit the benchmark with only a formal review.

rjvbb set the repository for this revision to R32 KDevelop.Sep 28 2017, 2:33 PM
mwolff requested changes to this revision.Sep 28 2017, 3:50 PM

Can you please put the benchmark into a separate review as I requested? It must be self-contained, the "do stuff in background" change should then depend on that revision.

kdevplatform/project/abstractfilemanagerplugin.cpp
126

you removed the wrong conditional - I added the comment to this line, the if for the m_watchers.contains() - this should be removed, _not_ the #ifdef for the actual timing feature (that should remain optional)

This revision now requires changes to proceed.Sep 28 2017, 3:50 PM
mwolff added inline comments.Sep 28 2017, 3:54 PM
kdevplatform/project/abstractfilemanagerplugin.cpp
297 ↗(On Diff #22412)

sigh, lost in translation once more... With "keep it as-is" I meant keep your version that removed this altogether. As I said, I misunderstood this code initially - you are right in that this loop should be removed now that you listen to dirty instead of created.

407

5.2 is fine with me

rjvbb marked 4 inline comments as done.Sep 28 2017, 9:00 PM
rjvbb added inline comments.
kdevplatform/project/abstractfilemanagerplugin.cpp
297 ↗(On Diff #22412)

I think it just sounded different (clearer) in your head.
(we could try you speaking German and me Dutch but I doubt that'll improve anything) ;)

kdevplatform/project/abstractfilemanagerplugin.h
106 ↗(On Diff #20192)
rjvbb updated this revision to Diff 20064.Sep 28 2017, 9:01 PM
rjvbb marked an inline comment as done.

One last update before splitting off the benchmark.

rjvbb set the repository for this revision to R32 KDevelop.Sep 28 2017, 9:02 PM
rjvbb added a comment.Sep 29 2017, 1:13 AM

Re: the risk of using the unprotected result of AFMP::projectWatcher(). I've only found it being used in QMakeProjectManager::import(), and then only to connect another slot to the dirty signal. That slot doesn't do anything with the dirwatcher itself.

With this few users it will be trivial to update them (it) to use ProjectWatcher instead of KDirWatcher.

rjvbb updated this revision to Diff 20162.Sep 30 2017, 3:51 PM

This modified version of the patch follows a suggestion from the Qt ML, using a FileManagerListJob::watchDir(path) signal connected to a slot (lambda) in AbstractFileManagerPluginPrivate with Qt::QueuedConnection. This removes the need for a mutex since the watcher instance is now used on a single thread exclusively (in the code I touch).

In addition I now export the project filter to the ProjectWatcher class, which makes it possible to ignore add requests for folders that the project manager is supposed to ignore. (I assume there is no point in watching such folders, correct me if I'm wrong.)
Typical threspassers are the .git and build folders; not adding them speeds up dirwatcher set-up even more (and minimises resource usage).

rjvbb set the repository for this revision to R32 KDevelop.Sep 30 2017, 3:52 PM
rjvbb updated this revision to Diff 20192.Oct 1 2017, 4:38 PM

incorporates most of the requests from D8059

rjvbb set the repository for this revision to R32 KDevelop.Oct 1 2017, 4:39 PM

Note: I'll refrain from reviewing this until we finished with D8059 and the spin-off that then introduces the project watcher to apply the filter early. Once we have those two patches in, we can assess this patch here properly.

Sure. I'm just keeping this up to date and in sync.

rjvbb updated this revision to Diff 22412.Nov 15 2017, 7:31 PM

This is the updated version of my proposal, rebased on the master branch.

To summarise:

  • the KDirWatch instance is wrapped in an instance of a new ProjectWatcher class, ignoring duplicate watching requests and keeping track of the number of watched directories
  • the FileManagerListJob emits a watchDir signal when it visits a directory (which only happens when it's not rejected by the project filter.
  • the AbstractFileManagerPlugin receives this signal, adding individual directories of interest to the dirwatcher.

This construction prevents race conditions that would otherwise require a global mutex, More importantly, it also "inlines" the dirwatcher population, removing the need for an atomic, uninterruptable and potentially (very) long action on the main thread, blocking the GUI (the original issue this patch is meant to address).

The resulting project no longer watches all items (dirs AND files) under the project directory, but only the directories that are not filtered out. I think it is thus relevant to output the number of watched directories in the benchmark I'd add something like "of N directories total" but haven't been able to figure out if that information is even available from the project or project controller.

rjvbb set the repository for this revision to R32 KDevelop.Nov 15 2017, 7:31 PM
mwolff requested changes to this revision.Nov 16 2017, 10:55 AM

I still think the number of watched directories is completely useless information. The total time is interesting, and whether the unit tests still work that ensure the correct dirs are watched. This means: remove the project watcher class, keep using dirwatcher directly.

kdevplatform/project/abstractfilemanagerplugin.cpp
1

remove

155

please document why queuing is required here? Qt should do this automatically, if you emit the signal from a background thread. I.e. it takes QThread::currentThread into account, and not the sender object's thread.

440

only when this is a local path

This revision now requires changes to proceed.Nov 16 2017, 10:55 AM

Milian Wolff wrote on 20171116::10:55:31 re: "D7995: KDevelop: address dirwatching inefficiency (WIP/PoC)"

I **still** think the number of watched directories is completely useless information. The total time is interesting, and whether the unit tests still work that ensure the correct dirs are watched. This means: remove the project watcher class, keep using dirwatcher directly.

We still disagree on that subject then and I can still keep the benchmark crippled if that's what you want. Did you actually read the argument why I reintroduced this output, how total time is now dependent on a number of watched directories that is no longer an external given but is now essentially an *un*controlled variable.

Consider 2 directories with exactly the same number of files and directories. One has a substantial subtree under a folder that will be filtered out, the other not. Which one do you think will import faster, and do you think you'll understand that difference at a glance from benchmark output that doesn't includes total time but not the number of timed operations?

Ultimately I don't care because I don't write that benchmark for myself - but removing this output will be done under protest and I'll be obliged to leave some trace of that in a comment.

Now, the ProjectWatcher class. This one doesn't only keep count of the number of watched directories. It also prevents directories from being added multiple times which leads to unclear behaviour inside KDirWatch and may ultimately lead to directories that aren't unwatched when necessary. So it'll have to stay.

please document why queuing is required here?

Will do, but that'll just say it's following good-practice instructions from Qt devs. Qt is indeed supposed be able to detect when queued connections are required but it cannot do so 100% reliably and this is a case where I've seen it go wrong

(You may not remember, but I also tested making the other connections queued, which shouldn't a prior have any impact AFAIU. It did: caused lock-ups.)

In D7995#168405, @rjvbb wrote:

Milian Wolff wrote on 20171116::10:55:31 re: "D7995: KDevelop: address dirwatching inefficiency (WIP/PoC)"

I **still** think the number of watched directories is completely useless information. The total time is interesting, and whether the unit tests still work that ensure the correct dirs are watched. This means: remove the project watcher class, keep using dirwatcher directly.

We still disagree on that subject then and I can still keep the benchmark crippled if that's what you want. Did you actually read the argument why I reintroduced this output, how total time is now dependent on a number of watched directories that is no longer an external given but is now essentially an *un*controlled variable.

Consider 2 directories with exactly the same number of files and directories. One has a substantial subtree under a folder that will be filtered out, the other not. Which one do you think will import faster, and do you think you'll understand that difference at a glance from benchmark output that doesn't includes total time but not the number of timed operations?

The benchmark changes its performance based on the commit which hopefully improves the performance. The commit message also includes the numbers and shows the advantage of the new behavior.

Ultimately I don't care because I don't write that benchmark for myself - but removing this output will be done under protest and I'll be obliged to leave some trace of that in a comment.

I won't accept it with traces of useless stuff in it.

Now, the ProjectWatcher class. This one doesn't only keep count of the number of watched directories. It also prevents directories from being added multiple times which leads to unclear behaviour inside KDirWatch and may ultimately lead to directories that aren't unwatched when necessary. So it'll have to stay.

Unclear behavior? That needs to be fixed in KDirWatch upstream then.

please document why queuing is required here?

Will do, but that'll just say it's following good-practice instructions from Qt devs. Qt is indeed supposed be able to detect when queued connections are required but it cannot do so 100% reliably and this is a case where I've seen it go wrong

FUD. I'm a Qt dev, and I told you how it works which should be reliable for your purpose here.

(You may not remember, but I also tested making the other connections queued, which shouldn't a prior have any impact AFAIU. It did: caused lock-ups.)

If you make stuff queued it does change everything - i.e. direct signal emission will never happen, even when the threads are the same.

rjvbb added a comment.Nov 16 2017, 1:20 PM

Milian Wolff wrote on 20171116::12:22:21 re: "D7995: KDevelop: address dirwatching inefficiency (WIP/PoC)"

I won't accept it with traces of useless stuff in it.

And I won't have my name associated with something that's not to par with my own standards. At the very least I'll have to remove my (C) notice.

Unclear behavior? That needs to be fixed in KDirWatch upstream then.

The unclearness yes, but it's not up to us to decide whether KDW::addDir() should ignore duplicates or keep some kind of internal track as it appears to be doing.

The proposed improvement adds the directory to the watched list each time that directory is loaded and reloaded. That's unnecessary and that should be enough reason to verify if a directory is already watched before handing it off to KDW::addDir(), and one logical place to do that is in ProjectWatcher .

FUD. I'm a Qt dev, and I told you how it works which should be reliable for your purpose here.

That has turned out not to be the case, I already told you I've seen deadlock-like situations.

Citing Konrad Rosenbaum from the development ML:

> With Qt there are some simple rules:
<snip> 
> For calls across threads use QueuedConnection explicitly.. (In most cases
> Qt detects this automatically, but there are some special cases usually
> involving moveToThread().)
<snip>
> Do not make any assumptions about the order of signals or slots. Try to
> avoid the assumption that a signal only returns after the slot has been
> executed (or that it returns immediately) - you'll mess with connection
> settings soon enough after you forgot that you rely on it. Regard signals
> as fire-and-forget-but-maybe-take-some-time-with-unknown-effects.
> 
> The combination of these will ensure that your application behaves in an
> acceptable manner.

That's not FUD, that's common sensical defensive programming (taking the form of "let's add a flag that normally should be added automatically"). I frankly don't understand why we're even having this discussion.

rjvbb added a comment.Nov 17 2017, 4:14 PM

I misremembered the exact reason I use an explicit queued connection. Some results on Linux, connecting FMLJ::watchDir without explicitly queued connection:

KDirWatch backend: Inotify
Starting import of project kdevelop-git
        creating dirwatcher took 0 seconds
Starting import of project /opt/local/site-ports
        creating dirwatcher took 0 seconds
Starting import of project /opt/local/linux-ports
        creating dirwatcher took 0 seconds
importing 3894 items into project #2 with 617 call(s) to KDirWatch::addDir() took 0.827 seconds
importing 5987 items into project #1 with 1189 call(s) to KDirWatch::addDir() took 1.321 seconds
importing 13361 items into project #0 with 2675 call(s) to KDirWatch::addDir() took 2.258 seconds
kdevplatform.filemanager: Deleting dir watcher took 0.026 seconds for project "Test Project"
kdevplatform.filemanager: Deleting dir watcher took 0.01 seconds for project "Test Project"
kdevplatform.filemanager: Deleting dir watcher took 0.005 seconds for project "Test Project"
Done in 2.259 seconds total
org.kde.kwindowsystem: Loaded plugin "/opt/local/share/qt5/plugins/kf5/org.kde.kwindowsystem.platforms/KF5WindowSystemX11Plugin.so" for platform "xcb"
cannot find .rc file "abstractfilemanagerpluginimportbenchmarkui.rc" for component "abstractfilemanagerpluginimportbenchmark"
KDirWatch backend: Inotify
Starting import of project kdevelop-git
        creating dirwatcher took 0 seconds
Starting import of project /opt/local/site-ports
        creating dirwatcher took 0 seconds
Starting import of project /opt/local/linux-ports
        creating dirwatcher took 0 seconds
importing 3894 items into project #2 with 617 call(s) to KDirWatch::addDir() took 0.972 seconds
importing 5987 items into project #1 with 1189 call(s) to KDirWatch::addDir() took 1.322 seconds
importing 13361 items into project #0 with 2675 call(s) to KDirWatch::addDir() took 2.315 seconds
kdevplatform.filemanager: Deleting dir watcher took 0.026 seconds for project "Test Project"
kdevplatform.filemanager: Deleting dir watcher took 0.009 seconds for project "Test Project"
kdevplatform.filemanager: Deleting dir watcher took 0.005 seconds for project "Test Project"
Done in 2.316 seconds total
org.kde.kwindowsystem: Loaded plugin "/opt/local/share/qt5/plugins/kf5/org.kde.kwindowsystem.platforms/KF5WindowSystemX11Plugin.so" for platform "xcb"
cannot find .rc file "abstractfilemanagerpluginimportbenchmarkui.rc" for component "abstractfilemanagerpluginimportbenchmark"
KDirWatch backend: Inotify
Starting import of project kdevelop-git
        creating dirwatcher took 0 seconds
Starting import of project /opt/local/site-ports
        creating dirwatcher took 0 seconds
Starting import of project /opt/local/linux-ports
        creating dirwatcher took 0 seconds
importing 3894 items into project #2 with 617 call(s) to KDirWatch::addDir() took 0.818 seconds
importing 5987 items into project #1 with 1189 call(s) to KDirWatch::addDir() took 1.341 seconds
importing 13361 items into project #0 with 2675 call(s) to KDirWatch::addDir() took 2.354 seconds
kdevplatform.filemanager: Deleting dir watcher took 0.019 seconds for project "Test Project"
kdevplatform.filemanager: Deleting dir watcher took 0.011 seconds for project "Test Project"
kdevplatform.filemanager: Deleting dir watcher took 0.005 seconds for project "Test Project"
Done in 2.355 seconds total
org.kde.kwindowsystem: Loaded plugin "/opt/local/share/qt5/plugins/kf5/org.kde.kwindowsystem.platforms/KF5WindowSystemX11Plugin.so" for platform "xcb"
cannot find .rc file "abstractfilemanagerpluginimportbenchmarkui.rc" for component "abstractfilemanagerpluginimportbenchmark"
KDirWatch backend: Inotify
Starting import of project kdevelop-git
        creating dirwatcher took 0 seconds
Starting import of project /opt/local/site-ports
        creating dirwatcher took 0 seconds
Starting import of project /opt/local/linux-ports
        creating dirwatcher took 0 seconds
importing 3894 items into project #2 with 617 call(s) to KDirWatch::addDir() took 2.486 seconds
importing 5987 items into project #1 with 1189 call(s) to KDirWatch::addDir() took 6.355 seconds
importing 13361 items into project #0 with 2675 call(s) to KDirWatch::addDir() took 12.064 seconds
kdevplatform.filemanager: Deleting dir watcher took 0.02 seconds for project "Test Project"
kdevplatform.filemanager: Deleting dir watcher took 0.009 seconds for project "Test Project"
kdevplatform.filemanager: Deleting dir watcher took 0.006 seconds for project "Test Project"
Done in 12.065 seconds total

 Performance counter stats for 'kdevld-lnx-work/build/kdevplatform/project/tests/abstractfilemanagerpluginimportbenchmark kk-git/ /opt/local/site-ports/ /opt/local/linux-ports/' (5 runs):

       5300.862525      task-clock:u (msec)       #    0.766 CPUs utilized            ( +-  7.38% )
                 0      context-switches:u        #    0.000 K/sec                  
                 0      cpu-migrations:u          #    0.000 K/sec                  
            17,837      page-faults:u             #    0.003 M/sec                    ( +-  0.04% )
     5,006,341,532      cycles:u                  #    0.944 GHz                      ( +-  0.86% )
     2,406,178,384      instructions:u            #    0.48  insn per cycle           ( +-  0.11% )
       554,704,041      branches:u                #  104.644 M/sec                    ( +-  0.10% )
        28,264,799      branch-misses:u           #    5.10% of all branches          ( +-  1.00% )

       6.920373566 seconds time elapsed                                          ( +- 28.17% )

Notice the last run which all of a sudden took much longer. This sort of even happened to me twice in a row, without any apparent reason like another application hogging the CPU or preempting the disk (as far as I can tell). It's not something I can reproduce at will (and it used to be easier to observer in earlier versions of this patch) but I've never seen it when using an explicitly queued connection.

How would Qt detect here that a queued connection is required? How can it know that the FileManagerListJob allocated in the same thread as the call to connect() will send the watchDir signal from another thread?

In D7995#169067, @rjvbb wrote:

I misremembered the exact reason I use an explicit queued connection. Some results on Linux, connecting FMLJ::watchDir without explicitly queued connection:

<snip>

Notice the last run which all of a sudden took much longer. This sort of even happened to me twice in a row, without any apparent reason like another application hogging the CPU or preempting the disk (as far as I can tell). It's not something I can reproduce at will (and it used to be easier to observer in earlier versions of this patch) but I've never seen it when using an explicitly queued connection.

I think by now you should know my answer to this question: Use a profiler and figure out what's going on. Heck, even attaching a debugger and intermittently interrupting it manually should give you an idea of what's going on. It shouldn't be the difference between Auto and Queued connection though, since both should yield the same result, minus the code check below which is not explaining such a huge order-of-magnitude runtime performance difference.

How would Qt detect here that a queued connection is required? How can it know that the FileManagerListJob allocated in the same thread as the call to connect() will send the watchDir signal from another thread?

https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qmetaobject.cpp.html#1497

object is the receiver object, currentThread is the thread from where the signal is being emitted. This is not rocket science.

rjvbb added a comment.Nov 20 2017, 9:56 AM
I think by now you should know my answer to this question: Use a profiler and figure out what's going on. Heck, even attaching a debugger and intermittently interrupting it manually should give you an idea of what's going on.

Actually, I did attach a debugger back when the issue was more frequent and interrupted at intervals. That's what I base my hypothesis on that the connection type is involved, but the underlying mechanism is way too complex and asynchronous for me to follow what's going on. And currently far too intermittent to use tools which will probably interfere with timing in ways that turn this even more into a Heisenbug.
I far prefer to approach it as a black box and follow common-sense best-coding practices. Now if you have proof that an explicit connection type spec isn't needed, fine - I can always reintroduce it in my own fork.

It shouldn't be the difference between Auto and Queued connection though, since both should yield the same result

The key word here is "should", no?

This is not rocket science.

Multi-threaded programming with async events might actually be worse if it also wants no to check for Murphy at every other step 8-)

It's not "asynchronous and complex", it's just a queued connection. It posts an event to the receiving thread, and when that thread next runs its event loop, the slot gets invoked. As Milian says, it's not rocket science.

Are you aware of https://bugreports.qt.io/browse/QTBUG-50700, maybe that is related to your experience? Also, clearing the disk cache via "echo 1 > /proc/sys/vm/drop_caches" sometimes helps with inconsistent benchmark results ...

rjvbb added a comment.Nov 20 2017, 1:19 PM
It's not "asynchronous and complex", it's just a queued connection.

Queued just means that signals are processed in order, not synchronously, right? (There's another connection type for that IIUC.)

Taken in isolation ("in vitro") you may be right, but if you put this in real-world usage ("in vivo") it becomes a whole different matter.

I remember that the last time I had a project import hanging (on Mac) *none* of the numerous threads were doing anything that seemed even remotely related to project import.

As I said, I'll remove the explicit connection spec.

Are you aware of https://bugreports.qt.io/browse/QTBUG-50700, maybe that is related to your experience?

I can't exclude it in the full app but the benchmark doesn't link to QtDeclarative and the issue appears to be Linux/X11 specific. Does it still apply to Qt 5.8?

Also, clearing the disk cache via "echo 1 > /proc/sys/vm/drop_caches" sometimes helps with inconsistent benchmark results ...

That could make sense to do before each benchmark invocation but again only on Linux.

FWIW, I'm using ZFS on Linux, and even the benchmark's project import does something at the level of the duchain cache (= write to disk). This could also explain the freak result I posted, somehow (IOW doing a manual sync before running the benchmark might make just as much sense as writing to drop_caches).

No, Queued means they are queued in the receiving object's thread's event loop, i.e. the slots are executed in that thread when it re-enters its event loop. Direct means the slots are called immediately, in the sender's thread. There are no other connection types.

rjvbb updated this revision to Diff 22667.Nov 20 2017, 3:51 PM

modified as demanded.

rjvbb set the repository for this revision to R32 KDevelop.Nov 20 2017, 3:51 PM
rjvbb marked 3 inline comments as done.Nov 20 2017, 3:54 PM
mwolff added inline comments.Nov 23 2017, 1:27 PM
kdevplatform/project/abstractfilemanagerplugin.cpp
153 ↗(On Diff #20192)

on a more serious note: your crash probably arises from this being bound to the AFMP lifetime - it won't get disconnected when the watcher gets killed. You probably want to use watcher instead of q as third param here.

still, doesn't change the fact that the whole watcher wrapper should be removed

rjvbb added a comment.EditedNov 23 2017, 2:10 PM

I found and will upload another solution which gets rid of the whole signal/slot overhead and potential risk of delivering signals to a stale object. Hand off a pointer to the watcher instance to the FIleManagerListJob instance and do the addDir() call directly from there.
Should maybe use a QSharedPointer or similar wrapper but that's a later concern.

still, doesn't change the fact that the whole watcher wrapper should be removed

No. Reasons given before that I'm not going to repeat - it should be even clearer now that KDW::addDir() shouldn't be called directly each time a directory is (re)loaded.
EDIT: calling it directly without a reimplementation in the ProjectWatcher class that checks if the directory isn't already being watched. Not "directly as opposed to indirectly via a signal/slot indirection".

EDIT2: the dirwatcher could be made part of iProject since it's supposed to be a crucial project feature, no? iProject::watchDir(), iProject::unWatchDir() (and ditto for files) don't feel wrong, could do all the required checks and would allow to remove all the AFMP::m_watchers housekeeping. But that means AbstractFileManagerPlugin::projectWatcher() would have to be replaced and its users refactored.

There's another concern however:
https://bugs.kde.org/show_bug.cgi?id=387238

In D7995#171218, @rjvbb wrote:

I found and will upload another solution which gets rid of the whole signal/slot overhead and potential risk of delivering signals to a stale object. Hand off a pointer to the watcher instance to the FIleManagerListJob instance and do the addDir() call directly from there.
Should maybe use a QSharedPointer or similar wrapper but that's a later concern.

Is the dirwatching feature

still, doesn't change the fact that the whole watcher wrapper should be removed

No. Reasons given before that I'm not going to repeat - it should be even clearer now that KDW::addDir() shouldn't be called directly each time a directory is (re)loaded.

It's not being called directly, it's being called indirectly via the eventloop which is fine as long as you setup the connection properly, which you didn't do here.

There's another concern however:
https://bugs.kde.org/show_bug.cgi?id=387238

rjvbb added a comment.Nov 23 2017, 2:28 PM

It's not being called directly, it's being called indirectly via the eventloop which is fine as long as you setup the connection properly, which you didn't do here.

You misread me, see the edits I made to the comment you were citing.

rjvbb added a comment.Nov 23 2017, 6:59 PM

on a more serious note: your crash probably arises from this being bound to the AFMP lifetime - it won't get disconnected when the watcher gets killed. You probably want to use watcher instead of q as third param here.

All this discussion made me forget why I created a dedicated class in the first place and think could do away with the signal/slot mechanism.

Correct me if I'm wrong, but that's probably not true: the dirwatcher can be fed from the main thread so we either need a locking mechanism or else we need to feed it only from the main thread.

Using watcher as the target object should also prevent it from receiving signals after being deleted. I should have seen that myself (and it was probably a bug), doh.

rjvbb updated this revision to Diff 22858.Nov 23 2017, 9:04 PM

updated to fix the potential signal delivery to a stale watcher instance.

This new approach watches only directories and will thus trigger a full reload of any directory in which something changed. It would be pointless and potentially counterproductive to do this just before all projects will be unloaded; prevent this by connecting KDirWatch::stopScan() to QCoreApplication::aboutToQuit. This should be safe, right?

rjvbb set the repository for this revision to R32 KDevelop.Nov 23 2017, 9:04 PM
mwolff requested changes to this revision.Nov 24 2017, 11:25 AM

I refuse to look at this until the project watcher is gone

This revision now requires changes to proceed.Nov 24 2017, 11:25 AM
rjvbb abandoned this revision.Nov 24 2017, 12:55 PM

Fine then, I'll make it simple for you: let's just keep everything as it is. No one seems to care anyway.

I care, otherwise I wouldn't review. Simplify the code, then we can run the benchmark and see what it brings.

rjvbb added a comment.EditedNov 25 2017, 11:32 PM
I care, otherwise I wouldn't review. Simplify the code, then we can run the benchmark and see what it brings.

I already know what the benchmark brings, and what's more, I know what my implementation brings in actual KDevelop sessions (the good and the bad).

I have tried to explain several times why I think the ProjectWatcher class is justified and why I don't want to use KDirWatch directly. We'll see what feedback I get on my question about the underlying main reason but as long as I'm not convinced KDW::addDir() can be called directly we'll remain in the current stalemate.

Simplifying? I've been doing the opposite since I gave up on upstreaming. I've been adding the simple measures against multiple concurrent reloads I posted elsewhere (too much hassle to maintain that as a separate patch). And a way to opt out and use the old dirwatching mechanism which does perform better for certain kinds of projects even if they take longer to load. That doesn't really make the patch more more complicated of course, and it should make benchmarking old vs. new more trivial (no more rebuilding with or without the patch).

EDIT: plus looking into representing FileManagerListJobs in the runcontroller so there's at least some kind of indication what KDevelop is busy with.

Let me know if any of those are of interest for upstreaming.