Hi Blake, Yep, that's exactly what I meant. I'm aware that the main risk lies with compatibility, but I think it's minimal.
Regards, ~ Simon On Tue, Jan 5, 2010 at 8:00 PM, Blake Sullivan <[email protected]>wrote: > Simon Lessard said the following On 1/5/2010 2:34 PM PT: > > Blake, > > For 1, both possibilities exist. However, I would prefer them to not be > available on the FacesBean from a performance PoV. Those don't have indexed > property keys anyway so the lookup for them is actually quite inefficient. > That would requires some additional changes to the state saving though. > > > Hmm. I believe that FlaggedPropertyMap uses a HashMap to store these, so > they aren't that bad. (this isn't necessarily the best choice for size > reasons, but that's a separate issue) > > We are talking about optimization at the constant level--all proposed > mechanism are O(1) in all cases right now. The differences between the > different proposals are: > > > 1) Current > 2) ValueExpression Proposal > 3) Split AttributeMap and FacesBean (Simon's proposal A) > 4) Simon's Custom Properties in FacesBean-B > UIComponent.getFoo() > Flagged Map Access > Flagged Map Access Flagged Map Access Flagged Map Access * > UIComponent.getId()* *Map Access (since id always set) > * *Direct > * *Direct > * *Direct* > FacesBean.getProperty(FOO_KEY) > Flagged Map Access Flagged Map Access Flagged Map Access > > Flagged Map Access *UIComponent.getAttributes().get("foo") > * *2 Map Accesses* *2 Map Accesses* *2 Map Accesses (one flagged) and a > reflection call > * *2 Map Accesses (one flagged) and a reflection call* * > UIComponent.getAttributes().get("id") > * *2 Map Accesses > * *2 Map Accesses, 1 cast and function call > * *1 Map Access and reflection call > * *1 Map Access and reflection call* *UIComponent.getAttributes().get("custom > foo")* *2 Map Accesses* *2 Map Accesses* *Map access > * *2 Map Accesses (one flagged) and a reflection call* * > *I've bold-faced the rows that are actually different. > > The proposals also differ slightly with regards to whether the same values > are available through the attributeMap, UIComponent direct accessor, and the > FacesBean. The current implementation makes all three of these identical. > The ValueExpression does likewise. In the split implementation custom > attributes aren't available from the FacesBean. In the custom properties > case, proeprties that were optimized, wouldn't be available from the > FacesBean, which may or may not be OK (some Renderer apis unfortunately only > pass FacesBeans and not the UIComponent as well) > > Another option for speeding up attributes like getId(), would be to add a > different flag to the PROPERTY_KEYS, requesting that the storage of this > particular property be optimized. Depending on how flexible the use of > these keys needed to be, this could result in only the lowest keys being > allowed to be optimized (so that one index would suffice), or adding a > separate optimized index. This would result in hybrid storage where the > optimized keys were available for direct access from an array. However, > while this has some storage size advantages, I doubt it would actually be > significantly faster than the current HashMap--the performance issue is > really the work we do before we get to the HashMap. In addition, this > solution would not make it easier to add code to do extra work in order to > handle, say clientId caching. > > I believe that 3) definitely and 4), potentially, have backwards > compatibility issues. My biggest complaints with 2) is that checking the > extra flag is a little gross and is potentially make other attribute access > slightly slower (though profiling doesn't show it). The extra custom > ValueExpression isn't great, but on the other hand, it is essentially a > minimal object--just an object with a back pointer to the component. In > addition, the size if far outweighed by our using a HashMap to store the > properties (which I will come up with a proposal for fixing). I agree that > both 3) and 4) make it easier to optimize additional properties, rendered > being the most important, however, not having rendered available from the > FacesBean would almost certainly cause backwards compatibility problems > > Simon, does this correctly represent your proposals? Essentially, I'm > worried about the compatibility issues. > > -- Blake Sullivan > > So, we would have: > > For predefined properties: > 1. Direct access: > UIComponent.getX() --> FacesBean.getProperty(X_KEY) ==> O(1), one indexed > map loopkup > > 2. FacesBean access (in renderers): > FacesBean.getProperty(X_KEY) ==> O(1), one indexed map loopkup > > 3. Attribute map access: > UIComponent.getAttributes().get("x") --> UIComponent.getX() --> > FacesBean.getProperty(X_KEY) ==> O(1), one hashed map lookup > (String.hashCode is cached), one reflection call, one indexed map loopkup > > For custom properties: > 3. Attribute map access: > UIComponent.getAttributes().get("x") ==> O(1), one hashed map lookup > > > If we keep setting the custom properties in FacesBean, then the AttributeMap > must also have a link to the FacesBean and the algorithm would be > Accessor accessor = getAccessor(propertyName); > if (accessor == null) > { > // custom property, use the faces bean directly > PropertyKey propertyKey = _bean.getType().findKey(propertyName); > if (propertyKey == null) > propertyKey = PropertyKey.createPropertyKey(propertyName); > > return _bean.getProperty(propertyKey); > } > else > { > // predefined property > return accessor.get(component); > } > > private Accessor getAccessor(String propertyName) > { > // Using an accessor cache should be required, sadly Method is not > Serializable, > // but it would still be possible to cache it in a semi static > ClassLoaderLocal Map<Class<?>, Map<String, Accessor>> ... > } > > private static class Accessor > { > private Method getter; > private Method setter; > private Class<?> type; > > public Object get(Object o) > { > return getter.invoke(o); > } > > // ... > } > > > Regards, > > ~ Simon > > On Tue, Jan 5, 2010 at 4:31 PM, Blake Sullivan <[email protected]> > <[email protected]>wrote: > > > > Simon, > > For part 1), are you proposing that we stop overriding getAttributes()? If > so, private implementation properties used by the component and set by using > setAttribtue(), would not be available on the FacesBean. So I assume that > you are suggesting that we change the components to set these on the > FacesBean directly in these cases. > > I did a quick grep and the components and they are using the attributeMap. > It is unclear how many of these would be left if we removed all of the cases > where the direct accessor could be used, and the cases where we would switch > to direct FacesBean access, however these case do suffer from the worst of > all worlds as far as performance, since the pay the cost of both reflection > and Map access. > > -- Blake Sullivan > > > Simon Lessard said the following On 1/5/2010 12:32 PM PT: > > Hi Blake, > > Actually it's harsher/simpler than that. Assuming that .getAttributes() is > very rarely used in a Trinidad application (exception for custom > attributes). > > 1. Have AttributeMap work exactly like standard JSF's AttributeMap. That is, > always call the getter/setter on the component (which in turn will use the > FacesBean if needed). For custom properties, they could either be stored in > the FacesBean or on the component itself > 2. Handle the id attribute manually for state saving purposes in > UIXComponentBase > > Point 1. does impact performances vs. direct FacesBean access when accessing > defined properties, but all Trinidad applications most likely directly use > FacesBean.getProperty( > PropertyKey) directly, like all our renderer do. For > custom properties, there's absolutely no hit. > > > Regards, > > ~ Simon > > On Tue, Jan 5, 2010 at 3:24 PM, Blake Sullivan <[email protected]> > <[email protected]> <[email protected]> > <[email protected]>wrote: > > > > Is your suggestion that we > 1) Add a new Map(String, Object>) implementation that takes both the > FacesBean and the UIComponent > 2) Explicitly test for the id attribute in all of the accessor and mutator > functions, in addition to the the Sets returned > 3) Override the state saving/restoration code to explicitly handle id > > -- Blake Sullivan > > Simon Lessard said the following On 1/5/2010 12:08 PM PT: > > Have the AttributeMap call the getId/setId. The contract for the Map > returned by getAttributes is supposed to call the getter/setter method on > the component anyway, > fromhttp://java.sun.com/javaee/5/docs/api/javax/faces/component/UIComponent.html#getAttributes%28%29 > : > > > > > - get() - If the property is readable, call the getter method and > return the returned value (wrapping primitive values in their corresponding > wrapper classes); otherwise throw IllegalArgumentException. > - put() - If the property is writeable, call the setter method to set > the corresponding value (unwrapping primitive values in their corresponding > wrapper classes). If the property is not writeable, or an attempt is made > to > set a property of primitive type to null, throw > IllegalArgumentException. > > > > > Regards, > > ~ Simon > > > On Tue, Jan 5, 2010 at 2:50 PM, Blake Sullivan <[email protected]> > <[email protected]> <[email protected]> > <[email protected]> <[email protected]> > <[email protected]> <[email protected]> > <[email protected]>wrote: > > > > The reason is that we need to support AttributeMap/component accessor > equivalence--get/set of the id attribute through the Map is supposed to work > correctly. The ValueExpression only exists to make this work. > > -- Blake Sullivan > > Simon Lessard said the following On 1/5/2010 10:57 AM PT: > > Hi, > > Why not simply NOT support a PropertyKey for the id attribute? I know it > isn't consistent with the other properties, but id is a very special case > not supporting EL anyway. In all the project I ever did, I never used > FacesBean.getProperty(ID_ > PROPERTY_KEY). The only drawback I would see is if > the component's id actually need to be read in a property getter method in a > renderer which receive only the FacesBean instance and not the component > itself. That would be much faster than a custom ValueExpression and the > memory footprint would also be better. > > Regards, > > ~ Simon > > On Tue, Jan 5, 2010 at 1:49 PM, Matthias Wessendorf <[email protected]> > <[email protected]> <[email protected]> <[email protected]> > <[email protected]> <[email protected]> <[email protected]> > <[email protected]> <[email protected]> <[email protected]> > <[email protected]> <[email protected]> <[email protected]> > <[email protected]> <[email protected]> <[email protected]>wrote: > > > > On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<[email protected]> > <[email protected]> <[email protected]> > <[email protected]> <[email protected]> > <[email protected]> <[email protected]> > <[email protected]> <[email protected]> > <[email protected]> <[email protected]> > <[email protected]> <[email protected]> > <[email protected]> <[email protected]> > <[email protected]> wrote: > > > UIComponent.getId() is by far the most requested component attribute. > > > There > > > are a number of reasons for this: > 1) The JSF RI has an issue in the JSP-JSF integration which causes > > > getId() > > > to be called n^2 times where n is the number of children a component has > > > I guess this is true for MyFaces as well, right? > > > > 2) getClientId() calls getId() > 3) FindComponent calls getId() > 4) The tree visiting code trades off calls to getClientId() for calls to > getId() > > FacesBean optimizes attribute Map access at the expense of access > > > directly > > > through the component. The the extent that Renderers are Components are > accessing the attributes through the attribute Map, this is fine, however > even the Renderers access attributes common to all UIComponents such as > > > id() > > > through the component directly. Considering the huge number of times > > > that > > > the the id is accessed (for some renders, this was 8% of the rendering > time), it makes sense to optimize this path. > > The proposal is to: > 1) Store the id an an instance variable on the UIXComponent > 2) Add a new capability flag to PropertyKey indicating that the property > > > is > > > actually stored elsewhere using a ValueExpression will be stored as the > property's value in the PropertyMap. For access through the FacesBean, > > > the > > > ValueExpression will be evaluated to get/set the actual value > 3) For state saving the ValueExpression is used to retrieve the actual > > > value > > > and for state restoration the ValueExpression (which has been > > > rebootstrapped > > > by the UIXComponent) is used to write the value back > 4) Instead of setting the id attribute in the FacesBean, UIXComponent > > > stores > > > it locally and sets an ValueExpression implementation into the FacesBean > that retrieves the value from the UIXComponent > > > +1 on api/patch > > > > API Changes: > > PropertyKey: > > add > > /** > * Capability indicating that values for this property should be > * be stored and retrieved through a ValueExpression rather than on the > * FacesBean itself > */ > static public final int CAP_VALUE_EXPRESSION_IMPLEMENTATION = 16; > > /** > * Returns <code>true</code> if property values for this key are set and > > > get > > > * using a ValueExpression rather than storing the value in the > > > FacesBean. > > > * @return <code>true</code> if properties values for this key are > > > retrieved > > > * with a ValueExpression. > */ > public boolean usesValueExpressionAsImplementation() > > After this change id retrieval doesn't make the 1% YourKit profiler hot > > > spot > > > cut off > > > > > > -- > Matthias Wessendorf > > blog: http://matthiaswessendorf.wordpress.com/ > > > > sessions: http://www.slideshare.net/mwessendorf > twitter: http://twitter.com/mwessendorf > > > > > > > > >
