That works as well, not optimal, but does reduce the backward compatibility
risk to the minimum, so I guess it's the best course of action.
On Wed, Jan 6, 2010 at 7:20 PM, Blake Sullivan <blake.sulli...@oracle.com>wrote:
> OK. I got smarter and looked more carefully at the FacesBean
> implementations. The best solution is to modify
> org.apache.myfaces.trinidadinternal.bean.UIXFacesBeanImpl to hang onto the
> UIXComponent passed to init() and then override setProperty(),
> getLocalPropertyImpl(), saveState(), and restoreState() to handle the id
> attribute by calling _component.getId()/setId() as appropriate. No api
> changes and the change is encapsulated in UIXFacesBeanImpl and
> UIXComponentBase.
>
> -- Blake Sullivan
>
>
> \Simon Lessard said the following On 1/6/2010 10:40 AM PT:
>
> 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 <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com>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 <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com>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 <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com>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 <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com>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 <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache
> .org> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org> <mat...@apache.org> <mat...@apache.org>
> <mat...@apache.org>wrote:
>
>
>
> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> <blake.sulli...@oracle.com>
> <blake.sulli...@oracle.com> 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
>
>
>
>
>
>
>
>
>
>
>
