Community
Participate
Working Groups
In order to enable async content-assist, we need to make sure all extensions to the completion computation can run out of UI Thread. This is not the current state and there is no way automatically check that at runtime. So the most appropriate way seems to be offering on the extension point a flag such as "nonUIThread" to let extensions declare when they do not require UI Thread and then are eligible for async content-assist.
New Gerrit change created: https://git.eclipse.org/r/128745
(In reply to Eclipse Genie from comment #1) > New Gerrit change created: https://git.eclipse.org/r/128745 Please have a look at the Gerrit patch for review comments.
(In reply to Noopur Gupta from comment #2) > Please have a look at the Gerrit patch for review comments. I think the Gerrit has been update. Please have a look
(In reply to Lars Vogel from comment #3) > (In reply to Noopur Gupta from comment #2) > > Please have a look at the Gerrit patch for review comments. > > I think the Gerrit has been update. Please have a look This patch is useful only with bug 538656. I will wait for the issues with that bug to be resloved first.
As I'm working on this stack of bugs, I'm wondering whether a flag on extension is the best approach. Indeed, the capacity to require UI Thread or not is not specific to the extension, and if a processor declares it's non-UI-thread-able, it's also the case for other consumers. So it seems to me this piece of information would better be placed directly in the code to enable a wider usage. Some proposals that I think are better than a flag on extension declaration: 1. A @NonUIThread annotation to place on the implementations => extenders add annotation to their class, client could check for this annotation 2. A NonUIThread interface that would be placed on implementations => extenders add interface, client could check for instanceof NonUIThread 3. a UIThreadInformation interface with a requiresUIThread() method => extenders add interface and implement method, client cast then invoke method 4. a requiresUIThread default method on the API interfaces IJavaCompletionProcessor and IJavadocCompletionProcessor => extenders implement method, client invokes method Should we adopt one of those?
I think annotations are the most "nice" and modern way to express this. Implementing additional interfaces / extending existing interfaces has a bit of a legacy touch to me. Another advantage of annotations is that they could be re-used not only for IJavadocCompletionProcessor but for arbitrary code, even in other bundles. As for the name of such an annotation, I think for example Android has a nice practice: https://developer.android.com/studio/write/annotations.html#thread-annotations Especially a @NonUIThread annotation feels more like it expresses that a certain implementation *must* (should) run in a worker thread, instead of expression that it is essentially thread-safe and can be called from *any* thread, including the UI thread. Therefore, I think something like Android's @AnyThread would be more suited for this concrete scenario.
+1 for annotation approach. In later stage we can even build static analysis based on this for PDE.
+1 for annotations Android already did all the "concept" work for us, hope we can just reuse the approach :)
+1 for annotations
Please consider adding another extension point for async handlers. A hypothetical thread safety analyzer would need a more sophisticated set of annotations, you can't use this as an argument in favor. Also, I can't agree that using interfaces has a "legacy touch". On the contrary, trying to distinguish service implementations via runtime annotation seems like an antipattern, both in OOP and in service oriented architercure.
(In reply to Nikita Nemkin from comment #10) > Please consider adding another extension point for async handlers. This seems overkill as the goal is to migrate or encourage most of existing extension and the existing extension point has worked like a charm so far. Creating a new extension point wouldn't provide anything really valuable and would increase the adoption cost. > A hypothetical thread safety analyzer would need a more sophisticated set of > annotations, you can't use this as an argument in favor. We're not trying to make any thread safety analyzer, we're trying to give a (preferably reusable) way to extenders to declare whether their code requires SWT UI Thread or not. Anything else is beyond the scope of this issue (and related ones) > Also, I can't agree that using interfaces has a "legacy touch". On the > contrary, trying to distinguish service implementations via runtime > annotation seems like an antipattern, both in OOP and in service oriented > architercure. I personally don't mind one or the other, but you can't deny annotations are usually popular for this kind of things (which are about annotating code more than adding some behavior) and that it's perceived -maybe wrongly- as more modern than an empty interface. And if it were an interface, would it be possible to host it in SWT, as annotating whether code requires UI Thread or not is a SWT specific problem, it makes sense that SWT tries to provide a solution more than pretending this issue doesn't exist while probably 100% of SWT developers probably faced an "InvalidThreadAccess" one day.
(In reply to Mickael Istria from comment #11) > And if it were an interface, would it be possible to host it in SWT, as > annotating whether code requires UI Thread or not is a SWT specific problem, > it makes sense that SWT tries to provide a solution more than pretending > this issue doesn't exist while probably 100% of SWT developers probably > faced an "InvalidThreadAccess" one day. By using an interface I meant having separate services for sync and async handlers. (NOT IAnyThread or anything like that). Anyway, the problem of marking completion sertices as thread safe has nothing to to with SWT. Native GUI toolkits are mostly single-threaded. There's no "solution" to that.
(In reply to Nikita Nemkin from comment #12) > By using an interface I meant having separate services for sync and async > handlers. We cannot afford adding APIs just like this, as it has a heavy cost on migration and maintenance. Creating a new API instead of reusing existing ones is a very probable cause of failure. The Generic Editor was built on the idea of *only* reusing APIs, and it has been successful, with high reusability of legacy and high adoption. That's what we need to enable in JDT. > Anyway, the problem of > marking completion sertices as thread safe has nothing to to with SWT. I think you missed the point, because yes, it definitely has something to do with SWT! More context: Like many "legacy" parts of Eclipse Platform/IDE, JDT runs all the completionProcessors in the UI Thread. It's something we cannot undo for backward compatibility, but we're trying to have a way for extenders/providers to state that their code can run in a non-UI Thread; with this JDT can optimize its execution. This is a very common problem for any SWT application. It's not about being thread safe or not, it's only about having some flag/annotation/interface/whatever that allows author to mark a piece of code of something that's ultimately part of a SWT application as not requiring UI Thread. It is a really typical SWT problem, it has occurrences everywhere SWT is. So that's why it deserves to be considered by SWT to help its adopters be more successful and not rejected as being a 3rd party problem.
+1 For Annotation. For being it in SWT, If suppose the code is in JDT Core, how will it access SWT annotation? Non UI plug-ins will not have a dependency on SWT.
(In reply to Sarika Sinha from comment #14) > +1 For Annotation. > > For being it in SWT, If suppose the code is in JDT Core, how will it access > SWT annotation? Non UI plug-ins will not have a dependency on SWT. I assume the annotation is required for UI affine projects only. JDT core is by definition not running in UI and so implicit is "AnyThread". JDT UI code that uses some JDT core code and can run outside of UI thread should be marked with AnyThread annotation.
Should we reinvent something? Android already described all the semantic forannotations like this here https://developer.android.com/studio/write/annotations.html#thread-annotations Do you think we can create ui-neutral bundle org.eclipse.<TBD>.annotations and put it there?
(In reply to Alexander Fedorov from comment #16) > Should we reinvent something? Android already described all the semantic > forannotations like this here > https://developer.android.com/studio/write/annotations.html#thread- > annotations > > Do you think we can create ui-neutral bundle org.eclipse.<TBD>.annotations > and put it there? +1 for that. I also do not like the idea to put new annotations into SWT. We have already org.eclipse.jdt.annotation bundles for Null semantics, why not have org.eclipse.ui.annotations (with "s") for UI semantics?
(In reply to Alexander Fedorov from comment #16) > Should we reinvent something? Android already described all the semantic > forannotations like this here > https://developer.android.com/studio/write/annotations.html#thread- > annotations > > Do you think we can create ui-neutral bundle org.eclipse.<TBD>.annotations > and put it there? I suggest to put the new annotation/plug-in in eclipse.platform.runtime.
(In reply to Andrey Loskutov from comment #17) > (In reply to Alexander Fedorov from comment #16) > > Should we reinvent something? Android already described all the semantic > > forannotations like this here > > https://developer.android.com/studio/write/annotations.html#thread- > > annotations > > > > Do you think we can create ui-neutral bundle org.eclipse.<TBD>.annotations > > and put it there? > > +1 for that. I also do not like the idea to put new annotations into SWT. > > We have already org.eclipse.jdt.annotation bundles for Null semantics, why > not have org.eclipse.ui.annotations (with "s") for UI semantics? -1 for putting it in JDT, non JDT projects should also be able to use it.
(In reply to Lars Vogel from comment #19) > (In reply to Andrey Loskutov from comment #17) > > (In reply to Alexander Fedorov from comment #16) > > > Should we reinvent something? Android already described all the semantic > > > forannotations like this here > > > https://developer.android.com/studio/write/annotations.html#thread- > > > annotations > > > > > > Do you think we can create ui-neutral bundle org.eclipse.<TBD>.annotations > > > and put it there? > > > > +1 for that. I also do not like the idea to put new annotations into SWT. > > > > We have already org.eclipse.jdt.annotation bundles for Null semantics, why > > not have org.eclipse.ui.annotations (with "s") for UI semantics? > > -1 for putting it in JDT, non JDT projects should also be able to use it. I've never proposed to push anything to jdt.
(In reply to Andrey Loskutov from comment #20) > I've never proposed to push anything to jdt. Thanks for the clarification, I misread. > We have already org.eclipse.jdt.annotation bundles for Null semantics, why > not have org.eclipse.ui.annotations (with "s") for UI semantics?
(In reply to Andrey Loskutov from comment #17) > We have already org.eclipse.jdt.annotation bundles for Null semantics, why > not have org.eclipse.ui.annotations (with "s") for UI semantics? Ok with that. (In reply to Lars Vogel from comment #18) > I suggest to put the new annotation/plug-in in eclipse.platform.runtime. If the annotations are about "does it have to run in SWT UI Thread" (which is the only thing we need at the moment), then it's purely UI and would make more sense in eclipse.platform.ui. (In reply to Alexander Fedorov from comment #16) > Do you think we can create ui-neutral bundle org.eclipse.<TBD>.annotations > and put it there? I don't think it's possible. From a UI backend to the other, the same code may require to run in UI Thread or not, depending on which operations of each back-end are thread safe or not. So annotations about whether UI Thread is required seem to really specific and not easily generalized. For instance, in the case of Android, those annotations are solely about Android, and their description matches Android-specific concepts.
(In reply to Mickael Istria from comment #22) eclipse.platform.ui is also fine for me.
(In reply to Mickael Istria from comment #22) > > If the annotations are about "does it have to run in SWT UI Thread" (which > is the only thing we need at the moment), then it's purely UI and would make > more sense in eclipse.platform.ui. > Well, what if we can consider more generic context like a hint for OSGi EventAdmin regarding EventHandler execution? > (In reply to Alexander Fedorov from comment #16) > > Do you think we can create ui-neutral bundle org.eclipse.<TBD>.annotations > > and put it there? > > I don't think it's possible. From a UI backend to the other, the same code > may require to run in UI Thread or not, depending on which operations of > each back-end are thread safe or not. So annotations about whether UI Thread > is required seem to really specific and not easily generalized. Not a problic with IoC approach. We already have org.eclipse.e4.core.services.events.IEventBroker that can vary dispatching according to the flag * @param headless * <code>true</code> if handing of the events does not require * UI; <code>false</code> otherwise The annotations (more than one you need now, but we can start only one) may improve the event handling capabilities.
(In reply to Alexander Fedorov from comment #24) Sorry for misprints
For those interested in static analysis, let me just remind you that we already have bug 482891. No need to invent anything, indeed. I'm not trying to hijack this bug, but if the initial interest in static analysis ever has a revival one day, it would be a pity, if we already support an annotation that is a near miss for static analysis.
(In reply to Stephan Herrmann from comment #26) > For those interested in static analysis, let me just remind you that we > already have bug 482891. No need to invent anything, indeed. > > I'm not trying to hijack this bug, but if the initial interest in static > analysis ever has a revival one day, it would be a pity, if we already > support an annotation that is a near miss for static analysis. Bug 482891 requires annotation on *all* involved UI /non UI types. While I understand how this could be used by static analysis, I believe that this is an overkill. What is discussed here is to annotate *some* methods of a UI related API type that can run in non UI thread. So by default everything in that API could require UI thread unless the annotation is added.
(In reply to Andrey Loskutov from comment #27) > (In reply to Stephan Herrmann from comment #26) > > For those interested in static analysis, let me just remind you that we > > already have bug 482891. No need to invent anything, indeed. > > > > I'm not trying to hijack this bug, but if the initial interest in static > > analysis ever has a revival one day, it would be a pity, if we already > > support an annotation that is a near miss for static analysis. > > Bug 482891 requires annotation on *all* involved UI /non UI types. While I > understand how this could be used by static analysis, I believe that this is > an overkill. > > What is discussed here is to annotate *some* methods of a UI related API > type that can run in non UI thread. So by default everything in that API > could require UI thread unless the annotation is added. For the feature at hand bug 482891 surely is overkill, no doubt about that. I just hope you are not implying one could (later) do meaningful static analysis with a trivial annotation solution (btw, does SpotBugs have similar analyses?)
Hi all, Have we decided on a approach. I think this is the first step towards supporting async completion in JDT which is very interesting feature to have.
(In reply to Gayan Perera from comment #29) > Hi all, > Have we decided on a approach. I think this is the first step towards > supporting async completion in JDT which is very interesting feature to have. We need bug 547683 solved first anyway, so let's delay a decision until we have fixed the other one.
So I'm back working on this. My current opinion is that for Java, whether a completion processor can be non-UI or not depends dynamically on the current processor and the context. So it looks like the best way forward for JDT is to add a new method `requiresUIThread(context)` in IJavaCompletionProposalComputer.
(In reply to Mickael Istria from comment #31) > So I'm back working on this. > My current opinion is that for Java, whether a completion processor can be > non-UI or not depends dynamically on the current processor and the context. > So it looks like the best way forward for JDT is to add a new method > `requiresUIThread(context)` in IJavaCompletionProposalComputer. And after more hands-on, it seems like I'm wrong: all the computers in JDT can e turned into requiring no UI Thread and annotated as such. So we have low need for making it dynamic, and we can keep a static opt-in flag on the extension to declare it's not requiring UI Thread.
Gerrit change https://git.eclipse.org/r/128745 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=00d3a2245a640401a17221e649f24f0415bf3093
Will work on N&N entry next week.
New Gerrit change created: https://git.eclipse.org/r/152764
Gerrit change https://git.eclipse.org/r/152764 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=2b4908beaf813e982d61e850f35edb42fbcdb78f
