[GLASSFISH-20670] Performance problem in getInitParameter and getInitParameterNames() of org.apache.catalina.core.ApplicationContext (glassfish version), which are called very often by JSF frameworks Created: 28/Jun/13  Updated: 03/Apr/15  Resolved: 13/May/14

Status: Resolved
Project: glassfish
Component/s: performance, web_container
Affects Version/s: 3.1.2.2, 4.0_b89_RC5
Fix Version/s: 4.1_b08

Type: Bug Priority: Major
Reporter: lsxx Assignee: Scott Oaks
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Tags: 4_0_1-review

 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;
}
}



 Comments   
Comment by Shing Wai Chan [ 13/May/14 ]

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

Comment by lsxx [ 08/Aug/14 ]

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.

Comment by Sergiy_Rezvan [ 26/Feb/15 ]

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.

Comment by Yury_Morozov [ 02/Mar/15 ]

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.

Comment by Shing Wai Chan [ 03/Apr/15 ]

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.

Generated at Fri Aug 28 23:30:39 UTC 2015 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.