Bug 387238 - concurrent project directory reloading
Summary: concurrent project directory reloading
Status: REPORTED
Alias: None
Product: kdevplatform
Classification: Developer tools
Component: project (show other bugs)
Version: unspecified
Platform: Compiled Sources All
: NOR task
Target Milestone: ---
Assignee: kdevelop-bugs-null
URL:
Keywords: triaged
Depends on:
Blocks:
 
Reported: 2017-11-23 14:05 UTC by RJVB
Modified: 2018-09-28 08:06 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
pseudo-patch to catch concurrent reloads (1.74 KB, patch)
2017-11-24 22:20 UTC, RJVB
Details
terminal output while running configure showing several "already queues reload" (7.08 KB, text/plain)
2017-11-24 22:31 UTC, RJVB
Details
pseudo patch (6.87 KB, patch)
2017-11-25 14:57 UTC, RJVB
Details

Note You need to log in before you can comment on or make changes to this bug.
Description RJVB 2017-11-23 14:05:56 UTC
I'm listing this as a task because I'm not yet entirely certain of my analysis.

The CPU burning evoked in #387232 should have been a clear sign: the entire project is being reloaded repeatedly because files keep changing in the project root.

As far as I can see there is no protection whatsoever against doing multiple concurrent reloads; FileManagerListJob happily runs as many copies of itself as requested. That seems wrong and something that should probably be addressed but how?

- abort the current load action before scheduling a reload?
- queue the (re)load until the previous one(s) is/are done?
Comment 1 Milian Wolff 2017-11-23 14:18:46 UTC
- how are the multiple reloads being triggered? i.e. how can this be reproduced?
- do you have a CPU profile that shows what is going on?
- I've never seen this, so before we go down into yet another of you rabbit holes, please make sure what is happening first

once we have established this with hard data, we can start thinking about solutions
Comment 2 RJVB 2017-11-23 16:53:29 UTC
1- I noticed the multiple reloads when I added a tracer to AFMP::eventuallyReloadFolder(). It seems unavoidable that you'd get multiple reloads when a background process causes frequent changes in a directory that's being monitored for changes. 

I observed this with the CodeBlocks source tree, a working copy off SVN which had been configured for a standard in-tree build. Making a change to configure.ac triggers an automatic process (autogen? autoconf + configure?) I'd never noticed before and that I certainly didn't configure myself.
You may remember though that I made earlier mention of frequent reloading when a project was being configured that I also had open in KDevelop.

I think this should be enough information to reproduce the issue.

FWIW here's a warning printed by your own 5.2.0 appimage build while loading the same session:

inotify_add_watch("/path/to/codeblocks/work/trunk/conftest.dir") failed: "No such file or directory"

That's a directory that was yanked out from under the project loader while it was working because of that automatic autoconf whatever-it-is.

And when I quit the appimage build before it had gone idle I saw this:

/tmp/.mount_i2N9LD/AppRun: line 35: 13111 Segmentation fault      (core dumped) kdevelop $@

(no core was dumped, btw)

2- CPU profile. I can try to get one, but how do I get one that doesn't include the initial load? Not sure how useful it'll be in diagnosing exactly what's going on though. I've already seen quite a few hits in Qt regexp. functions when I was interrupting the process earlier today.

3- I've also never really noticed it before. I just don't have that many large projects that a) I build in-tree and b) I have open in KDevelop while doing so. And even then: doing concurrent directory reloads is probably hardly ever a problem other than an unnecessary waste of resources. 

It can become a problem when reloads are triggered rapidly and for a sufficiently long time (and without feedback to the user); the logical explanation for the watchDir signals I received aimed at a stale watcher object would be that the list job somehow continued after the project was unloaded.
Comment 3 RJVB 2017-11-24 08:47:20 UTC
Let me just check if I get this right:

What happens in the current code when new items are created in a project directory? I think this gives rise to reloading that directory (via eventuallyReloadFolder), right?

Another relevant question: which are the file types (extensions) of which we know they're likely to change "behind the user's back"? We should probably watch those directly to avoid getting a dirty signal for their containing directory.
There's "*.kate-swp" which is changed whenever change a bit in an open file, are there others with a similar behaviour?
Comment 4 RJVB 2017-11-24 22:20:58 UTC
Created attachment 109045 [details]
pseudo-patch to catch concurrent reloads

With this in place, I see

- frequent if not systematic concurrent reloads during session startup
- multiple concurrent reloads running configure in an autoconf project (codeblocks) open in KDevelop (in-tree build): see the next attachment.

The frequency increases when using the alternative dirwatching model which only puts watches on directories.

I think the fix should be relatively trivial, along these lines:

- add a new private method eventuallyStartListJob(project,item, newjob) 
- this method scans the current project job list *starting at the end* until it encounters a job that queues a reload for the given project item. 
- if a hit is found, connect that job's finished signal to the new job's start method
- else, start the new job
- this new method replaces all direct calls to job->start()

The it might be safe to abort all jobs that have job->item()==item ; this would certainly speed up the final reload.
Comment 5 RJVB 2017-11-24 22:31:51 UTC
Created attachment 109046 [details]
terminal output while running configure showing several "already queues reload"

this is only a proof of existence, the directories concerned here are tiny and devoid of interest. Similar reloads of project (sub)directories will be more costly.
Comment 6 RJVB 2017-11-25 14:57:56 UTC
Created attachment 109056 [details]
pseudo patch

My previous patch had a glaring error and gave off false alarms. It is probably unlikely that dirwatching ever leads to concurrent reloads with the current dirwatching model, but that model is inappropriate as argued (and IIRC agreed upon) in #384880 .

Using (sub)directory-only dirwatching to implement project/disk sync means every on-disk change will be relayed to the project manager as a KDirWatch::dirty signal for the enclosing directory, triggering a reload of that directory.

There is a CPU cost to the consequence (running multiple KIO jobs; the cost is evident in terms of fan noise in larger projects and shows up as 100% CPU presumably of the main thread). Projects using in-tree builds will thus see competition for CPU cycles for a convenience feature with more crucial features like configuring, compiling and showing the output of such processes.

More importantly: how robust is the project model itself against changes in the same directory being fed in from multiple background directory reloads?

The updated pseudo-patch above explores a few ideas to limit those effects. For each new FileManagerListJob for a given directory, the current project job list is scanned and
- jobs started to reload the same directory are aborted and removed from the joblist
- the directory is unqueued from jobs that have it queued 
- async jobs are started with a small delay so that most are never actually started when the reload requests come in too rapidly. (NB: this doesn't affect the initial load nor reloads requested explicitly via the UI.)

I'll be running this patch in my own fork and I'll welcome constructive feedback.
Comment 7 Andrew Crouthamel 2018-09-28 03:20:35 UTC
Dear Bug Submitter,

This bug has been in NEEDSINFO status with no change for at least 15 days. Please provide the requested information as soon as possible and set the bug status as REPORTED. Due to regular bug tracker maintenance, if the bug is still in NEEDSINFO status with no change in 30 days, the bug will be closed as RESOLVED > WORKSFORME due to lack of needed information.

For more information about our bug triaging procedures please read the wiki located here: https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging

If you have already provided the requested information, please set the bug status as REPORTED so that the KDE team knows that the bug is ready to be confirmed.

Thank you for helping us make KDE software even better for everyone!
Comment 8 RJVB 2018-09-28 08:06:25 UTC
I'm the one waiting (for constructive feedback).

Barring formal proof that it is in fact impossible in "stock" KDevelop code to trigger a reload while another is already in process this a "task issue" that should remain open because it describes a possible undesirable situation. One that can have implications for performance or even lead to a crash.