Bug 372040 - KDevelop editor become unresponsive when clicking into a problem
Summary: KDevelop editor become unresponsive when clicking into a problem
Status: CONFIRMED
Alias: None
Product: kdevelop
Classification: Applications
Component: Language Support: CPP (Clang-based) (show other bugs)
Version: git master
Platform: Compiled Sources Linux
: HI normal
Target Milestone: 5.0.4
Assignee: kdevelop-bugs-null
URL:
Keywords:
: 362037 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-11-03 20:37 UTC by Lukas Jirkovsky
Modified: 2018-12-16 02:54 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
perf record output (949.68 KB, application/octet-stream)
2016-11-03 20:37 UTC, Lukas Jirkovsky
Details
workaround patch (1.06 KB, patch)
2017-01-16 21:31 UTC, RJVB
Details
improved workaround (2.99 KB, patch)
2017-01-30 20:04 UTC, RJVB
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lukas Jirkovsky 2016-11-03 20:37:18 UTC
Created attachment 102016 [details]
perf record output

If I click on an underlined identifier, the editor becomes unresponsive for several seconds. I noticed that when I navigate to the identifier in the editor using keyboard arrows, there is no slowdown and KDevelop is perfectly resnposive the whole time.

This problem happens with a work-related project that can be considered somewhat peculiar. There are about 6000 includes in ~500 subdirectories in the include path. The project itself is only a small subset of sources - the problem occurs even with a small test project that consists only of a single file that includes only few of the includes, though it becomes worse with larger projects. The sources are mounted using samba from a locally running virtual machine.

Attached is a profile generated using the linux perf utility with the following command:

perf record -g -p <PID> sleep 3

which I executed just before clicking on a problem in the editor.
Comment 1 Sven Brauch 2016-11-03 20:39:02 UTC
Is your home directory on a network share?
Comment 2 Lukas Jirkovsky 2016-11-03 20:42:54 UTC
The home directory is on a SSD, so the speed is decent.

Also, I forgot to mention the versions:
kdevplatform 6cf05bdad48f093dcf9efad4f78d8275ce44f7a6
kdevelop cbe05bc0d8218744cc30a0e0241f8c3f25497049
clang 3.9.0
Comment 3 Sven Brauch 2016-11-03 20:49:38 UTC
Ah, sorry, I overlooked the samba part. I find the perf data hard to read ... but it does seem to spend noticeable time in I/O. Maybe it would be interesting to copy the project to local disk and see if it still hangs.
Comment 4 Lukas Jirkovsky 2016-11-03 21:34:47 UTC
There must be something different in how the reparse (if it's the reparse) is invoked. I just did few more experiments:

Test #1
1. rename a variable
2. move to a previous use of the variable using arrow keys
3. in the meantime, the project is reparsed and all previous uses are now marked "Use of undeclared identifier"

The whole time KDevelop is responsive.

Another test
1. rename a variable
2. quickly click on a use before it gets marked as a problem

KDevelop stays responsive

Test #2
1. rename a variable
2. wait for the reparse to finish
3. click on a use that is now underlined

The whole KDevelop UI (I didn't realize it wasn't just the editor before) freezes for about 5 seconds. In fact, any click on the problem freezes the UI.


Maybe I should have emphasized that the UI freezes every time I click on a problem, even when the project is fully parsed, so the parser may not be the problem after all.
Comment 5 Lukas Jirkovsky 2016-11-03 21:39:13 UTC
Another bunch of tests:

Test #1
The project sources are in tmpfs, includes are on the samba share
=> same behaviour

Test #2
All sources, including includes are in tmpfs
=> everything's fine
Comment 6 Sven Brauch 2016-11-03 21:41:14 UTC
Ok, so it really seems to be I/O. Thanks for the tests! Maybe it would be useful after all to run kdevelop in gdb, interrupt it when it's hanging, and get a backtrace. I guess that's the easiest way to find the code path causing the I/O -- I can't see it from the perf data.
Comment 7 Lukas Jirkovsky 2016-11-03 21:46:39 UTC
I just got an idea, though I may be completely wrong. Is it possible that it's related to the Clang Fix-its? They don't show when navigating using keyboard.

I'll try to get backtrace.
Comment 8 Lukas Jirkovsky 2016-11-03 21:56:07 UTC
I hope I sent interrupt at the right time...

#0  0x00007ffff4cc4c9b in __getdents64 () at /usr/lib/libc.so.6
#1  0x00007ffff4cc4a10 in readdir_r () at /usr/lib/libc.so.6
#2  0x00007ffff5556773 in  () at /usr/lib/libQt5Core.so.5
#3  0x00007ffff54dcc8c in  () at /usr/lib/libQt5Core.so.5
#4  0x00007ffff54dd4dd in  () at /usr/lib/libQt5Core.so.5
#5  0x00007ffff54ddd5c in QDirIterator::QDirIterator(QString const&, QStringList const&, QFlags<QDir::Filter>, QFlags<QDirIterator::IteratorFlag>) () at /usr/lib/libQt5Core.so.5
#6  0x00007ffff54d6dcc in QDir::entryList(QStringList const&, QFlags<QDir::Filter>, QFlags<QDir::SortFlag>) const () at /usr/lib/libQt5Core.so.5
#7  0x00007fffbdb12bf5 in (anonymous namespace)::scanIncludePaths(QString const&, QDir const&, int) (identifier=..., dir=..., maxDepth=maxDepth@entry=2) at /home/stativ/aurbuild/kdevelop-git/src/kdevelop/languages/clang/duchain/unknowndeclarationproblem.cpp:95
#8  0x00007fffbdb12fbc in (anonymous namespace)::scanIncludePaths(QString const&, QDir const&, int) (identifier=..., dir=..., maxDepth=2, maxDepth@entry=3) at /home/stativ/aurbuild/kdevelop-git/src/kdevelop/languages/clang/duchain/unknowndeclarationproblem.cpp:105
#9  0x00007fffbdb16ae3 in (anonymous namespace)::scanIncludePaths (includes=..., identifier=...) at /home/stativ/aurbuild/kdevelop-git/src/kdevelop/languages/clang/duchain/unknowndeclarationproblem.cpp:118
#10 0x00007fffbdb16ae3 in (anonymous namespace)::includeFiles (file=..., declarations=..., identifier=...) at /home/stativ/aurbuild/kdevelop-git/src/kdevelop/languages/clang/duchain/unknowndeclarationproblem.cpp:390
#11 0x00007fffbdb16ae3 in (anonymous namespace)::fixUnknownDeclaration(KDevelop::QualifiedIdentifier const&, KDevelop::Path const&, KDevelop::DocumentRange const&) (identifier=..., file=..., docrange=...)
    at /home/stativ/aurbuild/kdevelop-git/src/kdevelop/languages/clang/duchain/unknowndeclarationproblem.cpp:466
#12 0x00007fffbdb172b6 in UnknownDeclarationProblem::solutionAssistant() const (this=0x7fff8d2e60f0) at /home/stativ/aurbuild/kdevelop-git/src/kdevelop/languages/clang/duchain/unknowndeclarationproblem.cpp:520
#13 0x00007ffff2e9a6f7 in KDevelop::ProblemNavigationContext::html(bool) (this=0x2601e40, shorten=<optimized out>) at /home/stativ/aurbuild/kdevplatform-git/src/kdevplatform/language/duchain/navigation/problemnavigationcontext.cpp:172
#14 0x00007ffff2e9fc16 in KDevelop::AbstractNavigationWidget::update() (this=this@entry=0x2821c50) at /home/stativ/aurbuild/kdevplatform-git/src/kdevplatform/language/duchain/navigation/abstractnavigationwidget.cpp:148
#15 0x00007ffff2ea0800 in KDevelop::AbstractNavigationWidget::setContext(QExplicitlySharedDataPointer<KDevelop::AbstractNavigationContext>, int) (this=this@entry=0x2821c50, context=..., initBrows=initBrows@entry=400)
    at /home/stativ/aurbuild/kdevplatform-git/src/kdevplatform/language/duchain/navigation/abstractnavigationwidget.cpp:116
#16 0x00007fffc22c04ba in ContextBrowserPlugin::navigationWidgetForPosition(KTextEditor::View*, KTextEditor::Cursor) (this=this@entry=0x118bef0, view=view@entry=0xe91640, position=...)
    at /home/stativ/aurbuild/kdevplatform-git/src/kdevplatform/plugins/contextbrowser/contextbrowser.cpp:539
#17 0x00007fffc22c20b2 in ContextBrowserPlugin::showToolTip(KTextEditor::View*, KTextEditor::Cursor) (this=0x118bef0, view=0xe91640, position=...) at /home/stativ/aurbuild/kdevplatform-git/src/kdevplatform/plugins/contextbrowser/contextbrowser.cpp:585
#18 0x00007fffc22c3e4f in ContextBrowserHintProvider::textHint(KTextEditor::View*, KTextEditor::Cursor const&) (this=0x118c068, view=<optimized out>, cursor=...) at /home/stativ/aurbuild/kdevplatform-git/src/kdevplatform/plugins/contextbrowser/contextbrowser.cpp:407
#19 0x00007ffff1a4ffbf in  () at /usr/lib/libKF5TextEditor.so.5
#20 0x00007ffff1b518a5 in  () at /usr/lib/libKF5TextEditor.so.5
#21 0x00007ffff55e9659 in QMetaObject::activate(QObject*, int, int, void**) () at /usr/lib/libQt5Core.so.5
#22 0x00007ffff55f66d8 in QTimer::timerEvent(QTimerEvent*) () at /usr/lib/libQt5Core.so.5
#23 0x00007ffff55ea473 in QObject::event(QEvent*) () at /usr/lib/libQt5Core.so.5
#24 0x00007ffff629de0c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () at /usr/lib/libQt5Widgets.so.5
#25 0x00007ffff62a5581 in QApplication::notify(QObject*, QEvent*) () at /usr/lib/libQt5Widgets.so.5
#26 0x00007ffff55bdde0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () at /usr/lib/libQt5Core.so.5
#27 0x00007ffff56116ae in QTimerInfoList::activateTimers() () at /usr/lib/libQt5Core.so.5
#28 0x00007ffff5611bd1 in  () at /usr/lib/libQt5Core.so.5
#29 0x00007fffec611587 in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#30 0x00007fffec6117f0 in  () at /usr/lib/libglib-2.0.so.0
#31 0x00007fffec61189c in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#32 0x00007ffff561270f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#33 0x00007ffff55bc23a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/libQt5Core.so.5
#34 0x00007ffff55c473c in QCoreApplication::exec() () at /usr/lib/libQt5Core.so.5
#35 0x000000000040ba6c in main(int, char**) (argc=<optimized out>, argv=<optimized out>) at /home/stativ/aurbuild/kdevelop-git/src/kdevelop/app/main.cpp:763
Comment 9 Sven Brauch 2016-11-03 21:59:15 UTC
Heh, yes, the trace shows what happens. The "missing include" problem assistant looks through all the files in your include paths recursively, and apparently across the network file system that takes too long. Not sure how to solve that right now -- but at least we know what the problem is. Thanks!

I'll set status to confirmed since this is certainly an issue which can be reproduced and fixed.
Comment 10 Kevin Funk 2017-01-16 18:10:40 UTC
*** Bug 362037 has been marked as a duplicate of this bug. ***
Comment 11 RJVB 2017-01-16 21:31:54 UTC
Created attachment 103447 [details]
workaround patch

(In reply to Kevin Funk from comment #10)
> *** Bug 362037 has been marked as a duplicate of this bug. ***

But but but ... this is a duplicate of my older 362037 :)

But yes, this is mostly caused by clang FixIts, but missing includes that lead to significant IO aren't the only problem. I get similar lock-ups for just about any problem for which a fix might be found, like undefined variables. That also leads to a call of `scanIncludePaths()`.
I only see this on Mac though, *much* less on my otherwise significantly slower Linux rig (both have spinning disks).

Whatever the exact cause, the underlying problem here is that the operation is done on the main (UI) thread.
 
The workaround patch makes the situation liveable. If there were a way to be certain events were processed it would probably (almost) be acceptable: when events are processed that stem from user activity this is good indication that the user isn't interested in seeing a proposed fix. For instance because s/he is already fixing the issue.

In fact, I do find the fix-it and similar pop-ups often rather intrusive and bordering on the annoying. Other times I see a pink/red line and have a hard time getting a popup that tells me what's wrong.
Comment 12 RJVB 2017-01-30 09:13:05 UTC
I've been looking for a somewhat less brute work-around, because I have had a bit too many cases where I couldn't even select a variable or function by double clicking without being taken to its declaration. That may be completely unrelated, but it may also be a result of bailing out of ongoing scans unexpectedly.

- Only Qt's QCoreApplication::eventDispatcher()->processEvents() tells us if any events were actually processed, QCoreApplication::sendPostedEvents() and QCoreAPplication::processEvents() do not. Knowing this for certain would probably require installing an event filter, but how? At the level of the application (KDevelopApplication), with a generic state variable that parser can (re)set and query  to know if something happened requiring them to abort ongoing operations?

- Being able to bail out of ongoing operations seems like a good idea in general, but only if upstream codepaths can distinguish a bail-out return from a regular return that didn't yield results. If an API has been implemented for that I completely missed it.
Comment 13 RJVB 2017-01-30 20:04:38 UTC
Created attachment 103723 [details]
improved workaround

This is a workaround patch implementing some of the ideas outlined in my previous message.
Comment 14 Milian Wolff 2017-01-31 10:26:02 UTC
this should be fixed by making sure the unknown declaration problem its check in another thread - probably this will require some more extensive refactorings but would certainly be worth it.
Comment 15 RJVB 2017-01-31 14:50:39 UTC
I agree that a fix would involve moving this algorithm to its own thread though if that's all that is done we will probably see the same kind of behaviour I had in an early version of my workaround.

In that I simply called QCoreApplication::processEvents() and then continued the headerfile scan. That also unblocks the GUI thread (just like moving the scan to a background thread would), but it also meant that user actions could queue a new unknown declaration problem. As a result I'd get a series of solution propositions that would pop up in rapid succession, and in practice that happened more frequently and was more annoying than I expected.

Hence the idea of bailing out of an ongoing scan. In theory you'd only have to do that for certain event types: keyboard strokes, anything that changes the focus (text cursor but also mouse pointer?), probably mouse right-clicks and maybe a few I've overlooked.

It's probably less work to implement a configuration option to disable this particular feature when it proves to be more of a PITA than useful. Examples would be headers on remote/slow media, but also projects with "enough" files that include headerfiles not available on the editing host (e.g. a remote source directory configured to build on the remote host).
Comment 16 RJVB 2017-02-02 13:10:09 UTC
(In reply to Milian Wolff from comment #14)
> this should be fixed by making sure the unknown declaration problem its
> check in another thread

How about using handling the headerdir scan in a QProcess, and using a local QEventLoop to handle event processing while waiting for the scan job to finish? Not as elegant as other solutions, but it would probably be an easy way to test the benefits of a multithreaded approach?

It seems that if we subclass QEventLoop we could override QEventLoop::event() to catch events that justify bailing out of the current scan operation.

Because however I look at it, scanning all include directories for a possible solution to a missing declaration remains expensive, esp if
- it's done every time the missing declaration gets focus
- it always runs to completion even if the user shifted the focus elsewhere and the solution is thus no longer needed.

I'll see how trivial (or not) it is to implement these ideas. Do we agree that only a single unknown declaration problem should be processing at any given time, i.e. that any ongoing scan should be aborted when another request comes in?

I also think that it would be very useful if each problem of this kind scanned a given set of headerfiles only once. That doesn't seem trivial to implement; a reset would be required when the headerdir search path changes but also when additional files are installed or files are edited.

Clang supports precompiled headers. Wouldn't it be possible to let the parser and ClangProblem features use a precompiled header of all headers in the search path? Keeping the .pch file is something that would be perfectly suitable for a background thread.
Comment 17 RJVB 2017-02-02 14:22:15 UTC
(Doh, I meant QThread of course, not QProcess)
Comment 18 RJVB 2017-02-02 18:04:30 UTC
https://phabricator.kde.org/D4411