MyFaces Core
  1. MyFaces Core
  2. MYFACES-3144

[PERF] Cache renderer in UIComponentBase

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.0-SNAPSHOT
    • Fix Version/s: 2.0.13, 2.1.7
    • Component/s: General
    • Labels:
      None
    • Environment:
      myfaces core trunk

      Description

      UIComponentBase uses getRenderer(): Renderer in five methods:
      1) decode
      2) encodeBegin
      3) encodeChildren
      4) encodeEnd
      50 getClientId

      getting the renderer is not cheap if you have thousands component in view.

      Cache renderer instance in UIComponentBase (Trinidad UIXComponentBase does it already)

        Activity

        Hide
        Martin Kočí added a comment -

        Patch - code copied from trinidad.

        Question:

        method _cacheRenderer is called from decode (first method in postback or partial execute) or encodeBegin (first method in non-postback or partial render)

        The second situation does not cover case for getClientId() method and getRendersChildren() - those can be called before encode of component begin.

        Is there any method which is always called as first if component will executed/rendered?

        Show
        Martin Kočí added a comment - Patch - code copied from trinidad. Question: method _cacheRenderer is called from decode (first method in postback or partial execute) or encodeBegin (first method in non-postback or partial render) The second situation does not cover case for getClientId() method and getRendersChildren() - those can be called before encode of component begin. Is there any method which is always called as first if component will executed/rendered?
        Hide
        Jakob Korherr added a comment -

        > Is there any method which is always called as first if component will executed/rendered?

        MyFaces internally yes, but if you use extensions for custom JSF comp-libs it might be different. We should not rely on an invariant like that!

        Show
        Jakob Korherr added a comment - > Is there any method which is always called as first if component will executed/rendered? MyFaces internally yes, but if you use extensions for custom JSF comp-libs it might be different. We should not rely on an invariant like that!
        Hide
        Leonardo Uribe added a comment -

        I think there is one alternative to do this. To retrieve a renderer we need three params:

        1. renderKitId
        2. family
        3. rendererType

        family and rendererType does not change, but renderKitId could change. So, why don't save on a transient field the latest renderKitId used to derive the renderer? Then, when getRenderer() is called we just compare that field against the current one and if are equals we can return the cached instance, otherwise recalculate it.

        But anyway it is questionable if we can gain something with this strategy, because in practice we replace a simple comparison between two strings + an additional field on every component against two lookups over a map. At first view, let the code "as is" looks good, so only if we can gain something it is worth to do it.

        Show
        Leonardo Uribe added a comment - I think there is one alternative to do this. To retrieve a renderer we need three params: 1. renderKitId 2. family 3. rendererType family and rendererType does not change, but renderKitId could change. So, why don't save on a transient field the latest renderKitId used to derive the renderer? Then, when getRenderer() is called we just compare that field against the current one and if are equals we can return the cached instance, otherwise recalculate it. But anyway it is questionable if we can gain something with this strategy, because in practice we replace a simple comparison between two strings + an additional field on every component against two lookups over a map. At first view, let the code "as is" looks good, so only if we can gain something it is worth to do it.
        Hide
        Jakob Korherr added a comment -

        I agree with Leonardo. String operations are usually not as cheap as you would think, and HashMap lookups are usually not as expensive as you would think!

        However, note that normally the renderer of a component does not change at all. In most cases there is one renderer per component and also even only one render-kit: HTML_BASIC (there are really not many JSF apps using more than one renderkit at this time). So it would make sence to really implement the cache without the String comparisons of renderKitId, family and rendererType. Of course, this should only work via a custom init parameter, which is false by default, in order to allow applications with multiple renderers or renderkits to work.

        Show
        Jakob Korherr added a comment - I agree with Leonardo. String operations are usually not as cheap as you would think, and HashMap lookups are usually not as expensive as you would think! However, note that normally the renderer of a component does not change at all. In most cases there is one renderer per component and also even only one render-kit: HTML_BASIC (there are really not many JSF apps using more than one renderkit at this time). So it would make sence to really implement the cache without the String comparisons of renderKitId, family and rendererType. Of course, this should only work via a custom init parameter, which is false by default, in order to allow applications with multiple renderers or renderkits to work.
        Hide
        Martin Kočí added a comment -

        I'm little lost here ... the patch (trinidad code) does not use String comparisions. Is that patch applicable? Trinidad is well tested (and fast).

        The performance problem is in repeated HashMap lookups. Yes, it's only minor (compared with reflexion for example). But in large view it can save thousands such lookups - minor improvement, but still improvement

        Show
        Martin Kočí added a comment - I'm little lost here ... the patch (trinidad code) does not use String comparisions. Is that patch applicable? Trinidad is well tested (and fast). The performance problem is in repeated HashMap lookups. Yes, it's only minor (compared with reflexion for example). But in large view it can save thousands such lookups - minor improvement, but still improvement
        Hide
        Leonardo Uribe added a comment -

        Thinking about it, I found an alternative solution for this problem. The idea is create two vars in UIComponentBase:

        private transient Boolean _cachedIsRendered;
        private transient Renderer _cachedRenderer;

        copy the code from UIComponent.encodeAll() to UIComponentBase. Then, do the same trick for cache FacesContext, but on encodeAll(). The idea is set _cachedRendered and _cachedIsRendered at the beginning and then clear this variables before end the method.

        Then, some code is added in isRendered() and getRenderer() to check for the cached var and if is set, return that value. In this way we have control about the cached var. If by some reason encodeXXX() is called instead encodeAll(), the code will work as expected. The same thing can be done in decode() method, but in this case only for _cachedRendered, preventing a second call when getClientId() is called.

        In this way we can save between 6 and 8 lookups to a map. isRendered() is called only once per component, which is valid because this value does not change while encoding of the same component occur. Also, it preserves getRenderer() plugability, because it is possible to change the renderKit before render response time and reflect the change.

        Show
        Leonardo Uribe added a comment - Thinking about it, I found an alternative solution for this problem. The idea is create two vars in UIComponentBase: private transient Boolean _cachedIsRendered; private transient Renderer _cachedRenderer; copy the code from UIComponent.encodeAll() to UIComponentBase. Then, do the same trick for cache FacesContext, but on encodeAll(). The idea is set _cachedRendered and _cachedIsRendered at the beginning and then clear this variables before end the method. Then, some code is added in isRendered() and getRenderer() to check for the cached var and if is set, return that value. In this way we have control about the cached var. If by some reason encodeXXX() is called instead encodeAll(), the code will work as expected. The same thing can be done in decode() method, but in this case only for _cachedRendered, preventing a second call when getClientId() is called. In this way we can save between 6 and 8 lookups to a map. isRendered() is called only once per component, which is valid because this value does not change while encoding of the same component occur. Also, it preserves getRenderer() plugability, because it is possible to change the renderKit before render response time and reflect the change.

          People

          • Assignee:
            Leonardo Uribe
            Reporter:
            Martin Kočí
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development