You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@myfaces.apache.org by Maria Kaval <MA...@oracle.com> on 2010/02/16 18:10:53 UTC
RE: [TRINIDAD][API]TRINIDAD-1668 Speed up UIXComponent.getId()
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
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>