[core] TagAttributeImpl part II: object allocations (cache ELExpressions)

[Leonardo Uribe <lu...@gmail.com>]

Hi

There is a patch proposed (after many months thinking about it),
according to the discussion on:

http://markmail.org/message/kca64ojdvb6em367?q=%5Bcore%5D+performance:+TagAttributeImpl+part+II:+object+allocations

here:

https://issues.apache.org/jira/browse/MYFACES-3160

The idea is cache ValueExpressions when necessary to reduce
unnecessary object allocations and increase speed, because EL parsing
is also avoided when necessary.

I think it is a nice optimization. The idea is detect when the
VariableMapper has a value and if so, do not cache any expression.
Additionally, use a volatile variable to store expressions.

The patch adds a new web config param called
org.apache.myfaces.CACHE_EL_EXPRESSIONS to enable/disable this
feature.

@Martin Koci: since you was the one proposing this optimization, it
could be good if you can check if it is worth or not (I'm 99% sure, so
any help to reach 100% is welcome!) . If that so, I'll commit the
proposed code.

regards,

Leonardo Uribe

Re: [core] TagAttributeImpl part II: object allocations (cache ELExpressions)

[Leonardo Uribe <lu...@gmail.com>]

Hi

2011/5/31 Martin Koci <ma...@gmail.com>:
> Hi,
>

I have attached an updated patch that has some bug fixes, so it could
be good if you try it.

> thanks for implementing it.  There are some thoughts (about EL
> generally):
>
> 1) Many big views (where creation of expressions costs nonnegligible
> time) use ui:param (or other tag for variable mapping) thus optimization
> there is not possible. It depends on style of programming but I checked
> about 30 "most important" views in our company and variablemapper always
> contains a mapping so no cache is used there.

Yes, I know. Really we can't do too much in such cases. There big
problem is the dependency between ui:param and VariableMapper (in fact
FaceletContext shouldn't extends ELContext!). I have investigated if
it is possible to put something else but at this point there is no a
"perfect match" hack.

The closest solution is do something similar to t:aliasBean hack. In
JSF 2.0 there are these two components:

ui:component : "... This tag is the same as the ui:composition, except
for two things: JSF creates a component and adds it directly to the
tree, and there's no associated template. Use this tag to create a
component and specify a filename for the component as either the
source of a ui:include, or the source of a Facelets tag.  ..."

ui:fragment : "... The fragment tag is identical to the component tag,
except that ui:fragment, unlike ui:component, JSF does not disregard
all content outside of the tag. ..." (sounds like ui:decorate?)

If we take that strictly, that's not true, because ui:component and
ui:fragment does not support ui:param and ui:define, like
ui:composition and ui:decorate does. In fact, those components were
taken directly from facelets 1.1.x without do any questions.

I would like to make this two work as the javadoc says, using some
hack like the one proposed by t:aliasBean. After all, the spec javadoc
says it, and the implementation should do it like the spec says,
right? Note any work in this part should be proposed to be added to a
further spec version, but there is no reason why we can't do it now.

This sounds enough important to dedicate some time to do it and push
it on the Expert Group.

>
> 2) Cache of precompiled expression is resposibility of EL-impl
> (container). JUEL does it and it is good performance improvement; about
> other implementations I'm not sure.
>

In my personal opinion, I think the one who is responsible to cache EL
expressions is the one who knows about the context. So, in this case
myfaces should do it, but the EL implementation should be nice
providing caching to the "static" parts.

>
> 3) summary of problem: for one attribute in xhtml,
> myfaces create :
> * one instance of valueExpression with
> EpressionFactory.createValueExpression
> * one instance of TagValueExpression/TagValueExpressionUEL (wrapper)
> * optionally one instance of LocationValueExpression (wrapper for
> composite component)
>
> * lifespan of those instances is request, so they immediatelly GCed.
> * performance of createValueExpression is unknown and depends on
> implementaion of EL.
>

I think we should take JUEL as reference for testing, because it has
the best code in this part and allows us to see how far can we tune
this stuff, and if it is worth to do it or maybe it is better to let
JUEL cache them.

>
> 4) summary of requirements:
> * reduce number of objects created for one attribute expression (ideally
> to zero)
> * minimize dependency on methods with unknown performance (like
> createValueExpression)
> * preserve functionality of VariableMapping
>

Yes, that's the intention of the patch proposed.

> The main problem for implementation of such requirements is that EL
> spec. does not allow fine grained operations.
>
> For example we cannot provide own implementation is some factory method;
>
> We can fill issue against EL spec and wait for years for fix (like null
> -> 0 coercion problem waits for it's solution).
>
> Or we can cooperate with one EL-implementation to prototype some
> improvements (like how to avoid decorator pattern for expressions).
>

Number 2 sounds good. Anyway, the intention is try everything we can
from MyFaces side.

regards,

Leonardo Uribe

> Regards,
>
> Kočičák
>
> Leonardo Uribe píše v Po 30. 05. 2011 v 16:00 -0500:
>> Hi
>>
>> There is a patch proposed (after many months thinking about it),
>> according to the discussion on:
>>
>> http://markmail.org/message/kca64ojdvb6em367?q=%5Bcore%5D+performance:+TagAttributeImpl+part+II:+object+allocations
>>
>> here:
>>
>> https://issues.apache.org/jira/browse/MYFACES-3160
>>
>> The idea is cache ValueExpressions when necessary to reduce
>> unnecessary object allocations and increase speed, because EL parsing
>> is also avoided when necessary.
>>
>> I think it is a nice optimization. The idea is detect when the
>> VariableMapper has a value and if so, do not cache any expression.
>> Additionally, use a volatile variable to store expressions.
>>
>> The patch adds a new web config param called
>> org.apache.myfaces.CACHE_EL_EXPRESSIONS to enable/disable this
>> feature.
>>
>> @Martin Koci: since you was the one proposing this optimization, it
>> could be good if you can check if it is worth or not (I'm 99% sure, so
>> any help to reach 100% is welcome!) . If that so, I'll commit the
>> proposed code.
>>
>> regards,
>>
>> Leonardo Uribe
>>
>
>
>

Re: [core] TagAttributeImpl part II: object allocations (cache ELExpressions)

[Martin Koci <ma...@gmail.com>]

Hi,

thanks for implementing it.  There are some thoughts (about EL
generally):

1) Many big views (where creation of expressions costs nonnegligible
time) use ui:param (or other tag for variable mapping) thus optimization
there is not possible. It depends on style of programming but I checked
about 30 "most important" views in our company and variablemapper always
contains a mapping so no cache is used there.

2) Cache of precompiled expression is resposibility of EL-impl
(container). JUEL does it and it is good performance improvement; about
other implementations I'm not sure.


3) summary of problem: for one attribute in xhtml, 
myfaces create :
* one instance of valueExpression with
EpressionFactory.createValueExpression
* one instance of TagValueExpression/TagValueExpressionUEL (wrapper)
* optionally one instance of LocationValueExpression (wrapper for
composite component)

* lifespan of those instances is request, so they immediatelly GCed. 
* performance of createValueExpression is unknown and depends on
implementaion of EL. 


4) summary of requirements:
* reduce number of objects created for one attribute expression (ideally
to zero)
* minimize dependency on methods with unknown performance (like
createValueExpression)
* preserve functionality of VariableMapping

The main problem for implementation of such requirements is that EL
spec. does not allow fine grained operations. 

For example we cannot provide own implementation is some factory method;

We can fill issue against EL spec and wait for years for fix (like null
-> 0 coercion problem waits for it's solution). 

Or we can cooperate with one EL-implementation to prototype some
improvements (like how to avoid decorator pattern for expressions).

Regards,

Kočičák

Leonardo Uribe píše v Po 30. 05. 2011 v 16:00 -0500:
> Hi
> 
> There is a patch proposed (after many months thinking about it),
> according to the discussion on:
> 
> http://markmail.org/message/kca64ojdvb6em367?q=%5Bcore%5D+performance:+TagAttributeImpl+part+II:+object+allocations
> 
> here:
> 
> https://issues.apache.org/jira/browse/MYFACES-3160
> 
> The idea is cache ValueExpressions when necessary to reduce
> unnecessary object allocations and increase speed, because EL parsing
> is also avoided when necessary.
> 
> I think it is a nice optimization. The idea is detect when the
> VariableMapper has a value and if so, do not cache any expression.
> Additionally, use a volatile variable to store expressions.
> 
> The patch adds a new web config param called
> org.apache.myfaces.CACHE_EL_EXPRESSIONS to enable/disable this
> feature.
> 
> @Martin Koci: since you was the one proposing this optimization, it
> could be good if you can check if it is worth or not (I'm 99% sure, so
> any help to reach 100% is welcome!) . If that so, I'll commit the
> proposed code.
> 
> regards,
> 
> Leonardo Uribe
>