Hi gentleman,
We are submitting fixes for issue number 223 and fixes for newly
discovered multi-thread concurrency issue in Velocity Engine.
Here is a brief summary of the problem and what we did to fix it.
Issue number 223, Velocity Engine uses excessive amount of memory when a
large number of directives and macros are used.
When a macro or directive is used, they are parsed at run time and the
same macro will be parsed every time it is invoked from another macro.
This results in an explosion of the duplicated string images. We
introduce a string image pool. Before a string image is returned from
VelocityCharStream GetImage method, we simply checks against the string
image pool. If the string image exists in the pool, we will return the
image from the pool. Otherwise we simply return the image itself. We
observe a 30% memory footprint reduction after this.
We obtained additional savings by not adding ASTComment node to the
parse tree, as well as removing unused member variables in SimpleNode.
However, these changes are not part of submitted patch.
Multi-thread concurrency issue
During our concurrency testing, we observed NullPointer exceptions being
thrown when two people hit the same page at the same time for the first
time. Upon further investigation, it turns out that we need to
synchronize the init method on ASTDirective, ASTSetDirective, and render
method on ASTSetDirective, and VelocimacroProxy.
Basically, the problem is introduced as the following; when two threads
attempts to parse and render the same template at the same time. Thread1
finishes parsing first and proceeds to the render method call, while
thread 2 is still busy parsing and will overwrite the existing parse
tree that is being used by thread 1 for rendering purpose. Thus under
certainly condition a NullPointer exception will be thrown.
I have run through all existing test cases successfully and the memory
saving has been verified by YourKit profiler.
Let me know if you have any questions.
Thanks.
-- Lei
Index:
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
===================================================================
---
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
(revision 523820)
+++
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/directive/VelocimacroProxy.java
(working copy)
@@ -55,8 +55,8 @@
private SimpleNode nodeTree = null;
private int numMacroArgs = 0;
private String namespace = "";
-
- private boolean init = false;
+
+ private volatile boolean init = false;
private String[] callingArgs;
private int[] callingArgTypes;
private HashMap proxyArgHash = new HashMap();
@@ -164,10 +164,16 @@
if (nodeTree != null)
{
- if ( !init )
- {
- nodeTree.init( context, rsvc);
- init = true;
+ if(!init)
+ {
+ synchronized(this)
+ {
+ if ( !init )
+ {
+ nodeTree.init( context, rsvc);
+ init = true;
+ }
+ }
}
/*
@@ -398,7 +404,7 @@
for( int i = 1; i < argArray.length; i++)
{
- VMProxyArg arg = new VMProxyArg( rsvc, argArray[i], callArgs[i-1],
callArgTypes[i-1] );
+ VMProxyArg arg = new VMProxyArg( rsvc, argArray[i],
callArgs[i-1], callArgTypes[i-1] );
proxyArgHash.put( argArray[i], arg );
}
}
@@ -461,13 +467,3 @@
return args;
}
}
-
-
-
-
-
-
-
-
-
-
Index:
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/node/ASTDirective.java
===================================================================
---
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/node/ASTDirective.java
(revision 523820)
+++
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/node/ASTDirective.java
(working copy)
@@ -52,7 +52,7 @@
private Directive directive = null;
private String directiveName = "";
private boolean isDirective;
-
+ private volatile boolean isInitialized;
/**
* @param id
*/
@@ -72,7 +72,7 @@
/**
- * @see
org.apache.velocity.runtime.parser.node.SimpleNode#jjtAccept(org.apache.velocity.runtime.parser.node.ParserVisitor,
java.lang.Object)
+ * @see
org.apache.velocity.runtime.parser.node.SimpleNode#jjtAccept(org.apache.velocity.runtime.parser.ParserVisitor,
java.lang.Object)
*/
public Object jjtAccept(ParserVisitor visitor, Object data)
{
@@ -84,9 +84,7 @@
*/
public Object init( InternalContextAdapter context, Object data)
throws TemplateInitException
- {
- super.init( context, data );
-
+ {
/*
* only do things that are not context dependant
*/
@@ -91,7 +89,25 @@
* only do things that are not context dependant
*/
- if (parser.isDirective( directiveName ))
+ if (!isInitialized)
+ {
+ synchronized(this)
+ {
+ if (!isInitialized)
+ {
+ super.init( context, data );
+
+ setup(context);
+ isInitialized = true;
+ }
+ }
+ }
+ return data;
+ }
+
+ private void setup(InternalContextAdapter context)
+ {
+ if (parser.isDirective( directiveName ))
{
isDirective = true;
@@ -150,9 +166,7 @@
{
isDirective = false;
}
-
- return data;
- }
+ }
/**
* @see
org.apache.velocity.runtime.parser.node.SimpleNode#render(org.apache.velocity.context.InternalContextAdapter,
java.io.Writer)
@@ -207,6 +221,4 @@
.toString();
}
-}
-
-
+}
\ No newline at end of file
Index:
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/node/ASTSetDirective.java
===================================================================
---
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/node/ASTSetDirective.java
(revision 523820)
+++
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/node/ASTSetDirective.java
(working copy)
@@ -43,7 +43,8 @@
private Node right = null;
private ASTReference left = null;
boolean logOnNull = false;
-
+ volatile boolean isInitialized = false;
+
/**
* This is really immutable after the init, so keep one for this node
*/
@@ -67,7 +68,7 @@
}
/**
- * @see
org.apache.velocity.runtime.parser.node.SimpleNode#jjtAccept(org.apache.velocity.runtime.parser.node.ParserVisitor,
java.lang.Object)
+ * @see
org.apache.velocity.runtime.parser.node.SimpleNode#jjtAccept(org.apache.velocity.runtime.parser.ParserVisitor,
java.lang.Object)
*/
public Object jjtAccept(ParserVisitor visitor, Object data)
{
@@ -88,9 +89,25 @@
* init the tree correctly
*/
- super.init( context, data );
+ if (!isInitialized)
+ {
+ synchronized(this)
+ {
+ super.init( context, data );
+ if (!isInitialized)
+ {
+ setup(context);
+ isInitialized = true;
+ }
+ }
+ }
- uberInfo = new Info(context.getCurrentTemplateName(),
+ return data;
+ }
+
+ private void setup(InternalContextAdapter context)
+ {
+ uberInfo = new Info(context.getCurrentTemplateName(),
getLine(), getColumn());
right = getRightHandSide();
@@ -102,9 +119,7 @@
* grab this now. No need to redo each time
*/
leftReference = left.getFirstToken().image.substring(1);
-
- return data;
- }
+ }
/**
* puts the value of the RHS into the context under the key of the LHS
@@ -117,6 +132,8 @@
public boolean render( InternalContextAdapter context, Writer writer)
throws IOException, MethodInvocationException
{
+ synchronized(this)
+ {
/*
* get the RHS node, and its value
*/
@@ -139,9 +156,9 @@
{
boolean doit = EventHandlerUtil.shouldLogOnNullSet( rsvc,
context, left.literal(), right.literal() );
- if (doit && log.isInfoEnabled())
+ if (doit && rsvc.getLog().isInfoEnabled())
{
- log.info("RHS of #set statement is null. Context will
not be modified. "
+ rsvc.getLog().info("RHS of #set statement is null.
Context will not be modified. "
+ context.getCurrentTemplateName() + "
[line " + getLine()
+ ", column " + getColumn() + "]");
}
@@ -195,6 +212,7 @@
}
return true;
+ }
}
/**
@@ -216,4 +234,4 @@
{
return jjtGetChild(1);
}
-}
+}
\ No newline at end of file
Index:
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/VelocityCharStream.java
===================================================================
---
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/VelocityCharStream.java
(revision 523820)
+++
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/parser/VelocityCharStream.java
(working copy)
@@ -1,5 +1,7 @@
package org.apache.velocity.runtime.parser;
+import org.apache.velocity.runtime.StringImagePool;
+
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
@@ -401,11 +403,13 @@
*/
public final String GetImage()
{
+ String image = null;
if (bufpos >= tokenBegin)
- return new String(buffer, tokenBegin, bufpos - tokenBegin + 1);
+ image = new String(buffer, tokenBegin, bufpos - tokenBegin + 1);
else
- return new String(buffer, tokenBegin, bufsize - tokenBegin) +
+ image = new String(buffer, tokenBegin, bufsize - tokenBegin) +
new String(buffer, 0, bufpos + 1);
+ return StringImagePool.getImage(image);
}
/**
Index:
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/StringImagePool.java
===================================================================
---
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/StringImagePool.java
(revision 0)
+++
C:/eclipse-projects/Tiger/Velocity-Jakarta/src/java/org/apache/velocity/runtime/StringImagePool.java
(revision 0)
@@ -0,0 +1,27 @@
+package org.apache.velocity.runtime;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class StringImagePool
+{
+ private static Map<String, String> stringImagePool = new
HashMap<String, String>(17);
+
+ public static String getImage(String image)
+ {
+ String reference = stringImagePool.get(image);
+
+ if (reference != null)
+ {
+ return reference;
+ }
+ else
+ {
+ synchronized(stringImagePool)
+ {
+ stringImagePool.put(image, image);
+ }
+ return image;
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
