Re: [cp-patches] FYI: Fix for BoxLayout

[Roman Kennke]

Hi,

Tom Tromey wrote:

> Roman> +  public static Component[] getVisibleChildren(Container c)
> Roman> +  {
> Roman> +    Component[] children = c.getComponents();
> Roman> +    Vector visible = new Vector();
> Roman> +    for (int i = 0; i < children.length; i++)
> Roman> +      if (children[i].isVisible())
> Roman> +        visible.add(children[i]);
> Roman> +    return (Component[]) visible.toArray(new Container[visible.size()]);
>
> This means multiple allocations per layout.
> I wonder whether that is noticeable in a profile.
>  

I have optimized this. Now no new array is allocated at all. A List 
implementation is returned instead that iterates over the components and 
returns only the visible ones. The List itself is cached in a 
WeakHashMap, so multiple layouts for one Container will reuse the same 
List impl, still if the Component (more precisely: its children array) 
is no longer in use, it can be garbage collected. We still have one 
allocation though: the Iterator. But this is minimal and I regard the 
overhead of caching this as not worth the effort.

2005-06-29 Roman Kennke <address@hidden>

* gnu/java/awt/AWTUtilities.java
(VisibleComponentList): Added List implementation that iterates over
the child components of a Container and only returns Components
that are actually visible.
(getVisibleChildren): Now returns a List instead of an array. This
list is cached. This greatly decreases allocations in
LayoutManagers.
* javax/swing/BoxLayout.java:
Updated to use the new AWTUtilities.getVisibleChildren() method.

/Roman

Index: javax/swing/BoxLayout.java
===================================================================
RCS file: /cvsroot/classpath/classpath/javax/swing/BoxLayout.java,v
retrieving revision 1.13
diff -u -r1.13 BoxLayout.java
--- javax/swing/BoxLayout.java  27 Jun 2005 14:41:10 -0000      1.13
+++ javax/swing/BoxLayout.java  29 Jun 2005 09:24:30 -0000
@@ -47,6 +47,7 @@
 import java.io.Serializable;
 import java.util.Collection;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Vector;
 
 import gnu.java.awt.AWTUtilities;
@@ -404,16 +405,16 @@
     int x = 0;
     int y = 0;
 
-    Component[] children = AWTUtilities.getVisibleChildren(parent);
+    List children = AWTUtilities.getVisibleChildren(parent);
 
     if (isHorizontalIn(parent))
       {        
         x = insets.left + insets.right;
         // sum up preferred widths of components, find maximum of preferred
         // heights
-        for (int index = 0; index < children.length; index++)
+        for (Iterator i = children.iterator(); i.hasNext();)
           {
-            Component comp = children[index];
+            Component comp = (Component) i.next();
             Dimension sz = comp.getPreferredSize();
             x += sz.width;
             y = Math.max(y, sz.height);
@@ -425,9 +426,9 @@
         y = insets.top + insets.bottom;
         // sum up preferred heights of components, find maximum of
         //  preferred widths
-        for (int index = 0; index < children.length; index++)
+        for (Iterator i = children.iterator(); i.hasNext();)
           {
-            Component comp = children[index];
+            Component comp = (Component) i.next();
             Dimension sz = comp.getPreferredSize();
             y += sz.height;
             x = Math.max(x, sz.width);
@@ -454,15 +455,15 @@
     int x = insets.left + insets.right;
     int y = insets.bottom + insets.top;
 
-    Component[] children = AWTUtilities.getVisibleChildren(parent);
+    List children = AWTUtilities.getVisibleChildren(parent);
 
     if (isHorizontalIn(parent))
       {
         // sum up preferred widths of components, find maximum of preferred
         // heights
-        for (int index = 0; index < children.length; index++)
+        for (Iterator i = children.iterator(); i.hasNext();)
           {
-            Component comp = children[index];
+           Component comp = (Component) i.next();
             Dimension sz = comp.getMinimumSize();
             x += sz.width;
             y = Math.max(y, sz.height);
@@ -472,9 +473,9 @@
       {
         // sum up preferred heights of components, find maximum of
         //  preferred widths
-        for (int index = 0; index < children.length; index++)
+        for (Iterator i = children.iterator(); i.hasNext();)
           {
-            Component comp = children[index];
+           Component comp = (Component) i.next();
             Dimension sz = comp.getMinimumSize();
             y += sz.height;
             x = Math.max(x, sz.width);
@@ -565,16 +566,16 @@
     int x = insets.left + insets.right;
     int y = insets.top + insets.bottom;
 
-    Component[] children = AWTUtilities.getVisibleChildren(parent);
+    List children = AWTUtilities.getVisibleChildren(parent);
 
     if (isHorizontalIn(parent))
       {
         
         // sum up preferred widths of components, find maximum of preferred
         // heights
-        for (int index = 0; index < children.length; index++)
+        for (Iterator i = children.iterator(); i.hasNext();)
           {
-            Component comp = children[index];
+            Component comp = (Component) i.next();
             Dimension sz = comp.getMaximumSize();
             x += sz.width;
             // Check for overflow.
@@ -587,9 +588,9 @@
       {
         // sum up preferred heights of components, find maximum of
         //  preferred widths
-        for (int index = 0; index < children.length; index++)
+        for (Iterator i = children.iterator(); i.hasNext();)
           {
-            Component comp = children[index];
+            Component comp = (Component) i.next();
             Dimension sz = comp.getMaximumSize();
             y += sz.height;
             // Check for overflow
@@ -624,12 +625,12 @@
     // Set all components to their preferredSizes and sum up the allocated
     // space. Create SizeReqs for each component and store them in
     // sizeReqs. Find the maximum size in the crossing direction.
-    Component[] children = AWTUtilities.getVisibleChildren(parent);
+    List children = AWTUtilities.getVisibleChildren(parent);
     Vector sizeReqs = new Vector();
     int allocated = 0;
-    for (int i = 0; i < children.length; i++)
+    for (Iterator i = children.iterator(); i.hasNext();)
       {
-       Component c = children[i];
+       Component c = (Component) i.next();
        SizeReq sizeReq = new SizeReq(c, layoutDir);
        int preferred = layoutDir.size(c.getPreferredSize());
        sizeReq.size = preferred;
Index: gnu/java/awt/AWTUtilities.java
===================================================================
RCS file: /cvsroot/classpath/classpath/gnu/java/awt/AWTUtilities.java,v
retrieving revision 1.1
diff -u -r1.1 AWTUtilities.java
--- gnu/java/awt/AWTUtilities.java      24 Jun 2005 14:39:49 -0000      1.1
+++ gnu/java/awt/AWTUtilities.java      29 Jun 2005 09:24:30 -0000
@@ -39,7 +39,11 @@
 
 import java.awt.Component;
 import java.awt.Container;
-import java.util.Vector;
+import java.util.AbstractSequentialList;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.NoSuchElementException;
+import java.util.WeakHashMap;
 
 /**
  * This class provides utility methods that are commonly used in AWT
@@ -49,6 +53,248 @@
 {
 
   /**
+   * This List implementation wraps the Component[] returned by
+   * address@hidden Container#getComponents()} and iterates over the visible 
Components
+   * in that array. This class is used in address@hidden #getVisibleChildren}.
+   */
+  static class VisibleComponentList extends AbstractSequentialList
+  {
+    /**
+     * The ListIterator for this List.
+     */
+    class VisibleComponentIterator implements ListIterator
+    {
+      /** The current index in the Component[]. */
+      int index;
+
+      /** The index in the List of visible Components. */
+      int listIndex;
+
+      /**
+       * Creates a new VisibleComponentIterator that starts at the specified
+       * <code>listIndex</code>. The array of Components is searched from
+       * the beginning to find the matching array index.
+       *
+       * @param listIndex the index from where to begin iterating
+       */
+      VisibleComponentIterator(int listIndex)
+      {
+       this.listIndex = listIndex;
+       int visibleComponentsFound = 0;
+       for (index = 0; visibleComponentsFound != listIndex; index++)
+         {
+           if (components[index].isVisible())
+             visibleComponentsFound++;
+         }
+      }
+
+      /**
+       * Returns <code>true</code> if there are more visible components in the
+       * array, <code>false</code> otherwise.
+       *
+       * @return <code>true</code> if there are more visible components in the
+       *     array, <code>false</code> otherwise
+       */
+      public boolean hasNext()
+      {
+       boolean hasNext = false;
+       for (int i = index; i < components.length; i++)
+         {
+           if (components[i].isVisible())
+             {
+               hasNext = true;
+               break;
+             }
+         }
+       return hasNext;
+      }
+
+      /**
+       * Returns the next visible <code>Component</code> in the List.
+       *
+       * @return the next visible <code>Component</code> in the List
+       *
+       * @throws if there is no next element
+       */
+      public Object next()
+      {
+       Object o = null;
+       for (; index < components.length; index++)
+         {
+           if (components[index].isVisible())
+             {
+               o = components[index];
+               break;
+             }
+         }
+       if (o != null)
+         {
+           index++;
+           listIndex++;
+           return o;
+         }
+       else
+         throw new NoSuchElementException();
+      }
+
+      /**
+       * Returns <code>true</code> if there are more visible components in the
+       * array in the reverse direction, <code>false</code> otherwise.
+       *
+       * @return <code>true</code> if there are more visible components in the
+       *     array in the reverse direction, <code>false</code> otherwise
+       */
+      public boolean hasPrevious()
+      {
+       boolean hasPrevious = false;
+       for (int i = index - 1; i >= 0; i--)
+         {
+           if (components[i].isVisible())
+             {
+               hasPrevious = true;
+               break;
+             }
+         }
+       return hasPrevious;
+      }
+
+      /**
+       * Returns the previous visible <code>Component</code> in the List.
+       *
+       * @return the previous visible <code>Component</code> in the List
+       *
+       * @throws NoSuchElementException if there is no previous element
+       */
+      public Object previous()
+      {
+       Object o = null;
+       for (index--; index >= 0; index--)
+         {
+           if (components[index].isVisible())
+             {
+               o = components[index];
+               break;
+             }
+         }
+       if (o != null)
+         {
+           listIndex--;
+           return o;
+         }
+       else
+         throw new NoSuchElementException();
+      }
+
+      /**
+       * Returns the index of the next element in the List.
+       *
+       * @return the index of the next element in the List
+       */
+      public int nextIndex()
+      {
+       return listIndex + 1;
+      }
+
+      /**
+       * Returns the index of the previous element in the List.
+       *
+       * @return the index of the previous element in the List
+       */
+      public int previousIndex()
+      {
+       return listIndex - 1;
+      }
+
+      /**
+       * This operation is not supported because the List is immutable.
+       *
+       * @throws UnsupportedOperationException because the List is immutable
+       */
+      public void remove()
+      {
+       throw new UnsupportedOperationException
+         ("VisibleComponentList is immutable");
+      }
+
+      /**
+       * This operation is not supported because the List is immutable.
+       *
+       * @param o not used here
+       *
+       * @throws UnsupportedOperationException because the List is immutable
+       */
+      public void set(Object o)
+      {
+       throw new UnsupportedOperationException
+         ("VisibleComponentList is immutable");
+      }
+
+      /**
+       * This operation is not supported because the List is immutable.
+       *
+       * @param o not used here
+       *
+       * @throws UnsupportedOperationException because the List is immutable
+       */
+      public void add(Object o)
+      {
+       throw new UnsupportedOperationException
+         ("VisibleComponentList is immutable");
+      }
+    }
+
+    /**
+     * The components over which we iterate. Only the visible components
+     * are returned by this List.
+     */
+    Component[] components;
+
+    /**
+     * Creates a new instance of VisibleComponentList that wraps the specified
+     * <code>Component[]</code>.
+     *
+     * @param c the <code>Component[]</code> to be wrapped.
+     */
+    VisibleComponentList(Component[] c)
+    {
+      components = c;
+    }
+
+    /**
+     * Returns a address@hidden ListIterator} for iterating over this List.
+     *
+     * @return a address@hidden ListIterator} for iterating over this List
+     */
+    public ListIterator listIterator(int index)
+    {
+      return new VisibleComponentIterator(index);
+    }
+
+    /**
+     * Returns the number of visible components in the wrapped Component[].
+     *
+     * @return the number of visible components
+     */
+    public int size()
+    {
+      int visibleComponents = 0;
+      for (int i = 0; i < components.length; i++)
+       if (components[i].isVisible())
+         visibleComponents++;
+      return visibleComponents;
+    }
+  }
+
+  /**
+   * The cache for our List instances. We try to hold one instance of
+   * VisibleComponentList for each Component[] that is requested. Note
+   * that we use a WeakHashMap for caching, so that the cache itself
+   * does not keep the array or the List from beeing garbage collected
+   * if no other objects hold references to it.
+   */
+  static WeakHashMap visibleChildrenCache = new WeakHashMap();
+
+  /**
    * Returns the visible children of a address@hidden Container}. This method 
is
    * commonly needed in LayoutManagers, because they only have to layout
    * the visible children of a Container.
@@ -57,13 +303,19 @@
    *
    * @return the visible children of <code>c</code>
    */
-  public static Component[] getVisibleChildren(Container c)
+  public static List getVisibleChildren(Container c)
   {
     Component[] children = c.getComponents();
-    Vector visible = new Vector();
-    for (int i = 0; i < children.length; i++)
-      if (children[i].isVisible())
-        visible.add(children[i]);
-    return (Component[]) visible.toArray(new Container[visible.size()]);
+    Object o = visibleChildrenCache.get(children);
+    VisibleComponentList visibleChildren = null;
+    if (o == null)
+      {
+       visibleChildren = new VisibleComponentList(children);
+       visibleChildrenCache.put(children, visibleChildren);
+      }
+    else
+      visibleChildren = (VisibleComponentList) o;
+
+    return visibleChildren;
   }
 }