Ed Burns wrote:
>>>>>> On Mon, 27 Aug 2007 15:59:13 -0700, Ryan Lubke <Ryan.Lubke_at_Sun.COM> said:
>>>>>>
> RL> SECTION: Modified Files
> RL> ----------------------------
> RL> M src/javax/faces/component/UIData.java
> RL> - add client ID caching scheme for non nested tables.
> RL> In the non-nested case:
> RL> the base client ID will not change, so store this in
> RL> the StringBuilder. When a new per-row ID is needed
> RL> append the row index and call toString() on the builder,
> RL> and then reset the size of the builder to the base client
> RL> ID + the naming container separator
>
> Since diff doesn't always tell me what method I'm in, please say so
> here. For example, say "in getClientId(), add client ID caching..."
> Yeah, it should be obvious, but explicit is better.
>
>
Fair enough.
> RL> In the nested case, use the StringBuilder, but reset the
> RL> length to 0 after each computed ID
> RL> - In all cases where we call getChildren() or getFacets() call
> RL> getChildCount() and getFacetCount() and only call the methods
> RL> returning a collection when there are children or facets (reduces
> RL> the chance of creating empty collections)
> RL> - cache the result if isNestedWithinUIData since this is called multiple times
>
> I'm really sorry, but with a change like this, I have to ask why there
> is no addition to the automated tests? Unless there is some good reason
> for there not to be one, I cannot give an r=edburns.
>
And what exactly would you propose? There is a comprehensive set of
tests that already
exist for data tables/nested datatables. This didn't change how
datatable ultimately behaves,
all this tries to do is reduce the number of temp objects created.
> Some additional comments below.
>
> RL> SECTION: Diffs
> RL> ----------------------------
> RL> Index: src/javax/faces/component/UIData.java
> RL> ===================================================================
> RL> RCS file: /cvs/javaserverfaces-sources/jsf-api/src/javax/faces/component/UIData.java,v
> RL> retrieving revision 1.75.4.1
> RL> diff -u -r1.75.4.1 UIData.java
> RL> --- src/javax/faces/component/UIData.java 27 Aug 2007 04:57:28 -0000 1.75.4.1
> RL> +++ src/javax/faces/component/UIData.java 27 Aug 2007 22:52:13 -0000
> RL> @@ -177,6 +177,34 @@
> RL> */
> RL> private String var = null;
>
> RL> +
> RL> + /**
> RL> + * <p>Holds the base client ID that will be used to generate per-row
> RL> + * client IDs (this will be null if this UIData is nested within another).</p>
> RL> + */
> RL> + private String baseClientId;
>
> If this is not a part of the saved state, mark it transient using the
> "transient" java keyword. Also, I would like to have you explicitly
> initialize this to "null" since you are counting on that being the
> initial value in your new getClientId() method.
>
> RL> +
> RL> +
> RL> + /**
> RL> + * <p> Length of the cached <code>baseClientId</code> plus one for the
> RL> + * NamingContainer.SEPARATOR_CHAR. </p>
> RL> + */
> RL> + private int baseClientIdLength;
>
> Same here.
>
> RL> +
> RL> +
> RL> + /**
> RL> + * <p>StringBuilder used to build per-row client IDs.</p>
> RL> + */
> RL> + private StringBuilder clientIdBuilder;
>
> And here.
>
> RL> +
> RL> +
> RL> + /**
> RL> + * <p>Flag indicating whether or not this UIData instance is nested
> RL> + * within another UIData instance</p>
> RL> + */
> RL> + private Boolean isNested;
> RL> +
> RL> +
> RL> // -------------------------------------------------------------- Properties
>
>
> RL> @@ -680,11 +708,36 @@
> RL> if (context == null) {
> RL> throw new NullPointerException();
> RL> }
> RL> - String baseClientId = super.getClientId(context);
> RL> +
> RL> + if (baseClientId == null && clientIdBuilder == null) {
> RL> + if (!isNestedWithinUIData()) {
> RL> + clientIdBuilder = new StringBuilder(super.getClientId(context));
> RL> + baseClientId = clientIdBuilder.toString();
> RL> + baseClientIdLength = (baseClientId.length() + 1);
> RL> + clientIdBuilder.append(NamingContainer.SEPARATOR_CHAR);
> RL> + clientIdBuilder.setLength(baseClientIdLength);
> RL> + } else {
> RL> + clientIdBuilder = new StringBuilder();
> RL> + }
> RL> + }
>
> I need some inline comments here. When doing optimizations, comments
> are essential to maintainability.
>
> RL> if (rowIndex >= 0) {
> RL> - return (baseClientId + NamingContainer.SEPARATOR_CHAR + rowIndex);
> RL> + String cid;
> RL> + if (!isNestedWithinUIData()) {
> RL> + cid = clientIdBuilder.append(rowIndex).toString();
> RL> + clientIdBuilder.setLength(baseClientIdLength);
> RL> + } else {
> RL> + cid = clientIdBuilder.append(super.getClientId(context))
> RL> + .append(NamingContainer.SEPARATOR_CHAR).append(rowIndex)
> RL> + .toString();
> RL> + clientIdBuilder.setLength(0);
> RL> + }
> RL> + return (cid);
> RL> } else {
> RL> - return (baseClientId);
> RL> + if (!isNestedWithinUIData()) {
> RL> + return (baseClientId);
> RL> + } else {
> RL> + return super.getClientId(context);
> RL> + }
> RL> }
>
> RL> }
> RL> @@ -777,7 +830,8 @@
> RL> this.setRowIndex(newRow);
> RL> if (this.isRowAvailable()) {
> RL> found = super.invokeOnComponent(context,
> RL> - clientId, callback);
> RL> + clientId,
> RL> + callback);
> RL> }
> RL> }
> RL> }
> RL> @@ -850,7 +904,6 @@
> RL> FacesEvent rowEvent = revent.getFacesEvent();
> RL> rowEvent.getComponent().broadcast(rowEvent);
> RL> setRowIndex(oldRowIndex);
> RL> - return;
>
> RL> }
>
> RL> @@ -917,8 +970,7 @@
> RL> setDataModel(null); // Re-evaluate even with server-side state saving
> RL> if (null == saved || !keepSaved(context)) {
> RL> //noinspection CollectionWithoutInitialCapacity
> RL> - saved =
> RL> - new HashMap<String, SavedState>(); // We don't need saved state here
> RL> + saved = new HashMap<String, SavedState>(); // We don't need saved state here
> RL> }
>
> RL> iterate(context, PhaseId.APPLY_REQUEST_VALUES);
> RL> @@ -1103,47 +1155,43 @@
>
> RL> // Process each facet of this component exactly once
> RL> setRowIndex(-1);
> RL> - Iterator facets = getFacets().keySet().iterator();
> RL> - while (facets.hasNext()) {
> RL> - UIComponent facet = getFacets().get(facets.next());
> RL> - if (phaseId == PhaseId.APPLY_REQUEST_VALUES) {
> RL> - facet.processDecodes(context);
> RL> - } else if (phaseId == PhaseId.PROCESS_VALIDATIONS) {
> RL> - facet.processValidators(context);
> RL> - } else if (phaseId == PhaseId.UPDATE_MODEL_VALUES) {
> RL> - facet.processUpdates(context);
> RL> - } else {
> RL> - throw new IllegalArgumentException();
> RL> - }
> RL> - }
> RL> -
> RL> - // Process each facet of our child UIColumn components exactly once
> RL> - setRowIndex(-1);
> RL> - Iterator columns = getChildren().iterator();
> RL> - while (columns.hasNext()) {
> RL> - UIComponent column = (UIComponent) columns.next();
> RL> - if (!(column instanceof UIColumn)) {
> RL> - continue;
> RL> - }
> RL> - if (!column.isRendered()) {
> RL> - continue;
> RL> - }
> RL> - Iterator columnFacets = column.getFacets().keySet().iterator();
> RL> - while (columnFacets.hasNext()) {
> RL> - UIComponent columnFacet =
> RL> - column.getFacets().get(columnFacets.next());
> RL> + if (getFacetCount() > 0) {
> RL> + for (UIComponent facet : getFacets().values()) {
> RL> if (phaseId == PhaseId.APPLY_REQUEST_VALUES) {
> RL> - columnFacet.processDecodes(context);
> RL> + facet.processDecodes(context);
> RL> } else if (phaseId == PhaseId.PROCESS_VALIDATIONS) {
> RL> - columnFacet.processValidators(context);
> RL> + facet.processValidators(context);
> RL> } else if (phaseId == PhaseId.UPDATE_MODEL_VALUES) {
> RL> - columnFacet.processUpdates(context);
> RL> + facet.processUpdates(context);
> RL> } else {
> RL> throw new IllegalArgumentException();
> RL> }
> RL> }
> RL> }
>
> RL> + // Process each facet of our child UIColumn components exactly once
> RL> + setRowIndex(-1);
> RL> + if (getChildCount() > 0) {
> RL> + for (UIComponent column : getChildren()) {
> RL> + if (!(column instanceof UIColumn) || !column.isRendered()) {
> RL> + continue;
> RL> + }
> RL> + if (column.getFacetCount() > 0) {
> RL> + for (UIComponent columnFacet : column.getFacets().values()) {
> RL> + if (phaseId == PhaseId.APPLY_REQUEST_VALUES) {
> RL> + columnFacet.processDecodes(context);
> RL> + } else if (phaseId == PhaseId.PROCESS_VALIDATIONS) {
> RL> + columnFacet.processValidators(context);
> RL> + } else if (phaseId == PhaseId.UPDATE_MODEL_VALUES) {
> RL> + columnFacet.processUpdates(context);
> RL> + } else {
> RL> + throw new IllegalArgumentException();
> RL> + }
> RL> + }
> RL> + }
> RL> + }
> RL> + }
> RL> +
> RL> // Iterate over our UIColumn children, once per row
> RL> int processed = 0;
> RL> int rowIndex = getFirst() - 1;
> RL> @@ -1165,26 +1213,26 @@
> RL> // Perform phase-specific processing as required
> RL> // on the *children* of the UIColumn (facets have
> RL> // been done a single time with rowIndex=-1 already)
> RL> - Iterator kids = getChildren().iterator();
> RL> - while (kids.hasNext()) {
> RL> - UIComponent kid = (UIComponent) kids.next();
> RL> - if (!(kid instanceof UIColumn)) {
> RL> - continue;
> RL> - }
> RL> - Iterator grandkids = kid.getChildren().iterator();
> RL> - while (grandkids.hasNext()) {
> RL> - UIComponent grandkid = (UIComponent) grandkids.next();
> RL> - if (!grandkid.isRendered()) {
> RL> + if (getChildCount() > 0) {
> RL> + for (UIComponent kid : getChildren()) {
> RL> + if (!(kid instanceof UIColumn) || !kid.isRendered()) {
> RL> continue;
> RL> }
> RL> - if (phaseId == PhaseId.APPLY_REQUEST_VALUES) {
> RL> - grandkid.processDecodes(context);
> RL> - } else if (phaseId == PhaseId.PROCESS_VALIDATIONS) {
> RL> - grandkid.processValidators(context);
> RL> - } else if (phaseId == PhaseId.UPDATE_MODEL_VALUES) {
> RL> - grandkid.processUpdates(context);
> RL> - } else {
> RL> - throw new IllegalArgumentException();
> RL> + if (kid.getChildCount() > 0) {
> RL> + for (UIComponent grandkid : kid.getChildren()) {
> RL> + if (!grandkid.isRendered()) {
> RL> + continue;
> RL> + }
> RL> + if (phaseId == PhaseId.APPLY_REQUEST_VALUES) {
> RL> + grandkid.processDecodes(context);
> RL> + } else if (phaseId == PhaseId.PROCESS_VALIDATIONS) {
> RL> + grandkid.processValidators(context);
> RL> + } else if (phaseId == PhaseId.UPDATE_MODEL_VALUES) {
> RL> + grandkid.processUpdates(context);
> RL> + } else {
> RL> + throw new IllegalArgumentException();
> RL> + }
> RL> + }
> RL> }
> RL> }
> RL> }
> RL> @@ -1216,9 +1264,7 @@
> RL> */
> RL> private boolean keepSaved(FacesContext context) {
>
> RL> - Iterator clientIds = saved.keySet().iterator();
> RL> - while (clientIds.hasNext()) {
> RL> - String clientId = (String) clientIds.next();
> RL> + for (String clientId : saved.keySet()) {
> RL> Iterator messages = context.getMessages(clientId);
> RL> while (messages.hasNext()) {
> RL> FacesMessage message = (FacesMessage) messages.next();
> RL> @@ -1232,14 +1278,22 @@
>
> RL> }
>
> RL> - private boolean isNestedWithinUIData() {
> RL> - UIComponent parent = this;
> RL> - while (null != (parent = parent.getParent())) {
> RL> - if (parent instanceof UIData) {
> RL> - return true;
> RL> + private Boolean isNestedWithinUIData() {
> RL> + if (isNested == null) {
> RL> + UIComponent parent = this;
> RL> + while (null != (parent = parent.getParent())) {
> RL> + if (parent instanceof UIData) {
> RL> + isNested = Boolean.TRUE;
> RL> + break;
> RL> + }
> RL> + }
> RL> + if (isNested == null) {
> RL> + isNested = Boolean.FALSE;
> RL> }
> RL> + return isNested;
> RL> + } else {
> RL> + return isNested;
> RL> }
> RL> - return (false);
> RL> }
>
>
> RL> @@ -1250,11 +1304,11 @@
> RL> private void restoreDescendantState() {
>
> RL> FacesContext context = getFacesContext();
> RL> - Iterator kids = getChildren().iterator();
> RL> - while (kids.hasNext()) {
> RL> - UIComponent kid = (UIComponent) kids.next();
> RL> - if (kid instanceof UIColumn) {
> RL> - restoreDescendantState(kid, context);
> RL> + if (getChildCount() > 0) {
> RL> + for (UIComponent kid : getChildren()) {
> RL> + if (kid instanceof UIColumn) {
> RL> + restoreDescendantState(kid, context);
> RL> + }
> RL> }
> RL> }
>
> RL> @@ -1292,16 +1346,16 @@
> RL> }
>
> RL> // Restore state for children of this component
> RL> - Iterator kids = component.getChildren().iterator();
> RL> - while (kids.hasNext()) {
> RL> - restoreDescendantState((UIComponent) kids.next(), context);
> RL> + if (component.getChildCount() > 0) {
> RL> + for (UIComponent kid : component.getChildren()) {
> RL> + restoreDescendantState(kid, context);
> RL> + }
> RL> }
> RL> +
> RL> // Restore state for facets of this component
> RL> - Iterator facetNames = component.getFacets().keySet().iterator();
> RL> - while (facetNames.hasNext()) {
> RL> - UIComponent c = component.getFacet((String) facetNames.next());
> RL> - if (c != null) {
> RL> - restoreDescendantState(c, context);
> RL> + if (component.getFacetCount() > 0) {
> RL> + for (UIComponent facet : component.getFacets().values()) {
> RL> + restoreDescendantState(facet, context);
> RL> }
> RL> }
>
> RL> @@ -1315,11 +1369,11 @@
> RL> private void saveDescendantState() {
>
> RL> FacesContext context = getFacesContext();
> RL> - Iterator kids = getChildren().iterator();
> RL> - while (kids.hasNext()) {
> RL> - UIComponent kid = (UIComponent) kids.next();
> RL> - if (kid instanceof UIColumn) {
> RL> - saveDescendantState(kid, context);
> RL> + if (getChildCount() > 0) {
> RL> + for (UIComponent kid : getChildren()) {
> RL> + if (kid instanceof UIColumn) {
> RL> + saveDescendantState(kid, context);
> RL> + }
> RL> }
> RL> }
>
> RL> @@ -1352,16 +1406,16 @@
> RL> }
>
> RL> // Save state for children of this component
> RL> - Iterator kids = component.getChildren().iterator();
> RL> - while (kids.hasNext()) {
> RL> - saveDescendantState((UIComponent) kids.next(), context);
> RL> + if (component.getChildCount() > 0) {
> RL> + for (UIComponent uiComponent : component.getChildren()) {
> RL> + saveDescendantState(uiComponent, context);
> RL> + }
> RL> }
> RL> +
> RL> // Save state for facets of this component
> RL> - Iterator facetNames = component.getFacets().keySet().iterator();
> RL> - while (facetNames.hasNext()) {
> RL> - UIComponent c = component.getFacet((String) facetNames.next());
> RL> - if (c != null) {
> RL> - saveDescendantState(c, context);
> RL> + if (component.getFacetCount() > 0) {
> RL> + for (UIComponent facet : component.getFacets().values()) {
> RL> + saveDescendantState(facet, context);
> RL> }
> RL> }
>
>
>
>
> RL> ---------------------------------------------------------------------
> RL> To unsubscribe, e-mail: dev-unsubscribe_at_javaserverfaces.dev.java.net
> RL> For additional commands, e-mail: dev-help_at_javaserverfaces.dev.java.net
>
>