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.
Summary: | New file type wizard instantiate is slow and run in EDT | ||
---|---|---|---|
Product: | platform | Reporter: | _ rkubacki <rkubacki> |
Component: | Dialogs&Wizards | Assignee: | Jiri Rechtacek <jrechtacek> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | issues, issues, madamek, mkleint |
Priority: | P3 | Keywords: | API_REVIEW_FAST, PERFORMANCE |
Version: | 5.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | DEFECT | Exception Reporter: | |
Attachments: |
proposed patch
backward compatible patch |
Description
_ rkubacki
2005-08-11 14:26:47 UTC
not sure how I can influence the performance here from the actual wizard. Maybe the wizard framework should allow performing instantiation on the background? > not sure how I can influence the performance here from the actual wizard You can. Take a look into the Action wizard which uses background_loading/please_wait_model combinations. There I need to fill up nine combos with SFS values. > Maybe the wizard framework should allow performing instantiation > on the background? Would be nice to have :) sorry martin, this is a different usecase. I'm not about to populate the wizard. I'm creating the files in instantiate() method. And that one has to return the created files in the return statement's set. Still the same problem of our wizard support - instantiate runs in EDT and there is no way how to move it to backgroud and be able to return created objects so they get selected/opened. reassigning to openide/wizards then? Yes, seems to me like instantiate() should be called on a RP thread - maybe with an automatic progress task, or this could be left up to the individual wizard? - which would collect the results and then select them in EQ. Correct component may be projects/ui, but I'm not sure where this is really handled. Same case for New Project wizard - sometimes just takes some processing to create everything, and ought not block EQ during this time. Not sure if there are any issues associated with having EQ unblocked and no modal dialog while instantiate() is running. Ideally the last wizard panel would show a progress bar, but that is a tougher change to make. Created attachment 26574 [details]
proposed patch
What the attached patch does? When the Finish button is clicked then WizardDescriptor disables all movement buttons and writes a message The Finish in being processed... and replann calling InstantiatingIterator.instantiate() out of AWT in RP task. When instantiate() is done then WizardDescriptor does the rest of work in SU.invokeLater() e.g. reset and close the wizard dialog, firing a closing option etc. It means AWT is not blocked during instantiate() processing. An wizard can paint any progress indication or whatever. Martin, could you please try this patch if it would help you in your wizards? Thanks The value of MSG_WizardDescriptor_FinishInProgress will certainly have to be changed. I tried proposed patch with J2EE CMP wizard and it works as expected. Decision if we will use this I have to leave to HIEs. There is the proposed change to invoke WizardDescriptor.InstantiatingIterator.instantiate() outside EDT. There is no explicit contract that instantiate() can/must be called in EDT thus instantiate() can be called outside EDT. Covering tests and a change of WizardDescriptor will be provided. Should be easy to fix Wizards API clients to adjust this change. If a strong objection to this change, it's possible to enlarge Wizard API with a AsynchronousInstantiatingIterator which will force to be invoked outside EDT and InstantiatingIterator will left in EDT. I prefer the first one. Comments? Thanks This patch can have unpredictable impact to Wizards API clients. E.g. try to create new java file during background scanning. Without this patch the template is instantiated instantly. But with this path user must wait for end of class path scanning (for minutes). Other iterators may be broken as well... I know, that there is no explicit contract, that instantiate() is called inside EDT, but it simply worked this way. Java wizard is asking for mutex but it is not treated as a priority request because it is not called from EDT. Is it what happens? Exactly. > InstantiatingIterator.instantiate() vs. Classpath.scanning
Wrong premise of a mutually blocking, the background scanning *is* started after
the wizard ends for certain. I don't see as a problem.
Is there any next questions where it could fail? At least commit validation
suite works for me :-) also when I test a several wizards based on
InstantiatingIterator then no problem I have caught.
Created attachment 27372 [details]
backward compatible patch
After appeal for backward compatible of threading the instantiate() method. Although instantiate() hasn't been declared to be called in EDT explicitly, any API clients can depend on that. I propose add new iterator based on InstantiatingIterator which method instantiate() is processed asynchronously and original iterator InstantiatingIterator works as before. See the attachment backward_compatible_patch.diff with this new iterator. Thanks For whom exactly is there a backward compatibility problem with the original patch? I don't think we should clutter the APIs with extra interfaces unless there is a definite reason for it. As I am the one lobbying for compatible development, I feel I should answer. "For whom exactly?" For our customers. The API is described as stable and as such incompatible changes in it shall be minimized. Until now the Wizard api was AWT thread only (as mentioned for example at http://core.netbeans.org/proposals/threading/) and changing it may have severe impact (as illustrated by the worries by jbecicka). This is something that shall not be done for an API in wide use. Interesting thing happened during yesterday: Two approaches were discussed: #1 do the incompatible change(old patch), #2 add new interface and let the module writers decide whether they want AWT/non-AWT behaviour(new patch). It was pointed out that if we do #2 many module writers will refuse to switch their implementation as we are 10 days before code freeze. This was intended as an argument that #1 is better, but actually it shows why we cannot do #1 - it is too risky. If individual module writers refuse to switch off AWT thread, then we have no right to make that switch for them. That is why I believe #2 is better way. It gives people chance to make conscious decision to do the switch while it fully preserves functional compatibility. It is not going to magically heal the world, but it will give people a medicine that they can use to cure themselves, if they need it. PS: If the problem is just the additional interface, we can use WizardDescriptor.getProperty("runInitializationOffAWTThread") == true to get the same behaviour ;-) Thanks for review, I'm going to integrated new AsynchronousInstantiatingIterator tomorrow. Checking in apichanges.xml; /shared/data/ccvs/repository/openide/dialogs/apichanges.xml,v <-- apichanges.xml new revision: 1.5; previous revision: 1.4 done Checking in manifest.mf; /shared/data/ccvs/repository/openide/dialogs/manifest.mf,v <-- manifest.mf new revision: 1.6; previous revision: 1.5 done Checking in test/unit/src/org/openide/InstantiatingIteratorTest.java; /shared/data/ccvs/repository/openide/dialogs/test/unit/src/org/openide/InstantiatingIteratorTest.java,v <-- InstantiatingIteratorTest.java new revision: 1.2; previous revision: 1.1 done RCS file: /shared/data/ccvs/repository/openide/dialogs/test/unit/src/org/openide/AsynchronousInstantiatingIteratorTest.java,v done Checking in test/unit/src/org/openide/AsynchronousInstantiatingIteratorTest.java; /shared/data/ccvs/repository/openide/dialogs/test/unit/src/org/openide/AsynchronousInstantiatingIteratorTest.java,v <-- AsynchronousInstantiatingIteratorTest.java initial revision: 1.1 done Checking in src/org/openide/Bundle.properties; /shared/data/ccvs/repository/openide/dialogs/src/org/openide/Bundle.properties,v <-- Bundle.properties new revision: 1.3; previous revision: 1.2 done Checking in src/org/openide/WizardDescriptor.java; /shared/data/ccvs/repository/openide/dialogs/src/org/openide/WizardDescriptor.java,v <-- WizardDescriptor.java new revision: 1.16; previous revision: 1.15 done |