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.

Bug 174339 - TreeView.expandAll sould be fast even for large trees
Summary: TreeView.expandAll sould be fast even for large trees
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Explorer (show other bugs)
Version: 6.x
Hardware: All All
: P2 blocker (vote)
Assignee: Jan Peska
URL:
Keywords: PERFORMANCE
Depends on:
Blocks:
 
Reported: 2009-10-12 10:42 UTC by Marek Fukala
Modified: 2015-11-21 03:05 UTC (History)
5 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marek Fukala 2009-10-12 10:42:51 UTC
1) open the php file from issue #173003.
=> the navigator initialization runs in AWT for at least 20 seconds.

"AWT-EventQueue-1" prio=6 tid=0x0000000101918000 nid=0x14bfdb000 runnable [0x000000014bfd9000..0x000000014bfdaad0]
   java.lang.Thread.State: RUNNABLE
	at java.util.Vector.indexOf(Vector.java:361)
	- locked <0x000000010a172a78> (a java.util.Vector)
	at java.util.Vector.indexOf(Vector.java:335)
	at javax.swing.tree.DefaultMutableTreeNode.getIndex(DefaultMutableTreeNode.java:267)
	at javax.swing.tree.FixedHeightLayoutCache$FHTreeStateNode.getTotalChildCount(FixedHeightLayoutCache.java:878)
	at javax.swing.tree.FixedHeightLayoutCache$FHTreeStateNode.getTotalChildCount(FixedHeightLayoutCache.java:893)
	at javax.swing.tree.FixedHeightLayoutCache$FHTreeStateNode.getRowToModelIndex(FixedHeightLayoutCache.java:865)
	at javax.swing.tree.FixedHeightLayoutCache$FHTreeStateNode.createChildFor(FixedHeightLayoutCache.java:965)
	at javax.swing.tree.FixedHeightLayoutCache.getNodeForPath(FixedHeightLayoutCache.java:701)
	at javax.swing.tree.FixedHeightLayoutCache.ensurePathIsExpanded(FixedHeightLayoutCache.java:639)
	at javax.swing.tree.FixedHeightLayoutCache.setExpandedState(FixedHeightLayoutCache.java:282)
	at javax.swing.plaf.basic.BasicTreeUI.updateExpandedDescendants(BasicTreeUI.java:1648)
	at javax.swing.plaf.basic.BasicTreeUI$Handler.treeExpanded(BasicTreeUI.java:3721)
	at javax.swing.JTree.fireTreeExpanded(JTree.java:2659)
	at javax.swing.JTree.setExpandedState(JTree.java:3430)
	at javax.swing.JTree.expandPath(JTree.java:2166)
	at javax.swing.JTree.expandRow(JTree.java:2181)
	at org.openide.explorer.view.TreeView.expandAll(TreeView.java:594)
	at org.netbeans.modules.csl.navigation.ClassMemberPanelUI$4.run(ClassMemberPanelUI.java:240)
	at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
	at org.netbeans.core.TimableEventQueue.dispatchEvent(TimableEventQueue.java:117)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:300)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:210)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:200)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:195)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:187)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:121)
Comment 1 Marek Fukala 2009-10-12 11:09:56 UTC
ClassMembersPanelUI from CSL has such code on line 229:

                    Language language = LanguageRegistry.getInstance().getLanguageByMimeType(fileObject.getMIMEType());
                    if (language != null && language.getStructure() != null) {
                        StructureScanner scanner = language.getStructure();
                        Configuration configuration = scanner.getConfiguration();
                        if (configuration != null) {
                            expand = configuration.getExpandDepth() != 0;
                        }
                    }
                    if (expand) {
                        boolean scrollOnExpand = elementView.getScrollOnExpand();
                        elementView.setScrollOnExpand( false );
                        elementView.expandAll();
                        elementView.setScrollOnExpand( scrollOnExpand );
                    }

for PHP language the expand property becomes true and hence the TreeView.expandAll() is called. This expands all the
existing tree paths which is crazy.

I do not even understand the expand = configuration.getExpandDepth() != 0. I would expect the 
configuration.getExpandDepth() nodes levels expanded, not all if != 0.

Assigning back to PHP so they may update the property, but the real fix needs to be done in CSL.
Comment 2 rmatous 2009-10-14 08:54:51 UTC
There is already reported issue that there is no way how to control expandig/collapsing properly. expand depth simply
doesn't work as far as I can remember. Anyway expand depth isn't enough for us, better would be to have possibility to
say for every individual kind wheteher should be expanded or not  But the paragraph above  is more or less RFE and only
indirectly connected to this DEFECT. 

=> CSL
Comment 3 David Strupl 2009-10-14 13:34:25 UTC
IMHO there is no reason why the expandAll() call should take that long for any size of the tree. It should be
proportional to the size of the tree that is actually *visible* and not proportional to the size of the whole tree. The
only problem is how to get the overall size of the tree. Anyway this is a problem of TreeView --> assigning to core team.
Comment 4 t_h 2009-10-19 10:53:41 UTC
expandAll() of example php file took 140s for me. Now it takes 2s. core-main #363de11e2c61
Comment 5 David Strupl 2009-10-19 13:31:07 UTC
Tomas, you are a genius. I think this will improve the situation in #167708 as well.
Comment 6 Alexander Simon 2009-10-20 16:45:59 UTC
It seems you broke treeview functionality.
Please revert your changes.
Changes result in a lot of NPE:
java.lang.NullPointerException
        at javax.swing.plaf.basic.BasicTreeUI.paint(BasicTreeUI.java:1155)
        at javax.swing.plaf.metal.MetalTreeUI.paint(MetalTreeUI.java:152)
        at javax.swing.plaf.ComponentUI.update(ComponentUI.java:142)
        at javax.swing.JComponent.paintComponent(JComponent.java:743)
        at javax.swing.JComponent.paint(JComponent.java:1006)
        at org.openide.explorer.view.TreeView$ExplorerTree.access$1401(TreeView.java:1740)
        at org.openide.explorer.view.TreeView$ExplorerTree.guardedPaint(TreeView.java:1862)
        at org.openide.explorer.view.TreeView$ExplorerTree.access$1800(TreeView.java:1740)
        at org.openide.explorer.view.TreeView$ExplorerTree$GuardedActions.run(TreeView.java:2132)
It seems problem in code:
       TreeUI treeUI = tree.getUI();
        tree.setUI(null);
        TreeNode root = (TreeNode) tree.getModel().getRoot();
        expandOrCollapseAll(new TreePath(root), true);
        tree.setUI(treeUI);
        tree.updateUI();
    }
UI must be not null!
Comment 7 Alexander Simon 2009-10-20 16:51:29 UTC
NPE
Comment 8 David Strupl 2009-10-20 16:53:34 UTC
Alex, is the stack trace you pasted complete? I miss what thread was that exception thrown from ...

Also knowing the OS/JDK you get the exception would be nice to have.

Tomas is correctly putting non-null value after the expansion - it is either 2 threads accessing it or Tomas needs to
wrap that into try { set it to null } finally { set to non-null }. Was there some exception prior to the NPEs?
Comment 9 Alexander Simon 2009-10-20 17:04:18 UTC
Yes, two threads.
INFO [org.openide.explorer.view.TreeView]: Problems while painting
java.lang.NullPointerException
        at javax.swing.plaf.basic.BasicTreeUI.paint(BasicTreeUI.java:1160)
        at javax.swing.plaf.metal.MetalTreeUI.paint(MetalTreeUI.java:152)
        at javax.swing.plaf.ComponentUI.update(ComponentUI.java:142)
        at javax.swing.JComponent.paintComponent(JComponent.java:743)
        at javax.swing.JComponent.paint(JComponent.java:1006)
        at org.openide.explorer.view.TreeView$ExplorerTree.access$1401(TreeView.java:1740)
        at org.openide.explorer.view.TreeView$ExplorerTree.guardedPaint(TreeView.java:1862)
        at org.openide.explorer.view.TreeView$ExplorerTree.access$1800(TreeView.java:1740)
        at org.openide.explorer.view.TreeView$ExplorerTree$GuardedActions.run(TreeView.java:2132)
        at org.openide.util.Mutex.readAccess(Mutex.java:327)
        at org.openide.util.Mutex$1R.run(Mutex.java:1299)
        at org.openide.nodes.Children$ProjectManagerDeadlockDetector.execute(Children.java:1811)
        at org.openide.util.Mutex.doWrapperAccess(Mutex.java:1320)
        at org.openide.util.Mutex.readAccess(Mutex.java:275)
        at org.openide.explorer.view.TreeView$ExplorerTree$GuardedActions.<init>(TreeView.java:2125)
        at org.openide.explorer.view.TreeView$ExplorerTree.paint(TreeView.java:1835)
        at javax.swing.JComponent.paintWithOffscreenBuffer(JComponent.java:4972)
        at javax.swing.JComponent.paintDoubleBuffered(JComponent.java:4925)
        at javax.swing.JComponent._paintImmediately(JComponent.java:4868)
        at javax.swing.JComponent.paintImmediately(JComponent.java:4675)
        at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:451)
        at javax.swing.SystemEventQueueUtilities$ComponentWorkRequest.run(SystemEventQueueUtilities.java:114)
        at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
        at java.awt.EventQueue.dispatchEvent(EventQueue.java:461)
        at org.netbeans.core.TimableEventQueue.dispatchEvent(TimableEventQueue.java:117)
        at java.awt.EventDispatchThread.pumpOneEventForHierarchy(EventDispatchThread.java:242)
        at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:163)
        at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:157)
        at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:149)
[catch] at java.awt.EventDispatchThread.run(EventDispatchThread.java:110)
Comment 10 Alexander Simon 2009-10-20 17:06:03 UTC
reproduced on any jdk
Comment 11 t_h 2009-10-20 17:34:01 UTC
TreeView.exapandAll() should not be called out of AWT. I will add try {} finally {} as David suggested. If there is
problem with two threads, caller's side should be fixed.
Comment 12 Maria Tishkova 2009-10-20 17:37:27 UTC
Tomas, there is no javadoc to understand it should be called from the UI thread.
And it used to work before your last changes.
Should not you either check if it is invoked in AWT thread or add javadoc?
Comment 13 t_h 2009-10-20 17:53:00 UTC
It is swing component so all operations (except for those that explicitly states that can be called from other threads
than EDT) are supposed to be done in EDT. I guess it worked in most cases but it was not correct anyway. David's
suggestions in core-main #7030c4cf5c0f. 
Comment 15 Quality Engineering 2009-10-22 23:33:53 UTC
Integrated into 'main-golden', will be available in build *200910221401* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/7030c4cf5c0f
User: Tomas Holy <t_h@netbeans.org>
Log: #174339: adding try/finally to make temporary replacement more robust
Comment 16 bugmenot1 2015-10-16 22:15:41 UTC
I am reopening this bug to report that you've performed an incorrect "fix". Here is the code you use:

    public void expandAll() {
        TreeUI treeUI = tree.getUI();
        try {
            tree.setUI(null);
            TreeNode root = (TreeNode) tree.getModel().getRoot();
            expandOrCollapseAll(new TreePath(root), true);
        } finally {
            tree.setUI(treeUI);
            tree.updateUI();
        }
    }

Yes, the reason that expanding all nodes of a JTree is slow is because the JTree GUI will paint() the updates once for EVERY row it expands. So for thousands of expanded rows it is super slow.

But you CANNOT do this "solution". If you setUI(null), you will remove the TreeUI's CRITICAL listeners from the JTree, which means the UI is not aware of rows being expanded or collapsed anymore. So your UI "backup" is out-of-date. Then when you setUI(theOldUi), it will LOOK OK, until you try to add some more rows, then it will cause null pointer exceptions because the GUI internal cached data is out of sync with what happened in the tree while the GUI was disabled.

What you MUST do:

myTree.setUI(null);
massExpand(myTreeRoot);
myTree.setUI(new BasicTreeUI()); // or whatever GUI you used before. but INSTEAD of setUI you can just use "myTree.updateUI()" to instantly set it to the normal default for the current look&feel, which is always what you want UNLESS you've made your own custom UI!

Also remember that the setUI() will reset any custom things like custom row indentations, setShowGrid, etc, so you will have to re-apply those too.

These facts are the reason that the user said "It seems you broke treeview functionality. Please revert your changes. Changes result in a lot of NPE". It's also the reason why the "fixed fix" was to add updateUI(). But as you can see, it's not a proper fix. Here's the PROPER proper fix. ;-)

    public void expandAll() {
        try {
            tree.setUI(null);
            TreeNode root = (TreeNode) tree.getModel().getRoot();
            expandOrCollapseAll(new TreePath(root), true);
        } finally {
            tree.updateUI();
        }
    }
Comment 17 bugmenot1 2015-10-16 22:17:59 UTC
Oh and you do not need try{} finally{} unless an exception can be thrown by either of these:

            TreeNode root = (TreeNode) tree.getModel().getRoot();
            expandOrCollapseAll(new TreePath(root), true);

So you may be able to remove the try-block too. I don't use any in my own program. Just setUI(null); expandAll(); updateUI(); and nothing else.
Comment 18 Ondrej Vrabec 2015-11-20 13:50:45 UTC
> But you CANNOT do this "solution". If you setUI(null), you will remove the TreeUI's CRITICAL listeners from the JTree, which means the UI is not aware of rows being expanded or collapsed anymore. So your UI "backup" is > out-of-date. Then when you setUI(theOldUi), it will LOOK OK, until you try to add some more rows, then it will cause null pointer exceptions because the GUI internal cached data is out of sync with what happened in the tree while the GUI was disabled.
I do not fully understand. True, the old UI is set back but immediately followed by updateUI which then resets the UI to the default, new instance right? So AFAIK the old out-of-date UI should not be there anymore.
But you are right that setUI(old) is probably not needed when followed by updateUI, i am removing that...

fix: http://hg.netbeans.org/core-main/rev/f20f4a83318d
Comment 19 Quality Engineering 2015-11-21 03:05:18 UTC
Integrated into 'main-silver', will be available in build *201511210002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/f20f4a83318d
User: Ondrej Vrabec <ovrabec@netbeans.org>
Log: #174339 - TreeView.expandAll sould be fast even for large trees
Do not set the previous UI back, it may be out of date.