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