UnknownDeclarationProblem: scan headers on a background thread to preserve GUI responsiveness (WIP)
AbandonedPublic

Authored by kfunk on Feb 2 2017, 5:11 PM.

Details

Reviewers
mwolff
rjvbb
Group Reviewers
KDevelop
Summary

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

Scanning for fixes of "unknown declaration problems" involve scanning all available headerfiles, which can take a significant amount of time. This is currently done on the main thread, which means that any unknown variable or function can halt event processing and render the editor unresponsive. On Mac this will display the dreaded beachball cursor.

This patch proposes a relatively simple solution that moves the scan to a background thread with only minimal refactoring that is purely internal to the implementation.

It is based on the presumption that

  • only a single unknown declaration problem can be tackled at a time
  • there is no point in continuing the search for solutions if the user changes the focus of interest, regardless of whether the new (cursor) location has another unknown declaration or not.

Thus, fixUnknownDeclaration() is moved to a new class, UDPWorkerThread that inherits QThread, of which a single global instance is allocated by UnknownDeclarationProblem::solutionAssistant(). That method also interrupts still running scans before starting a new scan.
Event processing is ensured by a UDPScanEventLoop instance that inherits QEventLoop; this class also has an event sniffer that raises an interrupt request to the scan worker thread when events occur that justify bailing out of the current scan.

Test Plan

For now this has been tested on OS X 10.9 with Qt 5.7.1, KF5 5.29.0 and clang 3.9.0 . Initial testing suggests that the approach works as expected in preventing GUI freezes, but the event filter isn't used at all. As a result, solution popups still open (long) after the cursor has been moved elsewhere, potentially even replacing other popups.

It may be possible to replace the separate UDPScanEventLoop instance with QThread's event loop but I don't know if there is a real advantage to that.

It would probably also be useful if the ClangFixitAssistant class had a means to indicate that it does not have valid results, but maybe the solutionAssistant() method can return NULL or ClangFixitAssistant() when a scan was interrupted?

Diff Detail

Repository
R782 KDevelop Clang Integration
Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb updated this revision to Diff 10856.Feb 2 2017, 5:11 PM
rjvbb retitled this revision from to UnknownDeclarationProblem: scan headers on a background thread to preserve GUI responsiveness (WIP).
rjvbb updated this object.
rjvbb edited the test plan for this revision. (Show Details)
rjvbb updated this object.Feb 2 2017, 5:57 PM
rjvbb edited the test plan for this revision. (Show Details)
rjvbb added a reviewer: KDevelop.
rjvbb set the repository for this revision to R782 KDevelop Clang Integration.
rjvbb added a project: KDevelop.
rjvbb added a subscriber: KDevelop.
rjvbb updated this object.Feb 2 2017, 6:04 PM
rjvbb updated this revision to Diff 10868.Feb 2 2017, 8:25 PM

This iteration works a bit better, and actually catches (and filters) all events the application receives.

Aborting the scan for a solution still does NOT prevent a popup, annoyingly, despite that I now return a NULL pointer on interrupt. I thought every solutionAssistant() caller only used the return value from that method if not NULL, but apparently not for this kind of problem.

rjvbb edited subscribers, added: kdevelop-devel; removed: KDevelop.Feb 2 2017, 8:51 PM
mwolff requested changes to this revision.Feb 2 2017, 10:12 PM
mwolff added a reviewer: mwolff.
mwolff added a subscriber: mwolff.

sorry, but nested eventloops are a no-go.

the api has to be restructured to allow async assistants. until that is not done, shoehorning it in this way is a clear -2 from my side

languages/clang/duchain/unknowndeclarationproblem.cpp
71

probably needs to be atomic? and try to push it into one of the classes

73

what is UDP?

270

?? remove

754

nested eventloops are a no-go

758

?? remove

This revision now requires changes to proceed.Feb 2 2017, 10:12 PM
rjvbb added a comment.Feb 3 2017, 10:07 AM
In D4411#82688, @mwolff wrote:

sorry, but nested eventloops are a no-go.

the api has to be restructured to allow async assistants. until that is not done, shoehorning it in this way is a clear -2 from my side

Is it worth anyone's time then that I try to deal with the other comments?
Given the way the API is implemented in multiple places over 2 projects and the amount of work it will thus be to rewrite it I'm going to leave that to someone who knows the code much better. Or maybe it's a nice GSOC project (deadline for proposals coming up).
Just be aware that the UI freezes are a real blocker; having to wait 2-3s (or more) *each* time you stumble upon an unknown variable is just not acceptable and can put people off very quickly.

This was intended as an acceptable/reasonable workaround, which is also why I have changed as little as possible to the actual payload code.
I just have 1 request: If this won't go in please don't break the patch (too much) before a real fix is committed, so this workaround remains available as an optional local patch for those who want to use it.

Quick reactions to the comments:

  • the interrupted global var. is scheduled to go into the UDPWorkerThread class, hasn't been done yet because:
  • the #if 0 blocks are still there to make it easier to compare behaviour under old and new implementations.
  • UDP = Unknown Declaration Problem, no confusion intended (or expected...) with the networking protocol.
kfunk added a subscriber: kfunk.EditedFeb 21 2017, 11:30 AM

IMO you should abandon this. This would add tons of cumbersome code which is almost impossible to reason about (hint: nested event loops, event filtering of UI events, ...).

We'll find another solution, I actually have something in my Git stash I just need to polish a bit more.

kfunk requested changes to this revision.Feb 21 2017, 11:30 AM
rjvbb marked an inline comment as done.Feb 21 2017, 11:49 AM
In D4411#88229, @kfunk wrote:

IMO you should abandon this. This would add tons

Tons? Must be the imperial version then :)

I'll discard if and when a satisfactory alternative solution is found. In the meantime I intend to use my own patch and keep this up as a public reference for anyone else who's also too affected by the issue to put up with stock behaviour.

kfunk commandeered this revision.Jan 9 2018, 11:54 AM
kfunk abandoned this revision.
kfunk edited reviewers, added: rjvbb; removed: kfunk.

Abandoning as this is clearly not the right way to fix the issue at hand.

rjvbb added a comment.Jan 9 2018, 12:40 PM
In D4411#188133, @kfunk wrote:

Abandoning as this is clearly not the right way to fix the issue at hand.

I guess I should thank you then for having taken over authorship of it 8-)

I don't know about it being the right, left, or wrong fix, but AFAIK it's still the only workaround that makes KDevelop usable in situations where the issue at hand would cause incessant and highly disruptive lock-ups.