This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
We currently have the slowness detector with autoreporter. Often, the only way to fix a slowness issue is perform the action in a different thread, but block the user from doing anything (while allowing the AWT Event Dispatch Thread to repaint). I do not think it is feasible to force each and every feature to implement this (would cause inconsistencies, bigger code, more consumed memory/permgen, etc.). I think there should be a helper method for this, e.g.: runOffAWT(Runnable toRun, String featureName, AtomicBoolean cancel) (the cancel would be set to true by the method if the user would press Cancel button on a wait dialog, the Runnable should act accordingly) The method should IMO work like this: -for a short time, show a wait cursor and block the user. -if the action takes a longer time, show a dialog with warning and Cancel button Normally, most actions should run quickly enough to prevent appearance of the Dialog.
http://hg.netbeans.org/jet-main/file/tip/java.editor/src/org/netbeans/modules/editor/java/RunOffAWT.java
http://hg.netbeans.org/jet-main/diff/tip/csl.api/src/org/netbeans/modules/csl/editor/hyperlink/RunOffAWT.java
Another new copy created: http://hg.netbeans.org/main-silver?cmd=changeset;node=5b5c28b78bb7
*** Issue 173990 has been marked as a duplicate of this issue. ***
Bugs (about duplicated code) are depending on this issue and as such it shall have higher priority than just an enhancement.
+1 There used to be (and very likely still is) a similar helper method in J2EE utilities.
We should probably consider also non-cancel version of the call. Warning message is meant to be customizable?
We've got P2 issue #174198, which needs this as well. Thanks a lot!
Created attachment 89787 [details] patch
Please review.
[JG01] Missing class Javadoc. (2x) [JG02] Missing private null constructor. [JG03] You cannot run "off Abstract Window Toolkit". You can run off the "[AWT] event queue [thread]", or EQ. Compare java.awt.EventQueue for terminology.
[JG01-02] - I will add it [JG03] - runOffEDT() or runOffEQ()?
JG03 - not sure (EventQueue Javadoc variously refers to "the dispatch thread of the system EventQueue" or "the event dispatcher thread" or "the current AWT EventQueue's dispatch thread" or "the current AWT EventDispatchThread") but whichever you pick it is probably better to spell it out, e.g. runOffEventDispatchThread
I'd like to suggest that there is a bigger issue within this development. What I see happening, is an attempt to provide a low level API that runs some code in the background, watches it, and provides the user the opportunity to terminate (not sure how you will make that happen reliably) or ignore (this seems perilous) the work. While this is one way to manage the issue, it seems that over all, we are not focusing on the fact that the user needs to be impacted the least possible by all of the work the IDE is doing. These things that take a long time to do, should be few and far between, and probably need very, very specific facilities to allow cancellation to work. I don't really think that this API, in and of itself provides a great solution to this issue. It seems like there should be a class with abstract methods that users can implement and then plug their work parts into. The SwingWorker kind of layout with "setup", "background-work", "post-background-work" is the kind of thing I am thinking about. I'll attach a class implementation which I think makes the next step to help manage the issue that we'd like background processing to just happen in the background, and when it completes, something can change in the IDE, but there are not so many times that the user should really be kept from doing other work, and editing in particular needs to be possible in all but the most extreme cases, such as refactoring.
Created attachment 89796 [details] An example class providing Future implementation for background processing.
When I reported this enhancement (was enhancement at that time), I did not include a particular example of an action that should use the envisioned runOffAWT method. Let me, first of all, correct this mistake. This method is supposed to be used by user invoked actions, that are expected to run fast (but, as the IDE is not a realtime system, it cannot be guaranteed to run fast). An example is Go to Declaration. In vast majority of cases, this action should run very fast. In some, hopefully rare (modulo scanning), cases it may take a while to execute. For me, Go to Declaration is a foreground action - I wait for it to finish (when I use it to go to a method, I want to see that method). In case it would take a long time, I may opt to cancel it and find the target of the action manually, but that certainly should not be common. Do you really want to be able to e.g. edit the source, despite the fact that the new target can be opened at any time? Note that in 6.0, this action ran completely in AWT thread, in 6.1 or 6.5 (do not remember which) a check for scanning in progress was added, but if the scanning was not running, the action ran in AWT anyway. Note that the runOffAWT method is not supposed to be used by action that are expected to take a longer time under "normal" circumstances. Regarding the proposed ControlledProcessor class, I have a few comments/questions: -what is the intended use of the class? What features should use it to achieve what result? What advantages it provide compared to simple use of NB's RequestProcessor? Sorry, but I do not see much advantage of using this class (but I do not know what particular usecases it should solve). -I do not see the advantage of the setup(), performWork() and after() methods - these are executed in order in one thread - what is the benefit of having separate methods over simply performing the actions in one method (or calling separate methods manually, if that is appropriate for improving readability). -SwingUtilities.invokeAndWait should be used with maximal care - it is one of the most dangerous methods I know of. Note that in NB one can use Mutex.EVENT.readAccess(Action) (not that I would recommend using that).
BTW Mutex.EVENT.readAccess does not block when off EQ - it is like invokeLater. It is synchronous when on EQ.
Y01 Gregg, I know that the runOff method may easily be misused, but I rather see misuse of one API method than copy pasting the same code into many places in the IDE. Anyway that is not the question: The question is, what you mean by your comment? Do you believe that the patch, as produced by Tomáš Holý and after a bit of polishing, shall not be integrated at all? If so, you need to change API_REVIEW_FAST keyword into API_REVIEW before or when Tomáš Holý announces 24h countdown before integration. Y02 I like Jan Lahoda's explanation of the method's purpose. Something like this shall be added to method's javadoc. Y03 To avoid misuse, I suggest to add some detection of slowness. According to Jan's explanation following might work: Have a Map<Class<? extends Runnable>, Integer> that will remove(operation.getClass()) whenever the operation finishes faster than in 100ms. Otherwise if ((put(clazz, get(clazz) + time)) > 10s) /* at least two slow invocations in a row taking together longer than 10s */ LOG.log(Level.WARNING, ..., new Exception(clazz + " is too slow")); Y04 Gregg, if you believe that your new class shall be used instead of here-in proposed patch, start new review or wait for conf call (which will have to happen if you change the keyword as suggested in Y01). Y05 Don't use RP.getDefault(), use named RP with throughput one. Y06 The Trivial implementation is buggy, does not wait for end of background processing. Y07 The same test shall test both implementations. You can subclass the test in progress.ui and just provide different provider.
My knowledge of all internal methods of netbeans and how requests for work are dispatched for various reasons, is not that great, so you will have to help me understand which API I am missing out on the use cases for. The example class that I attached, it about providing an interface for GUI actions to use to access the underlying workings of the platform (and IDE in this case) so that if there needs to be some rewiring or rearchitecting of how things interact with the core of netbeans (the big problem with the EDT encountering the scanning/indexing locks is an example of wide spread problem because there was no "standard" way to do that "correctly"). I am suggesting Future because it means using a core Java standard for doing something in the background and getting the results from multiple thread contexts. I really want to push the idea of EDT listeners looking like protected void performAction( Node[] activatedNodes ) { ControlledProcessor cp = new ControlledProcessor() { .... }; RequestProcessor rp = ... rp.post( cp ); } The idea being that with the ControlledProcessor (or something like it) in place, there is a way to completely manage the orchestration of tasks that need to use background execution and EDT work as well. I did not make setup() and after() run in the EDT, but we could decide that was the right thing to do so that it was more inline with SwingWorker (That's really the model I am trying to push). Being a Future means that you can pass the object of to some other thread of execution that can use Future.get(...) as needed to get the result. In the case of these progressively exposed control mechanisms (first busy cursor, then dialog), overriding get() logic can use timed get() calls to implement the progression of control. There are a lot of things in netbeans that do already exist to manage this issue, but all of those things, to me anyways, don't fit together into a model that helps manage this issue. Instead, every developer has to make judgements based on their knowledge of the issues to create the "correct" interaction with the system. I've done tons of GUIs using Jini services as the "engine" that the GUIs talk through. That means that every action and interaction is a remote call with all kinds of possible partial failures to deal with. I've always been able to just use the SwingWorker model (extended with the features that I've discussed on the list regarding context class loader and JAAS Subject etc) and not had problems with that affecting how things work. I use control disables on action invocation to keep things from being requested twice (this makes locking a lot less needed), and the stuff just works. Hopefully, I've answered all the questions that were asked, I tried to cover all the things that were behind this suggestion. I'm not asking for the IDE to be rewritten, I'm asking for you guys to carefully consider how much control (or lack thereof) you make developers have to implement themselves to get stuff done and to make sure the IDE is usable and performant with all the things that are happening at the core as well as in the background related to individual features. RequestProcessor is a fine thing to use to manage when things get done and how much parallel activities are going on. Adding a class to wrap the work so that the EDT and non-EDT activities are programatically together, and logically sequenced, I thinkm, greatly simplifies the success rate of the developer.
Created attachment 89923 [details] updated patch
First of all thank you for this API, I like it and I vote for having it. Now my questions: VS01: Can I use this API for tasks that are not cancelable? VS02: And for tasks that are cancelable up to a certain point, which when reached makes the task non-cancelable? VS03: How is the UI going to look for these tasks? Should tasks somehow signal their state to the UI? VS04: What if a task is canceled, but does not finish? How is the UI going to react to this situation? Btw. I think that the vast majority of tasks that will use this API are going to be non-cancelable tasks. Tasks that run fast under normal circumstances and hence are not implemented in a way that allows canceling.
[MM01] Shouldn't the class be split into a generic org.openide.util part and a java support specific part? I know we can do it anytime later but IMHO we could agree to do it even now.
Does anyone have any further thoughts or comments on what I added to this issue? I still sense that it seems okay to netbeans developers to run "anything that is fast" on the EDT, when the reality of the impact that has when 10 modules are running 20ms tasks is a 200ms delay in EDT availability. The IDE is expandable and each module can have a feeling that their work will run fast, but the reality is that if 10 modules react to an editor event and quickly stick work in the EQ that is just 20ms that suddenly there is the 200ms delay between events that really starts to become visible and make the IDE feel clunky. I really feel like the overall platform has to be considered here, and individual modules and bits of code acting on events must consider that anything they run on the EDT has the ability to greatly impact the user experience.
"Does anyone have any further thoughts or comments on what I added to this issue?" - I don't.
"Does anyone have any further thoughts or comments on what I added to this issue?" - Gregg, I really like one of your statements: "AWT thread shall be used only for painting". It is obviously overstated goal, but such are all the tasks I like. However originally this issue is about removing duplicated code, so commenting on your statement or your diff seems to be out of the scope of this issue, at least from my point of view.
jtulach: I understand what this issue is about. What I'm worried about is that this solution is too simple and doesn't provide enough structure and is not focused enough to help deal with the bigger issue of keeping people from doing inappropriate work on the EDT. This issue helps address how timeouts and user cancellation are presented, but doesn't, I feel, address the needs of the developer to have this accessible to them in a way that they can truely make cancellable tasks as well as "see" how to structure the work as a sequence of "setup GUI for task", "perform task that might run for a while", "reconfigure the GUI after the task". I understand that this issue is a bit lower level in detail, but I am concerned that people will still just choose to run "whatever" stuff on the EDT, and only use this API when they want "cancellation" or a "man this is running a long time" capabilities. So, I'm trying to stress that if you provide the API this issue is about, that you really could raise the bar and provide something like what I suggested and say that this style of processing is preferred. I really think this would help people be much more clear on how to deal with events from the GUI and help make the processing on the EDT be much reduced to help the usability of the IDE (and platform) was well as to provide future ability to make changes in how threading is handled in more central places.
Well, we could move the discussion that Gregg wants to have back to the [openide-dev] EDT Was: New Language Support Issues thread. There is a social issue I've been thinking about bringing up. People who are co-located can discuss things "unproductively" at lunch or by the coffee stand or at the bar. For those of us who are isolated it becomes a bit frustrating since most of the communication venues are too formal or structured. So we end up with all this stuff we want to talk about but no outlet for it.
I suggest that the discussion about Gregg's proposal moves to a new issue (instead of openide-dev) that would be created by Gregg and "maintained" by Gregg. Gregg if you want your proposal to materialize you must provide close to final patch (not just a sketch) + answer the concerns raised by the people in the issue.
Re VS01-03: Interesting questions. For tasks that would not be cancelable at all, a null cancel parameter could be used. But, I think that most of the tasks would be "cancelable up to a point" - I will try to think about that (unless Tomas objects). Anyway, from the user's point of view, I think that we should try to make the tasks as cancelable as possible. Imagine that the user invokes a quick action and is blocked for a long time without possibility to cancel the action. I would not like such a situation (even if it was rare). VS04 (IMO): the canceled task will not probably finish immediately (it may wait on a condition and may not be able to check for cancel value), but should not have any effect. The UI should disappear.
I think only cancelable task should be supported. If this method should support also non-cancelable task calling this method is not much different than blocking EDT. UI will be painted but user will not be able to do anything than terminate NB.
I have created an issue and put a module project up as an attachment for people to look at for now. http://www.netbeans.org/issues/show_bug.cgi?id=175342
Created attachment 90269 [details] updated patch
After discussion with jtulach I made small change. If the task is canceled the method waits until task is finished. If it does not finish in 1s an exception is thrown. This should help in situation when it is necessary to prevent return from runOffEDT before task is finished. If there are no objections I will integrate after 24h.
I am not sure about this last change. Considering my usecase with Go to Declaration, it is very probable in this context that the task will not finish in the limit. The reason is that if the action is block for so long (~10s), it is probably because an indexing is in progress. In such a case, the action is waiting on a lock and is not checking cancel. It checks the flag when the scan finishes and the task is started, but this quite likely will not be in the time limit. So this new updated implementation does not seem useful for the Go to Declaration usecase.
OK, so waiting for canceled task should be probably optional.
Created attachment 90278 [details] updated patch - waiting for canceled task is optional
Looks like the amount of method arguments is getting more and more complicated. Here is a way to eliminate this complexity: Y11 Use cummulative factory or builder (http://wiki.apidesign.org/wiki/CumulativeFactory). In this case it would be: public class RunOffEventQueue { public static RunOffEventQueue create(Runnable r, String decr, AtomicBoolean cancelOp); public RunOffEventQueue waitForCancel(boolean wait); public RunOffEventQueue waitCursorAfter(int ms); public RunOffEventQueue ....(...); public void execute(); } with a sample usage: RunOffEQ.create(...).waitCursorAfter(1000).execute(). This is just an advice, if you do not like it integrate the patch with static methods.
I am fine with the optional waiting for cancel.
Thanks for review.
core-main #47c0ebae455e
Integrated into 'main-golden', will be available in build *200911030222* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/47c0ebae455e User: Tomas Holy <t_h@netbeans.org> Log: #170882: runOffAWT method