Bug 182698 - Menu construction creates Swing components on RP thread
Menu construction creates Swing components on RP thread
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Actions
6.x
Macintosh (x86) Mac OS X
: P1 (vote)
: 6.x
Assigned To: Jaroslav Tulach
issues@platform
: RANDOM, THREAD
Depends on:
Blocks: 182695
  Show dependency treegraph
 
Reported: 2010-03-24 17:14 UTC by _ tboudreau
Modified: 2010-04-07 04:46 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
:


Attachments
Exported changesets (171.80 KB, patch)
2010-03-27 22:40 UTC, _ tboudreau
Details | Diff
Updated changesets (379.05 KB, patch)
2010-03-28 08:58 UTC, _ tboudreau
Details | Diff
Bundled changesets (41.18 KB, application/octet-stream)
2010-03-29 01:05 UTC, _ tboudreau
Details

Note You need to log in before you can comment on or make changes to this bug.
Description _ tboudreau 2010-03-24 17:14:42 UTC
See thread dump attached to deadlock bug 182695.

I thought we were already handling the special case of JSeparators being constructed off the event thread - it should be simple enough to, say, test explicitly for "javax.swing.JSeparator" and, if present, construct an Action that implements Presenter.Menu and will provide a JSeparator when asked (on the event thread) for its presenter.  There's no reason we need to actually construct JSeparators when resolving a menu folder.

"Folder Instance Processor" daemon prio=1 tid=0x00000001025c7000 nid=0x15cc32000 waiting for monitor entry [0x000000015cc30000]
   java.lang.Thread.State: BLOCKED (on object monitor)
	at java.awt.KeyboardFocusManager.clearMostRecentFocusOwner(KeyboardFocusManager.java:1777)
	- waiting to lock <0x0000000108896600> (a java.awt.Component$AWTTreeLock)
	at java.awt.Component.setFocusable(Component.java:6965)
	at javax.swing.JSeparator.<init>(JSeparator.java:88)
	at javax.swing.JSeparator.<init>(JSeparator.java:70)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:39)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:27)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:513)
	at java.lang.Class.newInstance0(Class.java:355)
	at java.lang.Class.newInstance(Class.java:308)
	at org.openide.loaders.InstanceSupport.instanceCreate(InstanceSupport.java:217)
	at org.openide.loaders.InstanceDataObject$Ser.instanceCreate(InstanceDataObject.java:1380)
	at org.openide.loaders.InstanceDataObject.instanceCreate(InstanceDataObject.java:818)
	at org.openide.loaders.FolderInstance.instanceForCookie(FolderInstance.java:585)
	at org.openide.awt.MenuBar$LazyMenu$MenuFolder.instanceForCookie(MenuBar.java:651)
	at org.openide.loaders.FolderInstance$HoldInstance.instanceCreate(FolderInstance.java:1135)
	at org.openide.loaders.FolderInstance$1R.instances(FolderInstance.java:705)
	at org.openide.loaders.FolderInstance$1R.run(FolderInstance.java:726)
	at org.openide.util.RequestProcessor$Task.run(RequestProcessor.java:1356)
	at org.openide.util.RequestProcessor$Processor.run(RequestProcessor.java:1894)
Comment 1 _ tboudreau 2010-03-24 18:41:33 UTC
> test explicitly for "javax.swing.JSeparator" and, if present, construct an Action
> that implements Presenter.Menu and will provide a JSeparator when asked (on the
> event thread)

Actually, such a test could be generalized for menus and toolbars to look up the class name, see if it inherits from java.awt.Component and solve the problem for any Component instance someone wants to declare in such a folder.  To make it general-purpose to any folder, you could make it conditional on some attribute of a parent folder.
Comment 2 _ tboudreau 2010-03-27 09:24:04 UTC
See prototypes repository branch menus-rewrite-182698 for a much simplified rewrite of the menu infrastructure.  Everything basically works, and looks correct on-screen.  There are tests to ensure menu items can be garbage collected when a menu is off-screen.

What it does:  
 - Works very much like contrib/menus did.  No custom menu or menu item subclasses, does everything with normal listeners.
 - Classloading is performed on a background thread during startup
    - A model is constructed of the files under the menu folder
      - There are model element subclasses for regular Actions, DynamicMenuContent, Presenter.Menu and random Components - so each special-case is dealt with cleanly in its own class
      - Each model element is a factory for 0 or more JComponents
      - Folder-based model elements use a similar mechanism to DynamicMenuContent to update the components
 - Component construction runs on the event thread
 - Non-component instances are instantiated on the background thread and held
 - Adds one API class with one method - MenuProvider.installMenu (JFrame)

To-dos:
 - Seeing doubled text on a couple of menu items, using 2 fonts - need to figure out what that is - some special-case I haven't discovered (I once knew why this could happen :-/)
 - Tests to make sure file change listening is working - it should be
 - Some InstanceCookies lie about providing a Presenter.Menu - you have to instantiate the object -would be nice to find a way to avoid this
 - Listen for PROP_COOKIES and possibly update if InstanceCookie is replaced?

Should not be too much work to get this completely working.
Comment 3 _ tboudreau 2010-03-27 09:28:24 UTC
Probably will possible to delete the menu warm-up task - classloading is taken care of on a background thread ahead of time, there is no longer any waiting-for-initialization.  At least on my mac, first-time populating of menu components is < 100ms, and subsequent calls are between 7 and 20ms.
Comment 4 _ tboudreau 2010-03-27 21:22:23 UTC
Listening on folders for changes is now verified working, w/ tests (useful trick:  since such a tests *depends* on menu popups actually opening, etc., the user can interfere with the test passing - BUT, if you install an AWTEventListener which consumes all input events unless they are a special MouseEvent subclass, you can actually make it impossible for the user to directly interfere - only events posted by the test are actually received by any components).

All menus look and behave exactly (as far as I can tell) as they do with the original MenuBar/FolderInstance.  Main differences in code:
 - File changes affect the UI on a delay (rescheduled task in a 1-thread RP)
 - No fancy code to make a popup menu resize while open (I think this stuff was from NetBeans 2.0 anyway) - on a change, an open menu just hides and re-shows itself to handle this

Questions now are really performance-related - really just need to measure what represents the best trade-offs between laziness vs. memory-usage - all should be cleanly solvable, it's just a question of what works best:
 - The model should probably use one recursive FileChangeListener and be able to look up child nodes by path to refresh them
     - Currently each model object for a folder holds its DataObject and listens for changes on that folder
 - Currently, the model holds references to all DataObjects under Menu/.  It would be easy enough to change it to allow the subtree underneath the main menus to be discarded - but we would have to reconstruct it to post a menu, so that might be bad for responsiveness
 - Currently, model objects for Actions hold a reference to the Action for their entire life (part of the background preloading strategy - we do construct things which are safe to construct on a background thread, to keep that work out of AWT).  They don't have to, but this does mean moving more work to the event thread.
 - Menu model construction and menu bar population could be moved to after the main window is shown, to avoid thread contention and work building the model during startup - easy enough - just move the init() call that starts the background menu loading to be triggered by the menu bar getting a peer
 - Screen menu bar responsiveness on Mac OS when moving the mouse quickly across menus is visibly faster if the main menus are *not* depopulated on hide - might want to special-case this for Apple, but it does mean that invisible actions on closed menus will be updated on selection changes - need to measure the impact

So, there are lots of choices for what is best for performance, memory footprint and responsiveness;  and all of them are doable.
Comment 5 _ tboudreau 2010-03-27 22:40:11 UTC
Created attachment 96078 [details]
Exported changesets

Since I'm running into some issues with pushing to the prototypes repository (no remote changes, but push creates new heads, and push -f fails with "ssl required"), attaching exported changesets with the rewrite.
Comment 6 _ tboudreau 2010-03-28 08:58:22 UTC
Created attachment 96092 [details]
Updated changesets
Comment 7 _ tboudreau 2010-03-28 11:21:43 UTC
Turns out the attached files cannot be applied on Windows.  I'll do a few more changes tomorrow and attach a changeset bundle.
Comment 8 _ tboudreau 2010-03-29 01:05:28 UTC
Created attachment 96121 [details]
Bundled changesets

AFACT, menus working and looking as they do with MenuBar.

Note:  Currently, the initial menu bar initially just contains a placeholder containing only File > Exit.  All initialization (background and foreground) is delayed until the menu bar is physically shown on screen.  If this is undesirable, initialization can be started during startup; main window showing could also be delayed until the menu bar is populated (as with MenuBar), if desired.
Comment 9 Jaroslav Tulach 2010-04-06 07:51:14 UTC
Fixed in core-main#514c7d1a4263

Tim, if you want to rewrite the Menu system, get ready for integrating as soon as 6.9 is branched. Keep the work in another issue. Make sure there is a way to turn previous system on (probably via branding) to keep compatibility.
Comment 10 Quality Engineering 2010-04-07 04:46:28 UTC
Integrated into 'main-golden', will be available in build *201004070201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/514c7d1a4263
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #182698: Don't create instances of JSeparator outside of AWT thread


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo