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-1.patch
      25 kB
      Leonardo Uribe
    2. MYFACES-3160-2.patch
      18 kB
      Leonardo Uribe
    3. MYFACES-3160-3.patch
      18 kB
      Leonardo Uribe
    4. MYFACES-3160-4.patch
      22 kB
      Leonardo Uribe
    5. MYFACES-3160-5.patch
      26 kB
      Leonardo Uribe
    6. MYFACES-3160-8.patch
      37 kB
      Leonardo Uribe
    7. MYFACES-3160-9.patch
      38 kB
      Leonardo Uribe

      Issue Links

        Activity

        Leonardo Uribe created issue -
        Leonardo Uribe made changes -
        Field Original Value New Value
        Attachment MYFACES-3160-1.patch [ 12480873 ]
        Leonardo Uribe made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Leonardo Uribe made changes -
        Status Patch Available [ 10002 ] In Progress [ 3 ]
        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.
        Leonardo Uribe made changes -
        Attachment MYFACES-3160-2.patch [ 12480976 ]
        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
        Leonardo Uribe made changes -
        Attachment MYFACES-3160-3.patch [ 12480988 ]
        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).
        Leonardo Uribe made changes -
        Attachment MYFACES-3160-4.patch [ 12481026 ]
        Leonardo Uribe made changes -
        Attachment MYFACES-3160-5.patch [ 12481310 ]
        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!
        Leonardo Uribe made changes -
        Link This issue depends on MYFACES-3169 [ MYFACES-3169 ]
        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 .
        Leonardo Uribe made changes -
        Attachment MYFACES-3160-8.patch [ 12482598 ]
        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.
        Leonardo Uribe made changes -
        Attachment MYFACES-3160-9.patch [ 12482833 ]
        Leonardo Uribe made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Fix Version/s 2.0.8 [ 12316514 ]
        Fix Version/s 2.1.2 [ 12316512 ]
        Resolution Fixed [ 1 ]
        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.
        Leonardo Uribe made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Gavin made changes -
        Link This issue depends on MYFACES-3169 [ MYFACES-3169 ]
        Gavin made changes -
        Link This issue depends upon MYFACES-3169 [ MYFACES-3169 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Patch Available Patch Available
        1m 18s 1 Leonardo Uribe 30/May/11 21:48
        Patch Available Patch Available In Progress In Progress
        10s 1 Leonardo Uribe 30/May/11 21:49
        In Progress In Progress Resolved Resolved
        16d 21h 12m 1 Leonardo Uribe 16/Jun/11 19:01
        Resolved Resolved Closed Closed
        67d 8h 34m 1 Leonardo Uribe 23/Aug/11 03:36

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development