Bug 548891 - Move as much as possible of Hover calculations off UI thread
Summary: Move as much as possible of Hover calculations off UI thread
Status: RESOLVED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 4.13   Edit
Hardware: PC Mac OS X
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-Text-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-02 18:48 EDT by Alex Boyko CLA
Modified: 2019-07-04 10:09 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Boyko CLA 2019-07-02 18:48:40 EDT
See class org.eclipse.jface.text.TextViewerHoverManager
Method: computeInformation()

The call to hover.getHoverRegion(fTextViewer, offset); is done on the UI thread. However the only part needs UI thread is
JFaceTextUtil.computeArea(region, fTextViewer);

I propose 
1. move hover.getHoverRegion(fTextViewer, offset); off the UI thread under fThread
2. Move widget hover area calculation (JFaceTextUtil.computeArea(region, fTextViewer);) under fThread but wrap it inside the Display#asyncExec and wait for the resultant area to be computed on the UI thread.

Gerrit patch for the proposal is coming.
Comment 1 Alex Boyko CLA 2019-07-02 18:53:16 EDT
Language server protocol computes the hover region (text region) and hover content on the language server process side. (LSP4J and LSP4E projects)
Therefore, it'd benefit if calculations that might be done outside of Eclipse process would be moved off the UI thread where possible.
Comment 2 Eclipse Genie CLA 2019-07-02 18:54:58 EDT
New Gerrit change created: https://git.eclipse.org/r/145344
Comment 3 Mickael Istria CLA 2019-07-03 02:13:10 EDT
I'm not sure it's really something we can change at this point in existing hover strategy (see comment on Gerrit).
I think we need to workaround it in LSP4E instead (returning some dummy good enough value in case operation take too long)
Comment 4 Alex Boyko CLA 2019-07-03 11:02:15 EDT
Yes, I agree moving something off UI thread is indeed an API breakage...

Wonder if we could still do something like what i proposed without breaking the API...

For example TextViewerHoverManager asks the text viewer for ITextHover... Perhaps we could add a new method to the interface ITextHover called isAsync() and if it returns true perform calculations off the UI thread (except widget area). Otherwise, if not async proceed with calculations exactly as before thus not breaking the API. The method could be introduced to the interface with default implementation returning false.

The ExtensionBasedTextViewerConfiguration can create a subclass of CompositeTextHover where isAsync() returns true and LSBasedHover would also return isAsync() true.

What do you think?
Comment 5 Mickael Istria CLA 2019-07-03 11:06:41 EDT
(In reply to Alex Boyko from comment #4)
> What do you think?

This is the right path IMO. However, since we see more and more occurrences of this problem (moving as much computation as possible out of UI Thread), I think a common solution need to be chosen by Platform at large rather than a Generic Editor specific workaround.
I've started a thread on this topic on platform-dev@eclipse.org.
Comment 6 Mickael Istria CLA 2019-07-04 06:58:38 EDT
See bug 548970 about a common way to declare UI Thread isn't required.
Once we have this is (or something similar), we can rework the hover mechanism to check for that and go to another thread when possible.
Comment 7 Alex Boyko CLA 2019-07-04 10:09:14 EDT
I'll close this as wontfix then