No need to get defensive, I apologize that I must have been too tired last night and I suppose that I should not have been replying to coding emails in that state. The point that I was making, is simply if JSF core, both Mojarra and MyFaces cache the client IDs in UIComponentBase and clear that cache when setId is called and Trinidad has to honor this behavior, whether or not the design of stamping components in JSF is less than ideal (to put it nicely), then instead of abstaining from this code, we could embrace it for performance reasons. The code will not get slower as we are already traversing the components to make JSF core components happy and the cached client IDs in the Trinidad components should make things much faster as getClientId is called a lot in JSF.
In summary I was simply proposing that we add a "private transient String _clientId;" or equivalent to UIXComponentBase that is set in the first call to getClientId and cleared in calls to setId() just like in UIComponentBase and this way we avoid any breaking API changes. -Andrew On Mon, Jan 4, 2010 at 11:02 PM, Blake Sullivan <blake.sulli...@oracle.com> wrote: > Oh, I thought it was clear from my e-mail that I know how the RI implements > setClientId(). Note that the RI implementation isn't really compatible with > the spec language. The UIData doesn't change the children id attributes in > any Java sense of the word (precisely the same instance is used). In fact, > the id portion of the current spec language comes from Adam Winer and his > javadoc updates were submitted without knowing the details of the just > checked-in UIData component. Craig McClanahan made the change to add > clientId caching at the same time he added the UIData implementation. There > was no EG discussion of Craig's setId() hack. In fact, the check-in > comments make clear that the clientId was cached in order to (incorrectly) > fix the problem JSF was having that if the id attribute wasn't set, each > call to getClientId() would return a different value. > > As for the JavaDoc--of course it is normative--it just seems a little odd to > make the specification and the JavaDoc mutually dependent when there is no > need to do so. > > Now it is completely fair to argue that we are stuck with the bogus RI > cache-clearing implementation detail anyway, so we might as well embrace it, > which is why I laid out the steps we would need to take to correctly > implement the caching. Do you have any comments on those? > > -- Blake Sullivan > > Andrew Robinson said the following On 1/4/2010 8:08 PM PT: >> >> The JavaDoc is part of the spec, and according to JSF 1.2, the >> getClientId has this restriction (see http://goo.gl/eula): >> "The return from this method must be the same value throughout the >> lifetime of the instance, unless the id property of the component is >> changed, or the component is placed in a NamingContainer whose client >> ID changes (for example, UIData). However, even in these cases, >> consecutive calls to this method must always return the same value" >> >> As mentioned here, the client ID must not change unless someone >> changes the ID property of the component, the clause of the UIData >> does not mention how it should be updated though. >> >> Now the hack? that JSF uses can be seen here: >> http://kickjava.com/src/javax/faces/component/UIData.java.htm >> >> Look at the getInitialClientId method. >> >> Now looking at the UIComponentBase.java you can see that it caches the >> client ID as a member variable in getClientId: >> http://goo.gl/bNn1 >> >> You can also see in that source they set the client ID to null in >> setId(String) >> >> Now look at the code for UIData.java: >> http://goo.gl/1zCC >> >> There is this: >> private void restoreDescendantState(UIComponent component, >> FacesContext context) { >> >> // Reset the client identifier for this component >> String id = component.getId(); >> component.setId(id); // Forces client id to be reset >> >> Looking at the MyFaces core source code for UIData.java (jsf 1.2): >> http://goo.gl/1d7j >> >> It says this: >> // reset the client id (see spec 3.1.6) >> component.setId(component.getId()); >> >> At the end of 3.1.6 of the JSF 1.2 spec is this: >> "The value returned from this method must be the same throughout >> the lifetime of the component instance unless setId() is called, in >> which case it will be >> recalculated by the next call to getClientId()." >> >> So, according to this, the caching that JSF core does is okay per this >> spec. and Trinidad is actually incorrect in that its client IDs may >> change without setId() being called. Not very intuitive or easy to >> find, but it is there. >> >> I happen to know about this as I had to fix a bug in the Trinidad >> Table component because JSF component's setId() functions were not >> being called and therefore the client IDs were all wrong, otherwise I >> would have had no idea this was true. >> >> -Andrew >> >> >> On Mon, Jan 4, 2010 at 5:37 PM, Blake Sullivan >> <blake.sulli...@oracle.com> wrote: >> >>> >>> Andrew Robinson said the following On 1/4/2010 2:38 PM PT: >>> >>> Andrew, >>> >>> Thanks for commenting on this. >>> >>>> >>>> Why isn't UIXComponentBase caching the client ID? >>>> >>> >>> Because of stamping and because I suspect that FacesBean would have >>> complicated things. >>> >>>> >>>> The JSF core >>>> components already cache their client ID. Part of the spec. is that >>>> setId() calls on the component clear the cache. >>>> >>> >>> Can you point me to the section, where it says this? This behavior is >>> not >>> part of section 3.1.6 of the JSF 2.0 Specification (which is wrong >>> anyway, >>> as the behavior it describes would preclude the stamping implemented by >>> UIData. And is it just me or do others think that it is backwards that >>> the >>> specification would actually tell the reader to look in the javadoc to >>> find >>> out what the specified behavior is?) >>> >>> >>>> >>>> In Trinidad, we must >>>> recursively call setId on child components when indexes change >>>> (tr:iterator, tr:table, etc.) so that JSF components function >>>> correctly. >>>> >>> >>> I believe this is an JSF RI implementation detail. I see nothing in the >>> specification that says that calling setId() will clear out the current >>> clientId even if the passed in id is identical to the component's current >>> id. This behavior is also not exactly performant as each row change for >>> any >>> reason requires a walk of the row subtree to clear out the clientIds. >>> >>>> >>>> Since we already have to do the work of clearing the cached >>>> client IDs for JSF components, can we not take advantage of this for >>>> Trinidad components? I would think that this would be a large >>>> performance improvement. >>>> >>>> >>> >>> We if we put in JIRA-1668--Speed up UIXComponent.getId(), we could, since >>> that JIRA solves the problem of how to add programmatic logic to >>> get/setId() >>> as a side-effect. The issues then would be quality and some performance. >>> The quality part would be double-checking that the StampState class >>> (which >>> implements the setId resetting behavior) is being used correctly in all >>> cases (since we would only notice right now if non UIComponents were >>> used) >>> and checking whether we will have problems with tag implementations that >>> bogusly call getClientId(). >>> >>> The two mechanisms are complimentary, but I would prefer not to make the >>> api >>> change if the caching change is good enough. >>> >>> -- Blake Sullivan >>> >>>> >>>> Do you feel that your proposed changes would give us better >>>> performance, or perhaps could we combine the two? >>>> >>>> -Andrew >>>> >>>> On Thu, Dec 31, 2009 at 4:23 PM, Blake Sullivan >>>> <blake.sulli...@oracle.com> wrote: >>>> >>>> >>>>> >>>>> Calling UIXComponentBase.getClientId consumes a great deal of transient >>>>> memory. Under light loads, this doesn't matter--the objects are >>>>> extremely >>>>> short-lived and are allocated out of the first-generation heap. >>>>> However, >>>>> when large numbers of users are accessing the server simultaneously >>>>> these >>>>> allocations contribute to first-generation heap exhaustion and >>>>> first-generation heap GC's when deeply nested NamingContainers are >>>>> used. >>>>> >>>>> There are two reasons that large amounts of transient memory is >>>>> consumed >>>>> in >>>>> these cases: >>>>> 1) UIXComponentBase doesn't cache clientIds because the clientIds are >>>>> partly >>>>> determined by the component's ancestors and there are cases (such as >>>>> stamping), where multiple clientIds may map to a single component >>>>> instance >>>>> >>>>> 2) clientIds are generated recursively by: >>>>> a) calling getContainerClientId() and appending the NamingContainer >>>>> separator and the component's id to the result >>>>> b) getContainerClientId() is implemented by calling >>>>> getContainerClientId() >>>>> and doing likewise >>>>> >>>>> So, each NamingContainer in the hierarchy is going to: >>>>> 1) Get it's ancestor's container clientId and if one exists >>>>> 2) Get it's id attribute >>>>> 3) Allocate a StringBuilder to contain these two Strings, append them >>>>> together >>>>> 4) Convert the StringBuilder to a String and return the result >>>>> >>>>> An earlier JIRA used a ThreadLocal StringBuilder to remove the >>>>> StringBuilder >>>>> allocation in step 3) in the common case, halving the transient memory >>>>> usage, however we still have the String allocations made necessary by >>>>> the >>>>> use of String getContainerClientId(FacesContext context, UIComponent >>>>> child). >>>>> >>>>> For a 20 row table containing 10 columns nested four NamingContainers >>>>> deep >>>>> (counting the table as one of these), we end up with 1000 String >>>>> allocations, which wouldn't necessarily be that bad if the size of the >>>>> Strings wasn't increasing and if the Rendering code was the only code >>>>> calling getClientId() (InvokeOnComponent is the primary culprit here, >>>>> though >>>>> replacing invokeOnComponent calls with visitTree calls improves >>>>> things). >>>>> >>>>> The proposed solution is to replace generating new Strings at each >>>>> NamingContainer level with appending the NamingContainer ids into a >>>>> StringBuilder (in fact, the shared StringBuilder) passed to the >>>>> appending >>>>> code--a String is only generated when the returning the value from >>>>> getClientId(). In scalability testing, this change has been worth >>>>> about >>>>> 8%. >>>>> >>>>> The advantages of this approach are: >>>>> 1) If the component code compiles, the code will almost certainly work >>>>> correctly >>>>> 2) It clientId caching is also used, this approach speeds up generation >>>>> of >>>>> the cached result >>>>> >>>>> The disadvatanges of this approach is: >>>>> 1) Any overrides of getClientId() or getContainerClientId() must be >>>>> changed >>>>> to overrides of appendClientId() or appendContainerClientId(). To >>>>> enforce >>>>> this, getClientId() and getContainerClientId() are made final on >>>>> UIXComponentBase. This, is of course, an incompatible api change >>>>> >>>>> The new/changed apis on UIXComponentBase: >>>>> >>>>> /** >>>>> * Appends the container's clientId for the requesting child to the >>>>> StringBuilder, returning the passed in StringBuilder. >>>>> * Component implementations are only allowed to mutate the >>>>> StringBuilder >>>>> other than to append. >>>>> * Subclasses that wish to modify the clientIds returned for their >>>>> children >>>>> should override this method rather than >>>>> * <code>getContainerClientId</code>. >>>>> * @param context FacesContext >>>>> * @param child Optional child component that is requesting the >>>>> container's >>>>> * clientId >>>>> * @param clientIdAppendable StringBuilder to append the container's >>>>> clientId to >>>>> * @see #getContainerClientId(FacesContext, UIComponent) >>>>> */ >>>>> public StringBuilder appendContainerClientId( >>>>> FacesContext context, >>>>> UIComponent child, >>>>> StringBuilder clientIdAppendable) >>>>> >>>>> /** >>>>> * Appends the clientId of this component to the StringBuilder, >>>>> returning >>>>> the passed in StringBuilder. >>>>> * Component implementations typically only mutate the StringBuilder to >>>>> append. >>>>> * Subclasses that wish to modify the clientIds that they return should >>>>> override this method rather than >>>>> * <code>getClientId</code>. >>>>> * @param context FacesContext >>>>> * @param clientIdAppendable StringBuilder to append the component's >>>>> clientId to >>>>> * @return the clientIdAppendable StringBuilder passed in as the >>>>> clientIdAppendable parameter >>>>> * @see #getClientId >>>>> */ >>>>> public StringBuilder appendClientId(FacesContext context, >>>>> StringBuilder >>>>> clientIdAppendable) >>>>> >>>>> /** >>>>> * Final override of getContainerClientId to make >>>>> <code>appendContainerClientId</code> >>>>> * the supported hook for modifying the clientIds of a component's >>>>> children. >>>>> * @see #appendContainerClientId >>>>> */ >>>>> �...@override >>>>> public String getContainerClientId(FacesContext context) >>>>> >>>>> /** >>>>> * Final override of getContainerClientId to make >>>>> <code>appendContainerClientId</code> >>>>> * the supported hook for modifying the clientIds of a component's >>>>> children. >>>>> * The implementation uses <code>appendContainerClientId</code> to >>>>> calculate >>>>> the >>>>> * the container's clientId prefix with far fewer temporary Strings >>>>> than >>>>> * the class JSF implementation. >>>>> * @param context FacesContext >>>>> * @param child Optional child component that is requesting the >>>>> container's >>>>> * clientId >>>>> * @return the clientId prefix to add to the child's id >>>>> * @see #appendContainerClientId >>>>> */ >>>>> �...@override >>>>> public final String getContainerClientId(FacesContext context, >>>>> UIComponent >>>>> child) >>>>> >>>>> /** >>>>> * Final override of getClientId to make <code>appendClientId</code> >>>>> * the supported hook for modifying the clientIds of a component. >>>>> * The implementation uses <code>appendClientId</code> to calculate the >>>>> * the component's clientId with far fewer temporary Strings than >>>>> * the class JSF implementation. >>>>> * @param context FacesContext >>>>> * @return the clientId >>>>> * @see #appendClientId >>>>> */ >>>>> �...@override >>>>> public final String getClientId(FacesContext context) >>>>> >>>>> >>>>> >>> >>> > >
