dev@javaserverfaces.java.net

Re: Review: Initial Fixes for Issue 125

From: Jacob Hookom <jacob_at_hookom.net>
Date: Sun, 03 Jul 2005 03:03:33 -0500

Pending getFacetCount() from the EG, 'test.appserver' runs successfully
against Glassfish.

Attached is the diff used for tests.

-- Jacob

Ed Burns wrote:

>>>>>>On Sat, 02 Jul 2005 02:50:11 -0500, Jacob Hookom <jacob_at_hookom.net> said:
>>>>>>
>>>>>>
>
>JH> https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=125
>JH> ============
>
>JH> In order to optimize processing on leaf nodes, we need to predicate
>JH> calls to:
>
>JH> - UIComponent.getChildren()
>JH> - UIComponent.getFacets()
>
>JH> Lots of wasteful object instantiation was occuring within
>JH> UIComponentBase's logic. JSF Impl
>JH> RenderKits and other objects should also be reviewed once this
>JH> changebundle is applied.
>
>
>JH> 1. The method getFacetCount() was added to UIComponent and UIComponentBase
>
>JH> 2. Reworked UIComponentBase and Renderer to first check for the
>JH> existence of children/facets
>JH> before calling getXXXX(), removing the possibility of lazy
>JH> instantiating these items
>
>JH> 3. StateSaving was reworked to simplify/optimize the tree walk and
>JH> Facet storage
>
>JH> 4. The method getFacetsAndChildIterator() was redone
>
>JH> The test target needs to be run against Glassfish yet. Demo
>JH> applications function fine
>JH> with these modifications. Tested by adding a UIComponent to the tree
>JH> which throws an
>JH> exception if getChildren or getFacets is ever invoked on it.
>
>I'd like to get this in but I understand the problem with testing
>against glassfish. Please hold until we can make sure the tests still
>work.
>
>JH> + // For state saving
>JH> + private final static Object[] EMPTY_ARRAY = new Object[0];
>JH> +
>JH> + // Empty iterator for short circuiting operations
>JH> + private final static Iterator EMPTY_ITERATOR = new Iterator() {
>JH> +
>JH> + public void remove() {
>JH> + throw new UnsupportedOperationException();
>JH> + }
>JH> +
>JH> + public Object next() {
>JH> + throw new NoSuchElementException("Empty Iterator");
>JH> + }
>JH> +
>JH> + public boolean hasNext() {
>JH> + return false;
>JH> + }
>JH> + };
>
>No need for this. Use java.util.Collections.EMPTY_LIST.iterator().
>
>Otherwise, I concurr withd Adam's comments. Once you get my +1 on the
>EG for getFacetCount(), you have r=edburns for going through the checkin
>process (doing the tests and all).
>
>Ed
>
>
>


-- 
Jacob Hookom - Minneapolis
--------------------------
http://hookom.blogspot.com


? .classpath
? .project
? diff.out
Index: src/javax/faces/component/UIComponent.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-api/src/javax/faces/component/UIComponent.java,v
retrieving revision 1.130
diff -u -r1.130 UIComponent.java
--- src/javax/faces/component/UIComponent.java 5 May 2005 20:51:03 -0000 1.130
+++ src/javax/faces/component/UIComponent.java 3 Jul 2005 07:43:44 -0000
@@ -488,6 +488,14 @@
      * </ul>
      */
     public abstract Map getFacets();
+
+ /**
+ * <p>Return the number of facet {_at_link UIComponent}s that are
+ * associated with this {_at_link UIComponent}. If there are no
+ * facets, this method must return 0. The method must not cause
+ * the creation of a facet component map.</p>
+ */
+ public abstract int getFacetCount();
 
 
     /**
Index: src/javax/faces/component/UIComponentBase.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-api/src/javax/faces/component/UIComponentBase.java,v
retrieving revision 1.110
diff -u -r1.110 UIComponentBase.java
--- src/javax/faces/component/UIComponentBase.java 19 May 2005 19:14:17 -0000 1.110
+++ src/javax/faces/component/UIComponentBase.java 3 Jul 2005 07:43:46 -0000
@@ -530,12 +530,15 @@
         if (parent == null) {
             return;
         }
- List children = parent.getChildren();
- int index = children.indexOf(component);
- if (index >= 0) {
- children.remove(index);
- return;
- } else {
+ if (parent.getChildCount() > 0) {
+ List children = parent.getChildren();
+ int index = children.indexOf(component);
+ if (index >= 0) {
+ children.remove(index);
+ return;
+ }
+ }
+ if (parent.getFacetCount() > 0) {
             Map facets = parent.getFacets();
             Iterator entries = facets.entrySet().iterator();
             while (entries.hasNext()) {
@@ -705,6 +708,17 @@
         return (facets);
 
     }
+
+ // Do not allocate the children List to answer this question
+ public int getFacetCount() {
+
+ if (facets != null) {
+ return (facets.size());
+ } else {
+ return (0);
+ }
+
+ }
 
 
     // Do not allocate the facets Map to answer this question
@@ -720,22 +734,11 @@
 
 
     public Iterator getFacetsAndChildren() {
-
- List combined = new ArrayList();
- if (this.facets != null) {
- Iterator facets = getFacets().values().iterator();
- while (facets.hasNext()) {
- combined.add(facets.next());
- }
- }
- if (this.children != null) {
- Iterator kids = getChildren().iterator();
- while (kids.hasNext()) {
- combined.add(kids.next());
- }
+
+ if (this.getChildCount() == 0 && this.getFacetCount() == 0) {
+ return EMPTY_ITERATOR;
         }
- return (new FacetsAndChildrenIterator(combined));
-
+ return new FacetsAndChildrenIterator(this);
     }
 
 
@@ -851,11 +854,11 @@
         if (getRendersChildren()) {
             encodeChildren(context);
         }
- else {
+ else if (this.getChildCount() > 0) {
             Iterator kids = getChildren().iterator();
             while (kids.hasNext()) {
- UIComponent kid = (UIComponent) kids.next();
- kid.encodeAll(context);
+ UIComponent kid = (UIComponent) kids.next();
+ kid.encodeAll(context);
             }
         }
         
@@ -1091,77 +1094,59 @@
             return null;
         }
         Object [] stateStruct = new Object[2];
- Object [] childState = null;
+ Object [] childState = EMPTY_ARRAY;
         
         // Process this component itself
         stateStruct[MY_STATE] = saveState(context);
         
- // Process all the children of this component
- int i = 0, len = getNonTransientChildLength() +
- getNonTransientFacetLength();
-
- childState = new Object[len];
- stateStruct[CHILD_STATE] = childState;
- Iterator kids = getChildren().iterator();
- while (kids.hasNext()) {
- UIComponent kid = (UIComponent) kids.next();
- if (!kid.isTransient()) {
- childState[i++] = kid.processSaveState(context);
+ // determine if we have any children to store
+ int count = this.getChildCount() + this.getFacetCount();
+ if (count > 0) {
+
+ // this arraylist will store state
+ List stateList = new ArrayList(count);
+
+ // if we have children, add them to the stateList
+ if (this.getChildCount() > 0) {
+ Iterator kids = getChildren().iterator();
+ UIComponent kid;
+ while (kids.hasNext()) {
+ kid = (UIComponent) kids.next();
+ if (!kid.isTransient()) {
+ stateList.add(kid.processSaveState(context));
+ }
+ }
             }
- }
-
- Iterator myFacets = getFacets().keySet().iterator();
- String facetName = null;
- UIComponent facet = null;
- Object facetState = null;
- Object[][] facetSaveState = null;
- while (myFacets.hasNext()) {
- facetName = (String) myFacets.next();
- facet = (UIComponent) getFacets().get(facetName);
- if (!facet.isTransient()) {
- facetState = facet.processSaveState(context);
- facetSaveState = new Object[1][2];
- facetSaveState[0][0] = facetName;
- facetSaveState[0][1] = facetState;
- childState[i] = facetSaveState;
- i++;
+
+ // if we have facets, add them to the stateList
+ if (this.getFacetCount() > 0) {
+ Iterator myFacets = getFacets().entrySet().iterator();
+ UIComponent facet = null;
+ Object facetState = null;
+ Object[] facetSaveState = null;
+ Map.Entry entry = null;
+ while (myFacets.hasNext()) {
+ entry = (Map.Entry) myFacets.next();
+ facet = (UIComponent) entry.getValue();
+ if (!facet.isTransient()) {
+ facetState = facet.processSaveState(context);
+ facetSaveState = new Object[2];
+ facetSaveState[0] = entry.getKey();
+ facetSaveState[1] = facetState;
+ stateList.add(facetSaveState);
+ }
+ }
             }
+
+ // finally, capture the stateList and replace the original,
+ // empty Object array
+ childState = stateList.toArray();
         }
+
+ stateStruct[CHILD_STATE] = childState;
         return stateStruct;
     }
     
-
- /**
- * @return the number of non-transient children in our child list.
- */
-
- private int getNonTransientChildLength() {
- int i, result = 0;
- Iterator kids = getChildren().iterator();
- UIComponent kid = null;
- while (kids.hasNext()) {
- kid = (UIComponent) kids.next();
- i = kid.isTransient() ? result : result++;
- }
- return result;
- }
-
- /**
- * @return the number of non-transient facet children in our facet
- * children list.
- */
-
- private int getNonTransientFacetLength() {
- int i, result = 0;
- Iterator kids = getFacets().keySet().iterator();
- UIComponent kid = null;
- while (kids.hasNext()) {
- kid = (UIComponent) getFacets().get(kids.next().toString());
- i = kid.isTransient() ? result : result++;
- }
- return result;
- }
-
     /**
      * @exception NullPointerException {_at_inheritDoc}
      */
@@ -1175,35 +1160,40 @@
         Object [] childState = (Object []) stateStruct[CHILD_STATE];
         
         // Process this component itself
- restoreState(context, stateStruct[MY_STATE]);
+ restoreState(context, stateStruct[MY_STATE]);
         
         int i = 0;
         
         // Process all the children of this component
- Iterator kids = getChildren().iterator();
- while (kids.hasNext()) {
- UIComponent kid = (UIComponent) kids.next();
- Object currentState = childState[i++];
- if (currentState == null) {
- continue;
- }
- kid.processRestoreState(context, currentState);
+ if (this.getChildCount() > 0) {
+ Iterator kids = getChildren().iterator();
+ while (kids.hasNext()) {
+ UIComponent kid = (UIComponent) kids.next();
+ Object currentState = childState[i++];
+ if (currentState == null) {
+ continue;
+ }
+ kid.processRestoreState(context, currentState);
+ }
         }
         
- int facetsSize = getFacets().size();
- int j = 0;
- Object[][] facetSaveState = null;
- String facetName = null;
- UIComponent facet = null;
- Object facetState = null;
- while (j < facetsSize) {
- if (null != (facetSaveState = (Object[][])childState[i++])) {
- facetName = (String) facetSaveState[0][0];
- facetState = facetSaveState[0][1];
- facet = (UIComponent) getFacets().get(facetName);
- facet.processRestoreState(context, facetState);
+ // process all of the facets of this component
+ if (this.getFacetCount() > 0) {
+ int facetsSize = getFacets().size();
+ int j = 0;
+ Object[] facetSaveState = null;
+ String facetName = null;
+ UIComponent facet = null;
+ Object facetState = null;
+ while (j < facetsSize) {
+ if (null != (facetSaveState = (Object[])childState[i++])) {
+ facetName = (String) facetSaveState[0];
+ facetState = facetSaveState[1];
+ facet = (UIComponent) getFacets().get(facetName);
+ facet.processRestoreState(context, facetState);
+ }
+ ++j;
             }
- ++j;
         }
     }
     
@@ -1268,11 +1258,11 @@
             HashMap attributesCopy = new HashMap(attributes);
             values[0] = attributesCopy;
         }
- values[1] = saveBindingsState(context);
+ values[1] = saveBindingsState(context);
         values[2] = clientId;
         values[3] = id;
         values[4] = rendered ? Boolean.TRUE : Boolean.FALSE;
- values[5] = renderedSet ? Boolean.TRUE : Boolean.FALSE;
+ values[5] = renderedSet ? Boolean.TRUE : Boolean.FALSE;
         values[6] = rendererType;
         values[7] = saveAttachedState(context, listeners);
         assert(!transientFlag);
@@ -1506,6 +1496,24 @@
 
     // --------------------------------------------------------- Private Classes
 
+ // For state saving
+ private final static Object[] EMPTY_ARRAY = new Object[0];
+
+ // Empty iterator for short circuiting operations
+ private final static Iterator EMPTY_ITERATOR = new Iterator() {
+
+ public void remove() {
+ throw new UnsupportedOperationException();
+ }
+
+ public Object next() {
+ throw new NoSuchElementException("Empty Iterator");
+ }
+
+ public boolean hasNext() {
+ return false;
+ }
+ };
 
     // Private implementation of Map that supports the functionality
     // required by UIComponent.getFacets()
@@ -1704,6 +1712,9 @@
         }
 
         public Iterator iterator() {
+ if (this.size() == 0) {
+ return EMPTY_ITERATOR;
+ }
             return (new ChildrenListIterator(this));
         }
 
@@ -1875,20 +1886,46 @@
 
 
     // Private implementation of Iterator for getFacetsAndChildren()
- private static class FacetsAndChildrenIterator implements Iterator {
+ private final static class FacetsAndChildrenIterator implements Iterator {
+
+ private Iterator iterator;
+ private boolean childMode;
+ private UIComponent c;
 
- public FacetsAndChildrenIterator(List list) {
- this.iterator = list.iterator();
+ public FacetsAndChildrenIterator(UIComponent c) {
+ this.c = c;
+ this.childMode = false;
+ }
+
+ private void update() {
+ if (this.iterator == null) {
+ // we must guarantee that 'iterator' is never null
+ if (this.c.getFacetCount() != 0) {
+ this.iterator = this.c.getFacets().values().iterator();
+ this.childMode = false;
+ } else if (this.c.getChildCount() != 0) {
+ this.iterator = this.c.getChildren().iterator();
+ this.childMode = true;
+ } else {
+ this.iterator = EMPTY_ITERATOR;
+ this.childMode = true;
+ }
+ } else if (!this.childMode
+ && !this.iterator.hasNext()
+ && this.c.getChildCount() != 0) {
+ this.iterator = this.c.getChildren().iterator();
+ this.childMode = true;
+ }
         }
-
- private Iterator iterator;
 
         public boolean hasNext() {
- return (iterator.hasNext());
+ this.update();
+ return this.iterator.hasNext();
         }
 
         public Object next() {
- return (iterator.next());
+ this.update();
+ return this.iterator.next();
         }
 
         public void remove() {
@@ -2012,6 +2049,9 @@
         }
 
         public Iterator iterator() {
+ if (map.size() == 0) {
+ return EMPTY_ITERATOR;
+ }
             return (new FacetsMapEntrySetIterator(map));
         }
 
@@ -2197,6 +2237,9 @@
         }
 
         public Iterator iterator() {
+ if (map.size() == 0) {
+ return EMPTY_ITERATOR;
+ }
             return (new FacetsMapKeySetIterator(map));
         }
 
@@ -2299,6 +2342,9 @@
         }
 
         public Iterator iterator() {
+ if (map.size() == 0) {
+ return EMPTY_ITERATOR;
+ }
             return (new FacetsMapValuesIterator(map));
         }
 
Index: src/javax/faces/render/Renderer.java
===================================================================
RCS file: /cvs/javaserverfaces-sources/jsf-api/src/javax/faces/render/Renderer.java,v
retrieving revision 1.34
diff -u -r1.34 Renderer.java
--- src/javax/faces/render/Renderer.java 21 Apr 2005 18:55:30 -0000 1.34
+++ src/javax/faces/render/Renderer.java 3 Jul 2005 07:43:54 -0000
@@ -110,11 +110,13 @@
         if (context == null || component == null) {
             throw new NullPointerException();
         }
- Iterator kids = component.getChildren().iterator();
- while (kids.hasNext()) {
- UIComponent kid = (UIComponent) kids.next();
- kid.encodeAll(context);
- }
+ if (component.getChildCount() > 0) {
+ Iterator kids = component.getChildren().iterator();
+ while (kids.hasNext()) {
+ UIComponent kid = (UIComponent) kids.next();
+ kid.encodeAll(context);
+ }
+ }
     }