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.
The RequestProcessor.getDefault() is dangerous. From time to time I find profiling snapshots that contain many RP threads doing the same operation. Just yesterday I found about 40 threads in parsing.api: http://hg.netbeans.org/main-silver/rev/3596ca474291 Looks like almost all usages of RequestProcessor.getDefault() are wrong. Let's deprecated it. Jan Lahoda promised to help me with that: Jan will create a Java Hint to automatically rewrite the improper use as I did manually in #3596ca474291. Jan will apply the hint to whole NetBeans code base and attach it here. I will deprecate the method and update javadoc and apichanges. After a week of review I will apply both patches. Affected code shall be reviewed by appropriate team and adjusted if necessary (by changing the throughput - not too common imho).
(In reply to comment #0) > The RequestProcessor.getDefault() is dangerous. [...] > > I will deprecate the method [...] I don't think this is a good idea. There is nothing inherently harmful about using this method. If you expect some operation to be called dozens of times, then you should indeed read the Javadoc and make your own RP. But for a one-off action RP.default is fine: it is unlikely to be called again before the first completes, and even if so it is correct to run the second instance in parallel - excessive serialization of unrelated tasks can be harmful. You just need to look at the particular situation and decide what style of threading to use. I would rather suggest adding some kind of run-time diagnostic when an unreasonably large number of RP threads are active simultaneously, especially if they all use a Runnable of the same impl class.
My experience of evaluating .nps files tells me that usage of RP.getDefault() is source of huge amount of errors. Anyway your idea of detecting that in runtime sounds good. Implemented as core-main#2bc794b05dce Now we will see how often the RP.getDefault() is misused. I expect that soon the hint for converting to named RP will be really helpful.
(In reply to comment #2) > core-main#2bc794b05dce final String msg = "Too many " + c.getName() + " in non-private processor. Use own static final RequestProcessor RP = new RequestProcessor(...)!"; // NOI18N logger().log(Level.WARNING, msg, todo.item); I just saw this reported today (for AU), but the exception reporter dialog only displayed the stack trace, not the msg - so it made no sense at all to me as a user: a $SlowItem with no message. See if you can get the exception reporter to display this properly.
Apparent P1, IMO - an unreviewed controversial incompatible API change (the RP.getDefault().post never threw/showed/logged exceptions before this). I did a hint and applied to whole NB codebase+contrib: http://hg.netbeans.org/main/contrib/rev/3ade1b181591 Attaching two zips with patches: the compatible should not cause regressions (uses compatible throughput), incompatible can cause regressions (throughput == 1). Note the hint is not 100% tested (and not maintained), so make sure the changes build and work OK after you apply them. Also note that due to c120a96805ab, I was not able to module fix dependencies - I am working on a fix, but that is going to take some time and this apparently needed to be done very fast. The patch was applied for: java.editor java.freeform java.hints java.j2seplatform java.j2seproject java.navigation java.project java.source javadoc refactoring.api refactoring.java http://hg.netbeans.org/jet-main/rev/54825f112483
Created attachment 95821 [details] Compatible patches.
Created attachment 95822 [details] Incompatible patches.
[JG01] The compatible patches are pointless - they would just transfer the potentially poor performance of RP.default to a different form that is harder to search for. I don't think it is wise to do a blanket automated conversion of all usages of RP.default. A human being needs to look at each case and decide whether unlimited (or otherwise high) throughput is appropriate for that case, or whether serialization is appropriate (and will not cause deadlocks or starvation of some kind). The usefulness of the recently introduced warning is that it points out places where high throughput is actually encountered in realistic usage and might need fixing. [JG02] We should introduce a convenience ctor for RP taking Class rather than String so you could write just private static final RequestProcessor RP = new RequestProcessor(JavaEditorWarmUpTask.class, 1, false, false); ^^^^^^^^^^^^^^^^^^^^^^^^^^
Integrated into 'main-golden', will be available in build *201003260201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/a56fe8515fa8 User: Jesse Glick <jglick@netbeans.org> Log: #180458: log only once per class per session.
Integrated into 'main-golden', will be available in build *201003270201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/54825f112483 User: Jan Lahoda <jlahoda@netbeans.org> Log: #180458: not using RequestProcessor.getDefault() in java.editor java.freeform java.hints java.j2seplatform java.j2seproject java.navigation java.project java.source javadoc refactoring.api refactoring.java modules.
Another case where using RP.default is fine (to my knowledge) is if you only use it to create(...) a single task, which you then schedule(...) when necessary. This should not trigger the runtime warning.
(In reply to comment #10) > if you only use it to create(...) a single task (That is, really a single task for this class, not one per instance of the class, unless the class is a singleton.)
Created attachment 96732 [details] Convenience constructor I can hear a request for convenience constructor. Anything else that shall be done?
Yes, you should blog about it -- explain how RequestProcessor.getDefault works, why it is wrong, and how it should be done instead.
I will integrate the convenience constructor tomorrow and also extend the javadoc to describe what Geertjan requested (blog is not suitable place for documenting API, imho javadoc is better).
For the record: I have never said that the right place to document an API is in a blog. (However, if you're not going to blog about important changes to an API, the change is less likely to be noticed.)
core-main#625d6a876c6e - language fixes appreciated. Blogosphere publicity too.
Integrated into 'main-golden', will be available in build *201004131450* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main/rev/625d6a876c6e User: Jaroslav Tulach <jtulach@netbeans.org> Log: #180458: Prevent parallel misuse of RequestProcessor.getDefault: Offer easy to use convenience constructor.
I think the warnParallel=3 limit needs to be reconsidered. Since this change, quite a lot of bug reports have been produced by this stack trace, resulting in a significant amount of work to replace old calls to RP.gD with custom RP's. But in my experience hardly any of these cases involved any real performance problem. Rather, some user-initiated action needs something to be replanned into an RP thread, and on rare occasions the user clicked some button three times or whatever. Running those three tasks in parallel would usually have been harmless. Nor is serializing those tasks necessarily better for performance; if they are mostly blocking on something else (e.g. network), it can actually make responsiveness worse to use a custom RP(Class) with throughput=1. More useful IMHO would be track when the total number of RP threads in the process which are in runnable mode (i.e. not just blocked on some lock), or blocked in local disk I/O, exceeds some threshold of reasonableness beyond which they are likely to all just be thrashing one another. In such a case, report an exception with a stack trace blaming whichever RP name has contributed the most parallel threads. This would catch the actual rare abuses like the initially reported overparallelization in parsing.api, or property change "firestorms" in some node class, etc.