glassfish
  1. glassfish
  2. GLASSFISH-20670

Performance problem in getInitParameter and getInitParameterNames() of org.apache.catalina.core.ApplicationContext (glassfish version), which are called very often by JSF frameworks

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.1.2.2, 4.0_dev
    • Fix Version/s: 4.1_dev
    • Component/s: performance, web_container
    • Labels:
      None

      Description

      During performance tests (multiple parallel threads) of our enterprise application we discovered that 62% of time is spent in org.apache.catalina.core.ApplicationContext.mergeParameters.

          • Background:
            The calls to this method have origin in JSF stack - there are calls to context.getExternalContext().getInitParameter() from
            javax.faces.component.UIComponent.pushComponentToEL
            javax.faces.component.UIComponent.popComponentFromEL
            In default JSF implementation provided with glassfish the value for UIComponent.HONOR_CURRENT_COMPONENT_ATTRIBUTES_PARAM_NAME is obtained (without caching) for each call withing these methods.
          • Source of problem:
            The mergeParameters, which is internally called from getInitParameter and getInitParameterNames(), is synchronized.
            I analysed source of this class and believe that synchronization in this area could be improved.
            The problem was discovered during tests with glassfish 3.1.1.2, but it seems that other versions including trunk are affected as well.
          • Suggested solution:
            The problem seems to be easy to fix.

      This is the original body for mergeParameters:
      private synchronized void mergeParameters() {
      if (parametersMerged)

      { return; }

      [...]

      parametersMerged = true;
      }

      As:
      - this is the only synchronized method and there no "synchronized (this)" in this class
      - the field parametersMerged is assigned a value only in mergeParameters (last line) and at initialization (private volatile boolean parametersMerged = false
      - after the first execution of mergeParameters the field parametersMerged will never be assigned any value (especially false) and other calls to mergeParameters are waste of time.
      - the field parametersMerged is volatile
      the problem could be successfully solved by moving the synchronization to inside of the method and double checking the initial condition:

      private void mergeParameters() {
      if (parametersMerged) { return; }

      synchronized(this){
      if (parametersMerged)

      { return; }

      [...]

      parametersMerged = true;
      }
      }

        Activity

        Hide
        Shing Wai Chan added a comment -

        Tomcat 7 has a different fix on this issue by moving the merging parameters logic into the beginning of the life cycle.
        Sending org/apache/catalina/core/ApplicationContext.java
        Sending org/apache/catalina/core/StandardContext.java
        Transmitting file data ..
        Committed revision 63827.

        Show
        Shing Wai Chan added a comment - Tomcat 7 has a different fix on this issue by moving the merging parameters logic into the beginning of the life cycle. Sending org/apache/catalina/core/ApplicationContext.java Sending org/apache/catalina/core/StandardContext.java Transmitting file data .. Committed revision 63827.
        Hide
        Yury_Morozov added a comment - - edited

        The bug hasn't been fixed in any version, the method mergeParameters is still synchronized and blocks all threads.
        Solution above fixes the problem. Please, include this in your next release.

        Show
        Yury_Morozov added a comment - - edited The bug hasn't been fixed in any version, the method mergeParameters is still synchronized and blocks all threads. Solution above fixes the problem. Please, include this in your next release.
        Hide
        Sergiy_Rezvan added a comment -

        I have had the same problem. This bug can be fixed by using the double-checked locking pattern.
        It looks like:
        org.apache.catalina.ApplicationContext

        public class ApplicationContext implements ServletContext {
        ....
        private volatile Object lockObject = new Object();
        ....
        private void mergeParameters()
        {
        if (!this.parametersMerged) {
        synchronized (this.lockObject) {
        if (!this.parametersMerged) {
        for (String name : this.context.findParameters())

        { this.parameters.put(name, this.context.findParameter(name)); }

        List<ApplicationParameter> params = this.context.findApplicationParameters();
        Iterator<ApplicationParameter> i = params.iterator();
        while (i.hasNext())
        {
        ApplicationParameter param = (ApplicationParameter)i.next();
        if (param.getOverride())
        {
        if (this.parameters.get(param.getName()) == null)

        { this.parameters.put(param.getName(), param.getValue()); }

        }
        else

        { this.parameters.put(param.getName(), param.getValue()); }

        }
        this.parametersMerged = true;
        }
        }
        }
        }

        Could you please check this approach and give me feedback that everything is okay?
        I have tested it and now I don't have this problem, but I want to be sure that it is correct.

        Show
        Sergiy_Rezvan added a comment - I have had the same problem. This bug can be fixed by using the double-checked locking pattern. It looks like: org.apache.catalina.ApplicationContext public class ApplicationContext implements ServletContext { .... private volatile Object lockObject = new Object(); .... private void mergeParameters() { if (!this.parametersMerged) { synchronized (this.lockObject) { if (!this.parametersMerged) { for (String name : this.context.findParameters()) { this.parameters.put(name, this.context.findParameter(name)); } List<ApplicationParameter> params = this.context.findApplicationParameters(); Iterator<ApplicationParameter> i = params.iterator(); while (i.hasNext()) { ApplicationParameter param = (ApplicationParameter)i.next(); if (param.getOverride()) { if (this.parameters.get(param.getName()) == null) { this.parameters.put(param.getName(), param.getValue()); } } else { this.parameters.put(param.getName(), param.getValue()); } } this.parametersMerged = true; } } } } Could you please check this approach and give me feedback that everything is okay? I have tested it and now I don't have this problem, but I want to be sure that it is correct.
        Hide
        lsxx added a comment -

        The bug is not fixed - the introduced change has no effect as long the method mergeParameters is still synchronized.
        Adding "sychnronized(this)"-block within synchronized method has no effect.

        Show
        lsxx added a comment - The bug is not fixed - the introduced change has no effect as long the method mergeParameters is still synchronized. Adding "sychnronized(this)"-block within synchronized method has no effect.
        Hide
        Shing Wai Chan added a comment -

        Sending src/main/java/org/apache/catalina/core/ApplicationContext.java
        Transmitting file data .
        Committed revision 63266.

        Show
        Shing Wai Chan added a comment - Sending src/main/java/org/apache/catalina/core/ApplicationContext.java Transmitting file data . Committed revision 63266.

          People

          • Assignee:
            Scott Oaks
            Reporter:
            lsxx
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: