MyFaces Core
  1. MyFaces Core
  2. MYFACES-3160

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.8, 2.1.2
    • Component/s: JSR-314
    • Labels:
      None
    1. MYFACES-3160-9.patch
      38 kB
      Leonardo Uribe
    2. MYFACES-3160-8.patch
      37 kB
      Leonardo Uribe
    3. MYFACES-3160-5.patch
      26 kB
      Leonardo Uribe
    4. MYFACES-3160-4.patch
      22 kB
      Leonardo Uribe
    5. MYFACES-3160-3.patch
      18 kB
      Leonardo Uribe
    6. MYFACES-3160-2.patch
      18 kB
      Leonardo Uribe
    7. MYFACES-3160-1.patch
      25 kB
      Leonardo Uribe

      Issue Links

        Activity

        Hide
        Leonardo Uribe added a comment -

        Attache patch (MYFACES-3160-2.patch) for fix case when getMethodExpression or getValueExpression is called with different types and paramTypes.

        Show
        Leonardo Uribe added a comment - Attache patch ( MYFACES-3160 -2.patch) for fix case when getMethodExpression or getValueExpression is called with different types and paramTypes.
        Hide
        Leonardo Uribe added a comment -

        Attached updated patch fix expressions created on composite components

        Show
        Leonardo Uribe added a comment - Attached updated patch fix expressions created on composite components
        Hide
        Leonardo Uribe added a comment -

        Attached patch MYFACES-3160-4.patch with two improvements:

        • Composite component EL caching (because content expressions does not take into account outer VariableMapper variables)
        • Better caching for method expressions (multiple signatures).
        Show
        Leonardo Uribe added a comment - Attached patch MYFACES-3160 -4.patch with two improvements: Composite component EL caching (because content expressions does not take into account outer VariableMapper variables) Better caching for method expressions (multiple signatures).
        Hide
        Leonardo Uribe added a comment -

        Attached patch (MYFACES-3160-5.patch) with solution for cache only expressions that does not require any VariableMapper resolved variables. Great!

        Show
        Leonardo Uribe added a comment - Attached patch ( MYFACES-3160 -5.patch) with solution for cache only expressions that does not require any VariableMapper resolved variables. Great!
        Hide
        Leonardo Uribe added a comment -

        This issue depends on MYFACES-3169. Some conclusions arise in that issue:

        1. facelets user tags cannot cache EL Expressions
        2. There is a potential risk of bad caching if a template is called with different number of parameters.

        The second one makes not possible to set this optimization by default on cases where ui:param is used, because we can't warrant proper operation. Suppose this

        a.xhtml
        <ui:composition template="c.xhtml">
        <ui:param name="var1" value="value1"/>
        </ui:composition>

        b.xhtml
        <ui:composition template="c.xhtml">
        <ui:param name="var1" value="value1"/>
        <ui:param name="var2" value="value2"/>
        </ui:composition>

        c.xhtml
        <ui:composition>
        <h:outputText value="#

        {var1}

        />
        <h:outputText value="#

        {var2}/>
        </ui:composition>

        if a.xhtml view is constructed before b.xhtml, #{var2}

        will be cached, even if this is not wanted and then when b.xhtml is called, the expression will not work correctly.

        In practice, it is up to the app develover to decide if this stuff applies for its particular case. The patch proposed here requires more work after commit the one on MYFACES-3169.

        Show
        Leonardo Uribe added a comment - This issue depends on MYFACES-3169 . Some conclusions arise in that issue: 1. facelets user tags cannot cache EL Expressions 2. There is a potential risk of bad caching if a template is called with different number of parameters. The second one makes not possible to set this optimization by default on cases where ui:param is used, because we can't warrant proper operation. Suppose this a.xhtml <ui:composition template="c.xhtml"> <ui:param name="var1" value="value1"/> </ui:composition> b.xhtml <ui:composition template="c.xhtml"> <ui:param name="var1" value="value1"/> <ui:param name="var2" value="value2"/> </ui:composition> c.xhtml <ui:composition> <h:outputText value="# {var1} /> <h:outputText value="# {var2}/> </ui:composition> if a.xhtml view is constructed before b.xhtml, #{var2} will be cached, even if this is not wanted and then when b.xhtml is called, the expression will not work correctly. In practice, it is up to the app develover to decide if this stuff applies for its particular case. The patch proposed here requires more work after commit the one on MYFACES-3169 .
        Hide
        Leonardo Uribe added a comment -

        Attached corrected patch (MYFACES-3160-8) after commit solution for MYFACES-3169.

        After thinking about this issue, the param to activate the this improvement works like this:

        /**

        • Indicates if expressions generated by facelets should be cached or not. Default is noCache. There there are four modes:
        • <ul>
        • <li>always: Only does not cache when expressions are inside user tags or the expression contains a variable resolved using VariableMapper</li>
        • <li>allowCset: Like always, but does not allow cache when ui:param was used on the current template context</li>
        • <li>strict: Like allowCset, but does not allow cache when c:set with var and value properties only is used on the current page context</li>
        • <li>noCache: All expression are created each time the view is built</li>
        • </ul>
        • */
          @JSFWebConfigParam(since="2.0.8", defaultValue="noCache", expectedValues="noCache, strict, allowCset, always")
          public static final String INIT_PARAM_CACHE_EL_EXPRESSIONS = "org.apache.myfaces.CACHE_EL_EXPRESSIONS";

        With the corrections done on c:set and ui:param, we can limit effectively the "scope" of the variables and detect early if we should cache it or not. The latest evidence shows it is not possible to set the improvement to other value than noCache by default, so the best is let the user decide to cache or not expressions, and the mode that applies for your project.

        Show
        Leonardo Uribe added a comment - Attached corrected patch ( MYFACES-3160 -8) after commit solution for MYFACES-3169 . After thinking about this issue, the param to activate the this improvement works like this: /** Indicates if expressions generated by facelets should be cached or not. Default is noCache. There there are four modes: <ul> <li>always: Only does not cache when expressions are inside user tags or the expression contains a variable resolved using VariableMapper</li> <li>allowCset: Like always, but does not allow cache when ui:param was used on the current template context</li> <li>strict: Like allowCset, but does not allow cache when c:set with var and value properties only is used on the current page context</li> <li>noCache: All expression are created each time the view is built</li> </ul> */ @JSFWebConfigParam(since="2.0.8", defaultValue="noCache", expectedValues="noCache, strict, allowCset, always") public static final String INIT_PARAM_CACHE_EL_EXPRESSIONS = "org.apache.myfaces.CACHE_EL_EXPRESSIONS"; With the corrections done on c:set and ui:param, we can limit effectively the "scope" of the variables and detect early if we should cache it or not. The latest evidence shows it is not possible to set the improvement to other value than noCache by default, so the best is let the user decide to cache or not expressions, and the mode that applies for your project.
        Hide
        Martin Kočí added a comment -

        Cool, thanks for implementing it!
        I'll deploy this with CACHE_EL_EXPRESSIONS=always for our qa-team asap and will do some stress tests too.

        Show
        Martin Kočí added a comment - Cool, thanks for implementing it! I'll deploy this with CACHE_EL_EXPRESSIONS=always for our qa-team asap and will do some stress tests too.

          People

          • Assignee:
            Leonardo Uribe
            Reporter:
            Leonardo Uribe
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development