Community
Participate
Working Groups
Code completion (Ctrl-Space) is currently single-threaded, and blocks the UI thread, which is inconvenient: 1) when project is large 2) when certain computations take considerably more time than other (e.g. completing "public" is always cheap, whereas completing to class names requires the indexes to be happy, etc). This applies to some other languages as well, such as C++. We (Zorzella and Douglas Pollock) have put together a small patch, which is an attempt to show how this might be done. We have tried, where possible, to be completely backward compatible. However, we are aware of two cases where we have not initially made it to be backward compatible. First of all, we have changed the interface of IJavaCompletionProposalComputer to allow its implementors to run in the background. Also, we have changed the contract of CompletionProposalCategory. Now, categories from the extension point must expect that they might be run in the background. We have a few ideas on how to work around these issues (without changing the public APIs), but we wanted to show a simpler patch first. Since the processing does not happen in the UI thread anymore, it was necessary to change several places that wanted to calculate the selected range. What we do now is to pass the "selectedRange" around as a parameter, which seems better anyway. This is responsible for a lot of "noise" in the patch. This is also responsible for the change to the IJavaCompletionProposalComputer interface (some places may eventually be simplified, since they continue on passing the offset as well as the selectedRange, which is now redundant). The basic principle is to change the way in which ContentAssistProcessors returned information about which completions were possible. In the old model, these implemented computeCompletionProposals, which returned an array or ICompletionProposal. In the new model, the same method (defined in ICompletionProposal2 for backwards-compat) is "void", and it receives a ProposalList, which is an object that accepts new ICompletionProposals to be added to it. This ProposalList interface is implemented by two classes (again, for backwards-compat). The fancy implementation of this interface -- DefaultProposalList (currently a private inner class of CompletionProposalPopup) -- is where the "meat" of this patch is. What it is is a call back. As different threads (as many as we want) find more valid completions, they add to this data structure. This, in turn, periodically (for perf reasons) updates the UI (calling the "old" setProposals) with its current state. It also knows (with the help of ICompletionProposal2) how to sort the entries it receives, so it is *always* in a consistent state. It abstracts (handles) filtering, which used to be special-cased. In terms of threading (and yes, we could convert this to jobs), there are two levels to the hierarchy of threads. Immediately on code complete trigger, a background thread is launched to gather data. After this is started, detached from the UI, it is also free to create multiple other threads, as desired. In fact, this implementation creates a thread per potential computer, and then executes all the computers in parallel. The parallel execution of computers can be seen in ContentAssistProcessor. One thing not done (but supported) in this patch is to extend the use of the ProposalList deeper, so that *each* entry is calculated to be valid (within a single computer), it can be displayed immediately in the UI. Issues (other than breaking the public APIs). 1) Cycling between types of completions (java, template, SWT, etc) does not currently work 2) Always need to ESC to kill the completion popup (even when backspacing to empty) 3) If there is only a single completion, we do not auto-complete 4) we lack some polishing, e.g. an "in progress" message 5) We don't have support for proposals that aren't ICompletionProposalExtension (or Extension2)s 6) CompletionProposalPopup.fMessageText becomes corrupted in a particular code path
Did bugzilla eat the attachment or something?
To start: can you provide a real-life example (bugzilla entry) where code assist is really slow and where doing it in the background would help? Note that for JDT we just added a timeout i.e. it will never take longer than that timeout and this is currently set to 500ms and so far none of our tests or daily usage hit it. So, I want to be sure we solve a real and concrete issue before we add this level of complexity. Doesn't the user want to wait for the proposals anyway i.e. how do you know that the proposal you want is already there? What does your code do if the user continues typing? Currently, if the user types more characters once the proposals are there, we are very fast because we only need to filter the first list. With a background thread approach it would probably cancel the first computation and then start over again, which will most likely be slower in the end. And probably the same issue when the user presses backspace. Sorting: since new entries get sorted I guess the list won't be stable anymore i.e. if I type fast and select the first entry and at the same time new entries come in, it might pick and insert the wrong one. I have the same usability issues with the CVS Repository view: I want to expand a node but suddenly new entries move my element away.
Created attachment 115426 [details] Draft patch
Indeed, it seems the original patch didn't go through. I just reattached it. To respond to the issues: 1) need: I would certainly not have spent the time I did to work on this if I didn't feel it was important. My first point is that there are several one-off "safeguards" (hacks) that were put in place to skirt the issue that completion blocks the UI thread until all proposals are done. a. The timeout is one of them. I'm not so sure about this half second timeout you speak of. In my (monster, everyday) project, I refreshed and did a Ctrl-Space on "N" (as a field of a class), and, after about 15 seconds I got the "Problems During Content Assist: The 'org.eclipse.mylyn.java.ui.avaTypeCompletionProposalComputer' proposal [...] took too long to return from 'computeCompletionProposals()' operation' [...] Another interesting thing to note here is that, upon cancelling that dialog I actually get completions for "N". ESC-ing it and hitting it a *second time* still took about 2 seconds. Only the third time around I actually got it to work in under half a second. b. Premature cropping is another safeguard. For example, if you Ctrl-Space where I put <HERE> class Foo { <HERE> } you will *not* see a list of all classes that could go there, but, rather, just the set of methods and template completions. c. Also, note that how much is too long to wait is totally arbitrary. Completion (IMHO) is mostly useful when I'm writing code where I know exactly what I want (rather than investigating possibilities). Even half second is too long (again IMHO). I find myself much more often using Ctrl-/ (hippie completion) than Ctrl-Space, since hippie is just instantaneous (almost never blocks the UI "at all"). I often fear Ctrl-Spac'ing just cuz I don't know how long it will take (and, when it takes long, it locks the whole UI and I can't do any work). d. There are potentially useful completions that could be written that do take a long time to compute. Why can't we have it both ways: always fast response, first class support for things that take a long time? 2) Question: What does your code do if the user continues typing? Exactly the same as it's done today (which is far from optimal): we continuously filter the original results (rather than recompute). We also handle both levels of backspace "correctly" (i.e. we do the same thing that the current code does, which is, again, not optimal). 3) Question: Doesn't the user want to wait for the proposals anyway i.e. how do you know that the proposal you want is already there? 2 answers: currently, as I said, we actually wait for each computer to finish before displaying all the entries in it. In other words, the list will be updated by adding batches of things (all the reserved keywords; all the java classes; all the templates; etc). This might or not be what we want in the long run (Shift-Ctrl-T works even more async'ly), but in either case I don't see why it's a problem (given he can continue to type-filter, and narrow down to what he is looking for). BTW, what we *are* still missing is an "In progress" message, as I mention! 4) Question: Sorting interferes with selection from list Sorting is not yet 100% polished, and, indeed, I think it resets the list. I think this is fixable (we can remember the currently selected position -- if any -- and restore it every time we update the list). 5) Lastly, remember that for all practical purposes, if the responses are very fast, the user will not notice anything different (granted, there are still quirks with our patch, as I also caveat).
Against what tag is the patch? It fails for me against HEAD. >c. Also, note that how much is too long to wait is totally arbitrary. >Completion (IMHO) is mostly useful when I'm writing code where I know exactly >what I want (rather than investigating possibilities). Even half second is too >long (again IMHO). I completely agree, that's why the first thing to do is to find out why it takes 12s in your workspace. Did you file a bug report for that? If not, you should do so (please cc me). Note that the 500ms timeout just went in last week.
I the 500ms went in last week, then I don't have it in my general use Eclipse. Also, if you just tinkered with this code area last week, that would explain why the patch fails. I can (and will) merge it with HEAD, but it would be great if you could be looking at this patch while I do that. I don't know exactly how to give you the right "version", but what I do is to run Eclipse 3.4.0, and cvs check out a few select plugins. The last sync I did was about 6 weeks ago (the patch should also have the version numbers for the individual files). Of course, if you'd rather wait for me to merge, I'll do that as soon as I can. Peace, Z
Created attachment 115646 [details] Patch (Version #2, HEAD as of 2008-10-20 4:15 p.m. PDT) Merged to HEAD as of around 4:15 p.m. (PDT) and rebuilt the patch.
>I don't know exactly how to give you the right "version", The easiest to make a patch work is to do it against the tag of the latest I-build (today would be "v20081021-0800") or milestone. OK, I looked at how it feels with the patch loaded but didn't look at any code yet. On my machine the result/LAF is worse than before: I first see an empty list (no proposals) then quickly afterwards it starts to get filled. This is quite a noisy user experience. Given that you also agree that even 500ms is too slow I don't see any benefit to add background computation here. Rather I spend my time to investigate and fix the cases where it takes too long. Even if code assist takes a second, doing it in the bg and updating the list isn't worth the less smooth user experience. I'd really need to get/see a real use case where I can test the patch and really see the benefit over the current approach. Note that I got a compile error due to usage of 1.5 API in a bundle that's spec'ed to run on a J2SE-1.4 EE. You should see a warning (and if you run on latest I-build even an error) on the project, telling you that no strictly compatible JRE could be found.
Adding a 500ms timeout does make things a lot better. Thanks for adding this. But we feel that we might be able to make things better. Here are some thoughts: * If your project is re-building the type indexes, completion of types needs (to the best of my guess) wait for that to finish before it can present them. * 500ms is too short. There might be plugins that want to allow for completion of things that legitimately take more than 500ms to complete. An arbitrary timeout might deter/prevent/hinder the creation of something that is useful (albeit slow). [There is an example of such a plugin internal to Google.] * 500ms is also too long. 500ms is a user-perceivable delay that will block typing and other interactions with the UI. Notice that in any of these examples, there are (or might be) other *types* of completion that complete quickly. For example, even while re-computing the index of a project, it knows how to complete keywords and code templates. My personal bias is that Eclipse shouldn't block the UI thread except when absolutely necessary. There are plenty of instances where it does, and I'd love to kill them all: Ctrl-T, Ctrl-Space, F4, F3.... I mean, there are very few things for which I am willing to wait. Thoughts? [The immediate appearance of the empty list is a bug, and we're working on it. It should appear with some (or all) items already populated, and, if appropriate, an "In Progress..." message until it's complete]
Created attachment 117154 [details] Patch (Version #3, fixes 1.4 incompatibility) We removed the use of isEmpty(). This should now compile without error on recent versions of Eclipse. With the recent version of Eclipse, we see some kind of error dialog the first (warm up) time we do code complete. It says it is disabling some forms of complete. Maybe this is part of the recent changes for a timeout? After things are "warmed up" (all paged into memory, etc), we can reenable the disabled completions and things start to work again. Another thing we noticed was that, still on a "warm up" completion request, that the list of results was incomplete. I typed: N<Ctrl-Space> and it brought a large (but incomplete) list of types that matched the prefix. In particular, it did not bring NullPointerException, so as I proceeded to type: NPE the completion list disappeared. Going back again and completing "N" gave me a complete (?) list. I suppose that the timeout you introduced would be causing any completions that get calculated after .5s to be discarded -- is that what's happening?
We're interested in trying to make this patch better, but it's not clear to me how we should proceed. It may end up that this approach does work sufficiently well, but I'd hate for the idea to be thrown out simply because the initial patch was a very rough sketch. What would you recommend as the first flaw we should focus on in this patch? (Clearly there is some deficiency in how Eclipse works right now. Remy Suen heard me talking about this bug on #eclipse-dev when I first posted it, and got excited about the prospect of a fix. Perhaps the 500ms will help (though we've pointed out some problems with that approach as well). But it might be interesting to explore this approach further as well.)
If the timeout is hit you should see a red message in the status bar. Otherwise it's a different problem. My problem is that I requested a scenario / test case several times now in this bug report and so far did not get one. Code assist taking longer than 500ms (+/-) is not longer considered code assist (as per our original design) or considered a bug in the processor and hence I'm reluctant to change the current behavior.
Well, this puts us at a common problem from Eclipse developers: we have a large workspace which we cannot provide for you because it is proprietary. Is there something else we could provide instead? A CPU profile of some kind? What kinds of formats are you capable of reading?
First, which one is the slow processor? Is it one of the SDK or Mylyn or one of yours? Comment 14 mentions 15s (again by far outside of what we call usable content assist). Can you attach traces that show where the time is spent? I suggest you create a new bug for that because it only helps justifying this RFE but doesn't directly belong into this one.
Daniel, 1) to the best of my understanding, even the standard java classes completion will sometimes take more than 15 seconds. I'll work with Douglas to try to time it from the code. 2) emphasis on "sometimes". Let me stress we're not claiming it always takes this long. When all the indexes are up-to-date, and things are all "warmed out" (paged into memory), things will be much faster (though almost never "instantaneous"). Again, I'll try to give some measurements from the code's perspective, though it's sometimes hard to trigger the best corner cases. Z
Just to clarify: when you see the 15 second case: you get a dialog right? Otherwise that would be a bug. re 2: in that case code assist should be smarter and abort (or return the completions that aren't taking long). Code assist (as we see and designed it) should be almost as fast as typing and hence in the UI thread. Anything that's slow is a bug which deserves resources to investigate and fix.
No, I don't see a dialog (I have, sometimes in my life, seen a dialog that reads: "such and such completion has been disabled", but this is not what I'm talking about). To state is very clearly, the 15 seconds is UI-blocking (no way to cancel, pray to finish). I'll see if I can get a screencast... BTW, it's funny how my experience with the feature can be so different from yours: for me, it never really returns as fast as a keystroke (just popping the dialog, under optimal conditions, takes longer than a keystroke, though by a small amount), and quite often takes much longer. I wonder if what's taking a long time is not to collect the completions, but simply to popup the dialog (I generally use it under Linux/GTK2 -- a very beefed up box, btw). I tried in my Mac, and it does seem generally more responsive, but, again, I can't quite load my monster app in it to see what gives. Let me explore this further.
>No, I don't see a dialog (I have, sometimes in my life, seen a dialog that >reads: "such and such completion has been disabled", but this is not what I'm >talking about). This is very strange. We measure the time for each processor and if it's over 5s we show the dialog and this dialog cannot be suppressed. Are we really talking about the Java editor here?
Dani, Douglas and I spent some time trying to gather more information. We tested a vanilla Eclipse 3.5M3 (which includes your recent improvements). In general, things were a lot better. It was impossible to trigger a really long UI-thread lock-up. One bug we did manage to stumble upon was that, under normal usage, code completion would sometimes return an empty completion list (more on this below). One of the stress experiments I often perform is to forced the type indexes to rebuild. BTW, for as long as the indexes are being rebuilt, the completion list remains empty -- i.e. not only types, but also other kinds of completion (say, completing the "private" keyword). Anyway, I also realized another reason why completion behaves so much differently for you than for us: though Eclipse's code is also big, it compartimentalizes the types better than the project I have to deal with (single huge classpath). So I put together a simple program to help you experience what I do. It is "somewhat extreme", but it is the simplest way to get the point across. Start an Eclipse against a new workspace (make sure to pass -vmargs -Xmx2048M as an argument), create a brand new project with the class below, run it and refresh your project. The DrStein class creates 26 classes Z[a-z]s.java, each with 26^2 inner classes, all starting with "Z" as well, which is a reasonable number to see the slowness. At this point, simply type "Z" Ctrl-Space. In my computer, I get up to almost a second delay. I also quite often observe the empty list bug. import java.io.File; import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.PrintStream; /** * Dr. Stein creates a monster... Got it? :) * * @author zorzella */ public class DrStein { private static final char[] A_TO_Z = new char[]{ 'a','b','c','d','e','f','g','h','i','j','k','l','m', 'n','o','p','q','r','s','t','u','v','w','x','y','z'}; private void newFile(String prefix) throws FileNotFoundException { String className = prefix + "s"; File newFile = new File(String.format( "src/%s.java", className)); PrintStream out = new PrintStream(new FileOutputStream(newFile)); out.println("public class " + className + " {"); for (char second: A_TO_Z) { for (char third: A_TO_Z) { out.println("public static class Za" + second + third + " {}"); } } out.println("}"); out.close(); } public static void main(String[] args) throws Exception { DrStein stein = new DrStein(); for (char c : A_TO_Z) { stein.newFile("Z" + c); } } }
BTW, you asked if this was really the Java Type Proposals that was taking long (rather than some other plugin/editor). I took a screenshot (Eclipse 3.4) so you confirm for yourself.
Created attachment 119701 [details] Screenshot
Dani, did you have the chance to try DrStein?
No, sorry. I'm currently completely busy with other stuff. I hope to look at the test case next week.
I looked at the example and yes, it takes about a second (or two) to open the huge list and that's usable and very reasonable given the amount of proposals. The needed time for this rather extreme example is also far away from the 15s mentioned in comment 4. I didn't try yet how it feels when the proposals are coming in via background thread. However, for both variants a list with thousands of proposals is not very useful for the user. When trying with a longer prefix (e.g. "Zaa") content assist is very quick but still, the list is simply not manageable by the user. Note that using 20090331-0901 I was neither able to get the empty list nor the dialog (on a Thinkpad TP61).
Dani, thanks for taking the time to look into this. Let me address some of your points: 1) "I was neither able to get the empty list nor the dialog" I'll try a new eclipse build as soon as I get a chance. 2) "The needed time for this rather extreme example" Unfortunately, this "rather extreme example" is very close to my actual work project. To put in perspective, there are about 17K classes total in Dr.Stein (all starting with "Z"). In one of my projects, there are about 12K classes starting in "C" (10K more starting in "A" etc). 3) "is also far away from the 15s mentioned in comment 4." Very true. We have not experienced the 15 seconds under the 3.5 branch (which is what I intended to say on comment #19 "It was impossible to trigger a really long UI-thread lock-up"). I think it was related to garbage collection, and it *might* still be possible to trigger it, but I won't dwell on this point for now. 3) "However, for both variants a list with thousands of proposals is not very useful for the user." Not true: a) Popular completions. If I use a lot of "List"s, and I ask it to complete "L", it will have List as the first suggestion, even if there are, as in a Dr.Stein scenario, 1000s of theoretical possible completions. I can still get excellent mileage out of completion, if only it does not take me 2 seconds to show up. BTW, in this case, since I can type List much faster than 2 seconds, I'll do it instead of using auto completion... b) keyword/template completion. Kind of the same point as above, but for completing, say, "st" (to "static") or "main" ("public static void main(String[] args)"). All of these keyword/template completions are quick to come up (and few), and will (almost always) show up before the types (the lowercase match gives them preference). Also, looking for another angle, one of the points of this initial patch was to prevent a UI freeze under *any* circumstances, even at the presence of slow completions (genuinely slow, or buggy plugins) mixed with quick ones. Let me know your thoughts, Z
>Also, looking for another angle, one of the points of this initial patch was to >prevent a UI freeze under *any* circumstances, I don't think it will do this under *any* circumstances as spawning another thread that interacts with the UI thread *might theoretically* even raise the risk of deadlock ;-) With the 12K example it was around a second and if it's really too slow, simply type two letters before invoking content assist. The patch makes the code more complicated and harder to maintain and not the road want to go down at this point. Also, we do not want to encourage participants to be slow. Rather we want them to see the issue and fix their participant.
Just wondering: do you just have so many types in your workspace or are those generate classes which you would prefer not to see at all? If that's the case you could add them (with wildcards) or the containing package(s) to the type filter list. Just an idea.
> Just wondering: do you just have so many types in your workspace or are those > generate classes which you would prefer not to see at all? If that's the case > you could add them (with wildcards) or the containing package(s) to the type > filter list. Just an idea. It's a mix of hand-written and generated classes. I'm not sure what's the exact proportion (it's quite hard to count), but, even if there were many, and even if I did not ever want them to show in type completion (which is not true -- there are plenty of generated classes I want to be able to auto-complete), I would still be out of luck, since the generated classes share the same java packages as hand-written ones (though, one feature that would make my life a lot better would be the ability to have auto-complete disregard/never-suggest inner classes, unless qualified. I.e. "java.util.Map.Entry" would be a suggestion for "Map.E", but not for "En"). > I don't think it will do this under *any* circumstances as spawning another > thread that interacts with the UI thread *might theoretically* even raise the > risk of deadlock ;-) So you got me, "any circumstance" is not formally, strictly true. But you said it yourself: "might theoretically". I'm not talking, of course, about trying to prevent all bugs, or a maliciously crafted piece of code. I'm talking about being quite resilient to a naively-written piece of code, or even a plugin that can legitimately take a long time to complete. > With the 12K example it was around a second and if it's really too slow, > simply type two letters before invoking content assist. I'm failing to convey three points here: 1) I never know, beforehand, how long it will take, until I try it. With the degenerate project, two letters is enough. With my real project (where names are long, and they match in several ways, with camelcase and whatnot), 2 or 3 letters might not be enough. 2) Yes, 1 second is waaaaaay too long. I should get you a screencast of someone here using IntelliJ's autocomplete, and beating the heck out of the Eclipse guy trying to type the same thing. 3) There are plugins which really, legitimately might need some time to complete. I've proven to you I can even make the java type completion be (arbitrarily) sluggish, and java types are not the only completion in the world... > The patch makes the > code more complicated and harder to maintain and not the road want to go > down at this point. The design, fundamentally, is no more complex than Shift-Ctrl-T and Shift-Ctrl-R. I'm the first to say that part of the code is quite convoluted, but great part of the complexity comes from trying to be 100% backwards-compatible. If adding the complexity is a show-stopper, we can always fork auto-completion... But note: a) the complexity added is in *one* place that we write once. The code in each plugin, in fact, should be simpler (it has less things to worry about). b) this would be a path to get the process started. As Plugins are all migrated, we could tear down the complexity (though I heard that, once you publish an API you can never change it, so this would not do). > Also, we do not want to encourage participants to be > slow. Rather we want them to see the issue and fix their participant. See point #3.
>would still be out of luck, since the generated classes share the same java >packages as hand-written ones Right, but if you have control over the generator you could modify the generator to add those types which you don't want to see into the filter list.
Well, I don't really have control over the generator, and that's quite marginal to this discussion anyway...
Dani, so you know, this represents a real-world situation for a non-trivial number of engineers at Google, because we have a large code base. One might suggest that the problem is that our projects are too big, and hey, couldn't we do something to limit the size of our projects? And the answer there is maybe, maybe not. The projects that Zorzella work with are already trimmed to the minimal source required to build, test and run whatever it is he's working on, so we're not talking about the ENTIRE Google source code base, by the way. For us, and possibly for some other organizations, huge source code bases are definitely how we're doing things, and making the tool zippier for us would matter greatly.
Oh fun! I have a usecase for this... I need a completion proposal that goes and talks to p2... basically to see if you can import a bundle to your target platform if something matches (ie., a missing import or bundle that can be satisfied by a p2 IU) It would be interesting to see some of this work looked at in 3.6...
Chris, this rather sounds like a good candidate for a quick assist/fix. For things that take longer we use this approach (e.g. to fix build path problems).
*** Bug 410680 has been marked as a duplicate of this bug. ***
*** Bug 458725 has been marked as a duplicate of this bug. ***
*** Bug 463210 has been marked as a duplicate of this bug. ***
Is this possible to resurrect this idea? Ability to create additional, async IContentAssistProcessor, could be very usable feature. Especially in cases when processor need network access and/or should be cancelable by ESC.
+1 In my case I would like to benefit with this feature for JavaScript and TypeScript features: * tern.java https://github.com/angelozerr/tern.java * typescript.java https://github.com/angelozerr/typescript.java where tsserver gives the capability to retrieve compeltions with async mode.
If bug 481796 works out, it's possible that code completion may run fast enough that it would not be necessary to make it run asynchronously.
(In reply to Stefan Xenos from comment #39) > If bug 481796 works out, it's possible that code completion may run fast > enough that it would not be necessary to make it run asynchronously. Despite the patch which seems to be only about JDT, this issue is about the abstract completion for all text editors, so specific editors could reuse it for almost free. I believe the change to handle that are to happen in org.eclipse.jface.text.contentassist.ContentAssistant#computeCompletionProposals it might require a new interface for the content proposal providers to pass whether they're synchronous or not.
Good point. My index work will only help the JDT proposals.
> org.eclipse.jface.text.contentassist.ContentAssistant#computeCompletionProposals it might require a new interface for the content proposal providers to pass whether they're synchronous or not. It should really fantastic to provide that. Today many IDE liek VSCode, WebStorms provides the async completion. It should be very good too to provide async text hover (with a "Loading..." message). @Mickael Istria is there any chance that you could provide a patch for async completion? In my case, tsserver (TypeScript server) provides the capability to support async completion. For the moment, my TypeScript is synchronized (which could freeze Eclipse). It should be very cool if Eclipse provide this async feature. Thanks!
(In reply to Angelo ZERR from comment #42) > It should be very good too to provide async text hover (with a "Loading..." message). Please open another bugzilla entry for that. The issue and solution may be similar, but I believe they'll be part of different patches and tracked separately. > @Mickael Istria is there any chance that you could provide a patch for async > completion? Not soon, but I add this issue it in my endless todo-list ;) Seriously, if other development I'm working on goes well, I wish I can give a try by the end of December. > In my case, tsserver (TypeScript server) provides the capability to support > async completion. For the moment, my TypeScript is synchronized (which could > freeze Eclipse). It should be very cool if Eclipse provide this async > feature. So, this is more to avoid the UI freeze rather than having explicit asynchronous completion computation. I've read the previous comments, and as Dani mentioned, there are risks of having usability issues it mixing synchronous and asynchronous. However, if we 1st try to simply avoid this freeze, we can simply extract the work related to completion in a single thread, to avoid the content of changing while user is watching it. Most optimizations would remain working, and performant completion engines such as JDT ones wouldn't appear slower. It would just replace the freeze by a delay before completion pops-up when a long-running completion engine is invoked. Overall, it's a bug fix.
> Please open another bugzilla entry for that. The issue and solution may be similar, but I believe they'll be part of different patches and tracked separately. Done with https://bugs.eclipse.org/bugs/show_bug.cgi?id=487150
I plan to at least keep on investigating on this topic for Oxygen. If we want to more easily consume external sources such as tsserver to provide completion, it seems to be an important requirement.
*** Bug 487150 has been marked as a duplicate of this bug. ***
@Mickael, I think async completion will come important since you wish to integrate VSCode language server which uses CompletableFuture (Promise like) https://github.com/TypeFox/ls-api/blob/master/io.typefox.lsapi.services/src/main/java/io/typefox/lsapi/services/TextDocumentService.xtend#L47
New Gerrit change created: https://git.eclipse.org/r/85343
Tentative for M4
New Gerrit change created: https://git.eclipse.org/r/86644
Gerrit change https://git.eclipse.org/r/85343 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=aa9773529d30c379ea5c0b25683e698ce2fb4574
New Gerrit change created: https://git.eclipse.org/r/88878
Code completion tests for generic editor are failing https://hudson.eclipse.org/platform/job/eclipse.platform.text-Gerrit/796/testReport/org.eclipse.ui.genericeditor.tests/CompletionTest/testCompletion/ Maybe related to this patch?
(In reply to Lars Vogel from comment #53) > Code completion tests for generic editor are failing > https://hudson.eclipse.org/platform/job/eclipse.platform.text-Gerrit/796/ > testReport/org.eclipse.ui.genericeditor.tests/CompletionTest/testCompletion/ > Maybe related to this patch? Yes, definitely related. I noticed it yesterday and I'm keeping this ticket opento track the test failure. The main issue ATM is that I didn't manage to reproduce it locally.
Gerrit change https://git.eclipse.org/r/88878 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=b690018b20f5120373fee3406c476a2f84192951
(In reply to Mickael Istria from comment #54) > Yes, definitely related. I noticed it yesterday and I'm keeping this ticket > opento track the test failure. The main issue ATM is that I didn't manage to > reproduce it locally. Just moving to a CentOS slaved made the tests succeed.
Mickael, I was tempted to revert the change, mainly because there is no real use case that shows that this is really working. Testing the Generic Editor on .project files felt slower in async than "normal" mode. I think the behavior and API should have been verified on a real world example like the CDT or JDT editor before committing. There are also issues in the code: - AsyncContentAssistant is not needed, a new constructor on the original ContentAssistant is enough and addContentAssistProcessor can be public instead of protected - Same for AsyncContentAssistSubjectControlAdapter - getContentAssistProcessors should only be package visible I've fixed the above with http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=889d3c82bc8592914b11549d395f58ae6fb3d8ab Other open issues: - Code contains TODOs - The message in the proposal list is completely opaque for the user. What does "Computing (2/3) ..." mean? Seconds? Minutes? Remaining proposals?
(In reply to Dani Megert from comment #57) > mainly because there is no real use case that shows that this is really working. The use-case that drove this development are: * Usage of a language server to retrieve completion (in LSP4E, relying on Generic Editor) * Completion of installation units in the new .target editor > Testing the Generic Editor on .project files felt slower in async than "normal" mode. Ok, that's indeed unfortunate. I'll see what can be improved on that topic. > I think the behavior and API should have been verified on a real world example like the CDT or JDT editor before committing. It was tested against the JDT editor. https://git.eclipse.org/r/86644 shows the necessary changes to enable async content assist on JDT, and Sergey did approve the patch considering this JDT example. > There are also issues in the code: > - AsyncContentAssistant is not needed, a new constructor on the original > ContentAssistant is enough and addContentAssistProcessor can be public > instead > of protected > - Same for AsyncContentAssistSubjectControlAdapter > - getContentAssistProcessors should only be package visible > I've fixed the above with > http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/ > ?id=889d3c82bc8592914b11549d395f58ae6fb3d8ab Thanks a lot. I initially tried that but got a bit afraid of changing this existing class. I fully agree your version of this change is better. > Other open issues: > - Code contains TODOs I'll work on those. Some of them are obvious mistakes of mine, some other are more topics for discussion (such as whether we do really need a preference to set the max wait delay). If you don't mind, I'll use other tickets to track them. > - The message in the proposal list is completely opaque for the user. What > does "Computing (2/3) ..." mean? Seconds? Minutes? Remaining proposals? Remaining proposal processors. But indeed, it's not obvious, I'll soon propose a patch to improve it.
(In reply to Mickael Istria from comment #56) > (In reply to Mickael Istria from comment #54) > > Yes, definitely related. I noticed it yesterday and I'm keeping this ticket > > opento track the test failure. The main issue ATM is that I didn't manage to > > reproduce it locally. > > Just moving to a CentOS slaved made the tests succeed. It failed in the official build (also CentOS, GTK+ 2): http://download.eclipse.org/eclipse/downloads/drops4/I20170119-1010/testresults/html/org.eclipse.ui.genericeditor.tests_ep47I-unit-lin64_linux.gtk.x86_64_8.0.html ==> Looks like the asynchronous computation is not stable.
(In reply to Mickael Istria from comment #58) > > I think the behavior and API should have been verified on a real world example like the CDT or JDT editor before committing. > It was tested against the JDT editor. https://git.eclipse.org/r/86644 shows > the necessary changes to enable async content assist on JDT, and Sergey did > approve the patch considering this JDT example. That experiment should have rung a bell. As you noticed there, processors (e.g. to get the current selection or to show a message to the user) and even proposals are allowed to access the UI thread, hence when you switch to use the asynchronous content assistant you must be 100% sure that they do not do this. For JDT, we do not have that knowledge or control since they can be contributed via extension point. > > Other open issues: > > - Code contains TODOs > > I'll work on those. Some of them are obvious mistakes of mine, some other > are more topics for discussion (such as whether we do really need a > preference to set the max wait delay). > If you don't mind, I'll use other tickets to track them. That's fine with me. > > - The message in the proposal list is completely opaque for the user. What > > does "Computing (2/3) ..." mean? Seconds? Minutes? Remaining proposals? > > Remaining proposal processors. But indeed, it's not obvious, I'll soon > propose a patch to improve it. Note that the term "processor" is not known to the user, hence just adding the word "processor" to the message won't work.
(In reply to Dani Megert from comment #60) > (In reply to Mickael Istria from comment #58) > > > > I think the behavior and API should have been verified on a real world example like the CDT or JDT editor before committing. > > > It was tested against the JDT editor. https://git.eclipse.org/r/86644 shows > > the necessary changes to enable async content assist on JDT, and Sergey did > > approve the patch considering this JDT example. > > That experiment should have rung a bell. As you noticed there, processors > (e.g. to get the current selection or to show a message to the user) and > even proposals are allowed to access the UI thread, hence when you switch to > use the asynchronous content assistant you must be 100% sure that they do > not do this. For JDT, we do not have that knowledge or control since they > can be contributed via extension point. Indeed, legacy editors that have allowed completion processors to access UI Threads for a while and don't have metadata to declare whether work must happen in UI Thread (ie to declare whether asynchronous work is possible) will for sure have more difficulty in adopting this API. At the moment, I don't know a good way to handle that, and for such editors who used to run processors in UI Thread, moving to async completion can be perceived as a breaking change. A possible hack would be to try to catch the InvalidThreadAccess and if it happens, put a warning in the log and re-invoke synchronously/"blockingly" from UI Thread the completion processor... Do you think it'd be an acceptable piece of code to write? Or maybe there's a better way of handling this transition from UI Thread to non-UI Thread properly?
(In reply to Mickael Istria from comment #61) > Indeed, legacy editors that have allowed completion processors to access UI > Threads for a while and don't have metadata to declare whether work must > happen in UI Thread (ie to declare whether asynchronous work is possible) > will for sure have more difficulty in adopting this API. There are no legacy editors. Content assist is in JFace Text and like SWT it's clear that the UI thread can always be used unless explicitly stated otherwise. > At the moment, I don't know a good way to handle that, and for such editors > who used to run processors in UI Thread, moving to async completion can be > perceived as a breaking change. It is a breaking change. > A possible hack would be to try to catch the InvalidThreadAccess and if it > happens, put a warning in the log and re-invoke synchronously/"blockingly" > from UI Thread the completion processor... Do you think it'd be an > acceptable piece of code to write? No. We provide a stable platform to our clients, so, hacks are not acceptable. > Or maybe there's a better way of handling this transition from UI Thread to > non-UI Thread properly? A first step could be to add a new default method to IContentAssistProcessor which tells whether it allows to be called asynchronously. That way each processor could migrate separately and would ensure that it and its proposals don't access the UI thread. The asynchronous content assistant would only call processors asynchronously that specify to support it. At the moment I think specifying this at the proposal type level would be overkill. Another case we need to provide API for is how processors can report errors that are shown to the user (see bug 510743).
(In reply to Dani Megert from comment #62) > A first step could be to add a new default method to IContentAssistProcessor > which tells whether it allows to be called asynchronously. That way each > processor could migrate separately and would ensure that it and its > proposals don't access the UI thread. The asynchronous content assistant > would only call processors asynchronously that specify to support it. Ok. I'll do that.
Marking as FIXED. Open issues are tracked by separate bugs.
NOTE: This caused an NPE (see bug 511863).
(In reply to Mickael Istria from comment #54) > (In reply to Lars Vogel from comment #53) > > Code completion tests for generic editor are failing > > https://hudson.eclipse.org/platform/job/eclipse.platform.text-Gerrit/796/ > > testReport/org.eclipse.ui.genericeditor.tests/CompletionTest/testCompletion/ > > Maybe related to this patch? > > Yes, definitely related. I noticed it yesterday and I'm keeping this ticket > opento track the test failure. The main issue ATM is that I didn't manage to > reproduce it locally. This is now tracked by bug 511957.
It looks like the change that introduced asynchronous code completion has to be reverted until all the regressions it caused are addressed.
(In reply to Sergey Prigogin from comment #67) > It looks like the change that introduced asynchronous code completion has to > be reverted until all the regressions it caused are addressed. I plan to address bug 511542 with highest priority today. AFAIK, those are the 2 only ones that can be perceived as blocker at the moment (or maybe this report misses link?). If I didn't manage to fix it by the end of this day, feel free to revert if you think it's best. But as there are multiple commits involved, there are chances than the effort of reverting them in the right order and so on can be more complicated than actually fixing the bug.
(In reply to Mickael Istria from comment #68) > I plan to address bug 511542 with highest priority today. AFAIK, those are > the 2 only ones that can be perceived as blocker at the moment (or maybe > this report misses link?). What's the second one you refer to?
(In reply to Dani Megert from comment #69) > What's the second one you refer to? Sorry, I missed to add the link. I was thinking about bug 511957, which is not really a blocker, but which is quite annoying and deserve some immediate love too.
(In reply to Mickael Istria from comment #70) > (In reply to Dani Megert from comment #69) > > What's the second one you refer to? > > Sorry, I missed to add the link. I was thinking about bug 511957, which is > not really a blocker, but which is quite annoying and deserve some immediate > love too. Yes, please give some love to him :-)
