Uploaded image for project: 'MyFaces Core'
  1. MyFaces Core
  2. MYFACES-3586

Performance improvement in Resource loading - HIGH CPU inflating bytes in ResourceHandlerImpl.handleResourceRequest

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.1.11
    • Component/s: None
    • Labels:
      None
    • Environment:
      ALL

      Description

      In a high concurrency load test with Apache MyFaces + RichFaces component library we found that under peak load majority of our web container threads were stuck in this call stack

      at java/util/zip/Inflater.inflateBytes(Native Method)
      at java/util/zip/Inflater.inflate(Inflater.java:249(Compiled Code))
      at java/util/zip/InflaterInputStream.read(InflaterInputStream.java:146(Compiled Code))
      at java/util/zip/InflaterInputStream.read(InflaterInputStream.java:116(Compiled Code))
      at java/io/FilterInputStream.read(FilterInputStream.java:77(Compiled Code))
      at java/io/FilterInputStream.read(FilterInputStream.java:77(Compiled Code))
      at java/io/PushbackInputStream.read(PushbackInputStream.java:133(Compiled Code))
      at org/apache/myfaces/shared_impl/resource/ResourceImpl$ValueExpressionFilterInputStream.read(ResourceImpl.java:117(Compiled Code))
      at java/io/InputStream.read(InputStream.java:175(Compiled Code))
      at java/io/InputStream.read(InputStream.java:97(Compiled Code))
      at org/apache/myfaces/application/ResourceHandlerImpl.pipeBytes(ResourceHandlerImpl.java:402(Compiled Code))
      at org/apache/myfaces/application/ResourceHandlerImpl.handleResourceRequest(ResourceHandlerImpl.java:357(Compiled Code))
      at org/richfaces/resource/ResourceHandlerImpl.handleResourceRequest(ResourceHandlerImpl.java:257(Compiled Code))
      at javax/faces/webapp/FacesServlet.service(FacesServlet.java:183(Compiled Code))
      at org/richfaces/webapp/ResourceServlet.httpService(ResourceServlet.java:110(Compiled Code))
      at org/richfaces/webapp/ResourceServlet.service(ResourceServlet.java:105(Compiled Code))

      After looking at the src code of org.apache.myfaces.application.ResourceHandlerImpl.handleResourceRequest(FacesContext) I can see that the ResourceHandlerCache caches the Resource metadata ; however the actual output of the resource which is written to the outputstream in ResourceHandlerImpl.pipeBytes is NEVER cached.

      This causes a scalability problem in our case because each access to the resource involves cracking open a jar, inflating the bytes and parsing a stream of bytes. This is done multiple times for the same resource(say a css file) inside the richfaces jar inspite of the resource not changing.

      I propose a much needed performance optimization wherein we cache the output of the Resource handled and stash the output byte[] it as softReference in the ResourceHandlerCache.ResourceValue.

      1. MYFACES-3586-1.patch
        18 kB
        Leonardo Uribe
      2. MYFACES-3586.patch
        6 kB
        Rohit Dilip Kelapure

        Activity

        Hide
        kelapure Rohit Dilip Kelapure added a comment -

        Paul Nicolucci will provide a patch for this issue.

        Show
        kelapure Rohit Dilip Kelapure added a comment - Paul Nicolucci will provide a patch for this issue.
        Hide
        lu4242 Leonardo Uribe added a comment -

        I see. So, in this scenario, Resource.getInputStream() becomes an expensive operation, and we cannot get rid of it, because in this case the css file is inside a jar file. I think it has sense to cache that information, because after all it is static (once the application starts it never change until the app is restarted). I'll keep an eye on this improvement.

        Show
        lu4242 Leonardo Uribe added a comment - I see. So, in this scenario, Resource.getInputStream() becomes an expensive operation, and we cannot get rid of it, because in this case the css file is inside a jar file. I think it has sense to cache that information, because after all it is static (once the application starts it never change until the app is restarted). I'll keep an eye on this improvement.
        Hide
        kelapure Rohit Dilip Kelapure added a comment - - edited

        Increase scalability and performance of JSF Resource handling code by caching the output of static resources.
        Patch has been built on the 2.0.x trunk stream.

        Show
        kelapure Rohit Dilip Kelapure added a comment - - edited Increase scalability and performance of JSF Resource handling code by caching the output of static resources. Patch has been built on the 2.0.x trunk stream.
        Hide
        lu4242 Leonardo Uribe added a comment -

        I have been thinking about it and this requires a discussion on dev list, so I'll make my comments there.

        Show
        lu4242 Leonardo Uribe added a comment - I have been thinking about it and this requires a discussion on dev list, so I'll make my comments there.
        Hide
        kito99 Kito D. Mann added a comment -

        Leonardo, I don't see a discussion about this on the list. What is your concern?

        Show
        kito99 Kito D. Mann added a comment - Leonardo, I don't see a discussion about this on the list. What is your concern?
        Hide
        lu4242 Leonardo Uribe added a comment -

        See http://markmail.org/message/xycbf77ku7x5wygj#query:+page:1+mid:jhmcr6a435xma3lu+state:results

        Basically, we have discussed before these kind of improvements over ResourceHandler, and some people in the community has rejected the improvements, because there are other alternatives that are supposed to be better that what we can ever do in this part.

        Show
        lu4242 Leonardo Uribe added a comment - See http://markmail.org/message/xycbf77ku7x5wygj#query:+page:1+mid:jhmcr6a435xma3lu+state:results Basically, we have discussed before these kind of improvements over ResourceHandler, and some people in the community has rejected the improvements, because there are other alternatives that are supposed to be better that what we can ever do in this part.
        Hide
        eag EAG added a comment - - edited

        Hi, I am also facing the same issue with myfaces 2.1. I am using Primefaces for my web application. Run a test for 500 users on 4core system. The CPU chocked to 100% when concurreny reached 300 users. The following exception is coming:

        org.apache.myfaces.application.ResourceHandlerImpl handleResourceRequest. SEVERE: Error trying to load resource primefaces.css with library primefaces :Broken pipe (errno:32)

        The resource handler definitely needs to be fixed. Otherwise me or the others using myfaces 2.x will not able be to scale the application. Please provide myfaces jar with upgraded resource handler.

        Also let me know a way to integrate this available patch so that in future i can upgrade myfaces with ease.

        Show
        eag EAG added a comment - - edited Hi, I am also facing the same issue with myfaces 2.1. I am using Primefaces for my web application. Run a test for 500 users on 4core system. The CPU chocked to 100% when concurreny reached 300 users. The following exception is coming: org.apache.myfaces.application.ResourceHandlerImpl handleResourceRequest. SEVERE: Error trying to load resource primefaces.css with library primefaces :Broken pipe (errno:32) The resource handler definitely needs to be fixed. Otherwise me or the others using myfaces 2.x will not able be to scale the application. Please provide myfaces jar with upgraded resource handler. Also let me know a way to integrate this available patch so that in future i can upgrade myfaces with ease.
        Hide
        lu4242 Leonardo Uribe added a comment -

        I have attached a patch ( MYFACES-3586-1.patch ) using a web config param to enable disable it. It only use a temporal folder to store the files, but does not apply the solution using a memory cache, which still sounds overkill because and the end it will be just a bad copy of the module in apache server. I think it is better to implement the memory cache only if more information is provided about the real benefit of it.

        Show
        lu4242 Leonardo Uribe added a comment - I have attached a patch ( MYFACES-3586 -1.patch ) using a web config param to enable disable it. It only use a temporal folder to store the files, but does not apply the solution using a memory cache, which still sounds overkill because and the end it will be just a bad copy of the module in apache server. I think it is better to implement the memory cache only if more information is provided about the real benefit of it.
        Hide
        kito99 Kito D. Mann added a comment -

        Are there any plans to add the patch to the trunk as an optional improvement? If not, which version(s) is the patch compatibile with?

        Show
        kito99 Kito D. Mann added a comment - Are there any plans to add the patch to the trunk as an optional improvement? If not, which version(s) is the patch compatibile with?
        Hide
        lu4242 Leonardo Uribe added a comment -

        I would say the patch is ok to commit it, but the solution using a memory cache still doesn't sound too convincing. The reason is this looks just like replicate the same work done in apache server, so if you really need something like that, do the logic in apache server.

        Anyway, if there is no objections, I can commit the code and include it in the next release. The patch suppose include a web config param:

        org.apache.myfaces.TEMPORAL_RESOURCEHANDLER_CACHE_ENABLED

        by default false to enable/disable it.

        In theory the patch "as is" is ok and becomes useful, but I would like to hear other opinions, to see how far is really necessary to go.

        Show
        lu4242 Leonardo Uribe added a comment - I would say the patch is ok to commit it, but the solution using a memory cache still doesn't sound too convincing. The reason is this looks just like replicate the same work done in apache server, so if you really need something like that, do the logic in apache server. Anyway, if there is no objections, I can commit the code and include it in the next release. The patch suppose include a web config param: org.apache.myfaces.TEMPORAL_RESOURCEHANDLER_CACHE_ENABLED by default false to enable/disable it. In theory the patch "as is" is ok and becomes useful, but I would like to hear other opinions, to see how far is really necessary to go.
        Hide
        lu4242 Leonardo Uribe added a comment -

        Closing this issue as fixed, since there is no objections and instead the is interest to get this one done. It was only committed the fix using a temporal folder (no mem cache).

        Show
        lu4242 Leonardo Uribe added a comment - Closing this issue as fixed, since there is no objections and instead the is interest to get this one done. It was only committed the fix using a temporal folder (no mem cache).

          People

          • Assignee:
            lu4242 Leonardo Uribe
            Reporter:
            kelapure Rohit Dilip Kelapure
          • Votes:
            2 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 48h
              48h
              Remaining:
              Remaining Estimate - 48h
              48h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development