You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@myfaces.apache.org by Blake Sullivan <bl...@oracle.com> on 2009/12/31 23:12:52 UTC
[TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
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
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
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
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Blake Sullivan <bl...@oracle.com>.
Hmm. It would be nice if the HTML formatting actually worked
1) Current Implementation:
UIComponent.getFoo(): Flagged Map Access
UIComponent.getId(): Map Access (since id
always set)
FacesBean.getProperty(FOO_KEY): Flagged Map Access
UIComponent.getAttributes().get("foo"): 2 Map Accesses
UIComponent.getAttributes().get("id"): 2 Map Accesses
UIComponent.getAttributes().get("custom foo"): 2 Map Accesses
2)Custom ValueExpression for id
UIComponent.getFoo(): Flagged Map Access
UIComponent.getId(): Direct instance access
FacesBean.getProperty(FOO_KEY): Flagged Map Access
UIComponent.getAttributes().get("foo"): 2 Map Accesses
UIComponent.getAttributes().get("id"): 2 Map Accesses, 1 cast,
one function call
UIComponent.getAttributes().get("custom foo"): 2 Map Accesses
3)Store Custom Properties Only in AttributeMap, Optimized attributes not
in FacesBean
UIComponent.getFoo(): Flagged Map Access
UIComponent.getId(): Direct instance access
FacesBean.getProperty(FOO_KEY): Flagged Map Access
UIComponent.getAttributes().get("foo"): 2 Map Accesses (1
Flagged), 1 reflection call
UIComponent.getAttributes().get("id"): 1 Map Access 1 reflection
call
UIComponent.getAttributes().get("custom foo"): 1 Map Access
4)Optimized attributes not in FacesBean
UIComponent.getFoo(): Flagged Map Access
UIComponent.getId(): Direct instance access
FacesBean.getProperty(FOO_KEY): Flagged Map Access
UIComponent.getAttributes().get("foo"): 2 Map Accesses (1
Flagged), 1 reflection call
UIComponent.getAttributes().get("id"): 1 Map Access 1 reflection
call
UIComponent.getAttributes().get("custom foo"): 2 Map Accesses (1
Flagged), 1 reflection call
Yet another alternative to the ValueExpression would be for the
PropertyKey to be able to indicate that it would prefer its value be
stored on the behavior object for the FacesBean, if-any. The
FacesBeanImplementation would then be responsible for knowing how to
call the behavior object. In the FacesBeans representing components,
the FacesBean would maintain the backpointer to the component and call
the component getId() directly. This is a little less flexible than the
ValueExpression scheme (which allowed to data to be stored anywhere),
but this is flexibility we don't need for this feature. We would save
the ValueExpression instance itself and the storage for the
id->ValueExpression mapping.
-- Blake Sullivan
Blake Sullivan said the following On 1/5/2010 5:00 PM PT:
> 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
>> <bl...@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
>>> <bl...@oracle.com> <bl...@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
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@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
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org>wrote:
>>>
>>>
>>>
>>> On Thu, Dec 31, 2009 at 11:12 PM, Blake
>>> Sullivan<bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
RE: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Maria Kaval <MA...@oracle.com>.
Are JIRA 1668 and JIRA 1700 dupes?
Maria
-----Original Message-----
From: Blake Sullivan
Sent: Tuesday, January 19, 2010 12:09 PM
To: MyFaces Development
Subject: Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
The api part of the fix is that UIXFacesBeanImpl and
UIXEditableFacesBeanImpl have been moved to
org.apache.myfaces.trinidad.component. There is nothing interesting
about these classes and it was kind of rude that they were present only
in impl, since everyone needs an implementation (for example, the test
code had copies of these classes). By moving these classes to api,
FacesBeanImpl can tell when it is OK to optimize by storing the id
locally and when it isn't.
There is also one subtle behavior change. getId() never returns null in
the current code. The old code would return null for getId() (but not
for getClientId()) until the id was set. That behavior was bad since it
resulted in cases where the last element of the clientId was not the
component's id. The JSF RI code as slightly better, but still incorrect
behavior--it sets the component's id as a side effect of calling
getClientId() if the component's id isn't set.
-- Blake Sullivan
Blake Sullivan said the following On 1/6/2010 4:20 PM PT:
> 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
>> <bl...@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
>>> <bl...@oracle.com> <bl...@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
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@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
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@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
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org>wrote:
>>>
>>>
>>>
>>> On Thu, Dec 31, 2009 at 11:12 PM, Blake
>>> Sullivan<bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Blake Sullivan <bl...@oracle.com>.
The api part of the fix is that UIXFacesBeanImpl and
UIXEditableFacesBeanImpl have been moved to
org.apache.myfaces.trinidad.component. There is nothing interesting
about these classes and it was kind of rude that they were present only
in impl, since everyone needs an implementation (for example, the test
code had copies of these classes). By moving these classes to api,
FacesBeanImpl can tell when it is OK to optimize by storing the id
locally and when it isn't.
There is also one subtle behavior change. getId() never returns null in
the current code. The old code would return null for getId() (but not
for getClientId()) until the id was set. That behavior was bad since it
resulted in cases where the last element of the clientId was not the
component's id. The JSF RI code as slightly better, but still incorrect
behavior--it sets the component's id as a side effect of calling
getClientId() if the component's id isn't set.
-- Blake Sullivan
Blake Sullivan said the following On 1/6/2010 4:20 PM PT:
> 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
>> <bl...@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
>>> <bl...@oracle.com> <bl...@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
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@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
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@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
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>
>>> <ma...@apache.org>wrote:
>>>
>>>
>>>
>>> On Thu, Dec 31, 2009 at 11:12 PM, Blake
>>> Sullivan<bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@oracle.com>
>>> <bl...@oracle.com> <bl...@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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Simon Lessard <si...@gmail.com>.
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 <bl...@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 <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <matzew@apache
> .org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>wrote:
>
>
>
> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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
>
>
>
>
>
>
>
>
>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Blake Sullivan <bl...@oracle.com>.
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 <bl...@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 <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>wrote:
>>
>>
>>
>> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Simon Lessard <si...@gmail.com>.
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 <bl...@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 <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>wrote:
>
>
>
> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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
>
>
>
>
>
>
>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Blake Sullivan <bl...@oracle.com>.
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 <bl...@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 <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>wrote:
>>
>>
>>
>> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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
>>
>>
>>
>>
>>
>>
>>
>>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Simon Lessard <si...@gmail.com>.
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.
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 <bl...@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 <bl...@oracle.com> <bl...@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 <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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 <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>wrote:
>
>
>
> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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
>
>
>
>
>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Blake Sullivan <bl...@oracle.com>.
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 <bl...@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 <bl...@oracle.com> <bl...@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 <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>wrote:
>>
>>
>>
>> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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
>>
>>
>>
>>
>>
>>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Simon Lessard <si...@gmail.com>.
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 <bl...@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 <bl...@oracle.com> <bl...@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 <ma...@apache.org> <ma...@apache.org> <ma...@apache.org> <ma...@apache.org>wrote:
>
>
>
> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@oracle.com> <bl...@oracle.com> <bl...@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
>
>
>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Blake Sullivan <bl...@oracle.com>.
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, from
> http://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 <bl...@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 <ma...@apache.org> <ma...@apache.org>wrote:
>>
>>
>>
>> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@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
>>
>>
>>
>>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Simon Lessard <si...@gmail.com>.
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, from
http://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 <bl...@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 <ma...@apache.org> <ma...@apache.org>wrote:
>
>
>
> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan<bl...@oracle.com> <bl...@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
>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Blake Sullivan <bl...@oracle.com>.
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 <ma...@apache.org>wrote:
>
>
>> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan
>> <bl...@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
>>
>>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Simon Lessard <si...@gmail.com>.
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 <ma...@apache.org>wrote:
> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan
> <bl...@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
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Blake Sullivan <bl...@oracle.com>.
Matthias Wessendorf said the following On 1/5/2010 10:49 AM PT:
> On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan
> <bl...@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?
>
I'd have to look at the code. There is a trivial change that the RI
could make that would make this o(n) in the common case. The trick is
that the matching code shouldn't start over at index 0 each time. It
should start looking at (last index found) + 1 and wrap around if it
can't find a match. In the common case where the same set of children
exist in the same order between the current saved state and the tags,
this would be o(n).
-- Blake
>
>> 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
>>
>>
>>
>>
>
>
>
>
Re: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
Posted by Matthias Wessendorf <ma...@apache.org>.
On Thu, Dec 31, 2009 at 11:12 PM, Blake Sullivan
<bl...@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