commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brett Porter <br...@apache.org>
Subject Re: cvs commit: jakarta-commons/jelly/src/java/org/apache/commons/jelly/util TagUtils.java
Date Tue, 28 Dec 2004 01:48:30 GMT
The test is failing for me. Is that to be expected?

polx@apache.org wrote:

>polx        2004/12/27 17:39:23
>
>  Modified:    jelly/src/java/org/apache/commons/jelly/impl
>                        StaticTagScript.java TagScript.java
>               jelly/src/java/org/apache/commons/jelly JellyContext.java
>               jelly/jelly-tags/xml/src/test/org/apache/commons/jelly/tags/xml
>                        TestParser.java
>               jelly/jelly-tags/xml/src/java/org/apache/commons/jelly/tags/xml
>                        TransformTag.java
>               jelly/src/java/org/apache/commons/jelly/util TagUtils.java
>  Added:       jelly/src/test/org/apache/commons/jelly/core
>                        TestCoreMemoryLeak.java BaseMemoryLeakTest.java
>  Removed:     jelly/src/java/org/apache/commons/jelly/impl
>                        WeakReferenceWrapperScript.java
>  Log:
>  Tag caching is now done in the JellyContext.
>  The TestCoreMemoryLeak shows that, at least with some usage of JellyContext.clear(),
one avoids leaks.
>  Removing Tag-caching flag. Also, removing the WeakReferenceWrapperScript, both were
in the era of ThreadLocal which is to be finished.
>  
>  Have tested these with an amount of the tag-libs... had to change the XML one but otherwise
apparently none.
>  
>  Feedback and testing eagerly welcome!
>  paul
>  
>  Revision  Changes    Path
>  1.25      +2 -2      jakarta-commons/jelly/src/java/org/apache/commons/jelly/impl/StaticTagScript.java
>  
>  Index: StaticTagScript.java
>  ===================================================================
>  RCS file: /home/cvs/jakarta-commons/jelly/src/java/org/apache/commons/jelly/impl/StaticTagScript.java,v
>  retrieving revision 1.24
>  retrieving revision 1.25
>  diff -u -r1.24 -r1.25
>  --- StaticTagScript.java	16 Sep 2004 01:12:11 -0000	1.24
>  +++ StaticTagScript.java	28 Dec 2004 01:39:22 -0000	1.25
>  @@ -59,14 +59,14 @@
>   
>           Tag tag = null;
>           try {
>  -            tag = getTag();
>  +            tag = getTag(context);
>   
>               // lets see if we have a dynamic tag
>               if (tag instanceof StaticTag) {
>                   tag = findDynamicTag(context, (StaticTag) tag);
>               }
>   
>  -            setTag(tag);
>  +            setTag(tag,context);
>           } catch (JellyException e) {
>               throw new JellyTagException(e);
>           }
>  
>  
>  
>  1.47      +12 -31    jakarta-commons/jelly/src/java/org/apache/commons/jelly/impl/TagScript.java
>  
>  Index: TagScript.java
>  ===================================================================
>  RCS file: /home/cvs/jakarta-commons/jelly/src/java/org/apache/commons/jelly/impl/TagScript.java,v
>  retrieving revision 1.46
>  retrieving revision 1.47
>  diff -u -r1.46 -r1.47
>  --- TagScript.java	27 Oct 2004 21:50:53 -0000	1.46
>  +++ TagScript.java	28 Dec 2004 01:39:22 -0000	1.47
>  @@ -61,15 +61,6 @@
>       /** The Log to which logging calls will be made. */
>       private static final Log log = LogFactory.getLog(TagScript.class);
>   
>  -    /**
>  -     * Thread local storage for the tag used by the current thread.
>  -     * This allows us to pool tag instances, per thread to reduce object construction
>  -     * over head, if we need it.
>  -     *
>  -     * Note that we could use the stack and create a new tag for each invocation
>  -     * if we made a slight change to the Script API to pass in the parent tag.
>  -     */
>  -    private ThreadLocal tagHolder = new ThreadLocal();
>   
>       /** The attribute expressions that are created */
>       protected Map attributes = new Hashtable();
>  @@ -194,11 +185,8 @@
>       public void run(JellyContext context, XMLOutput output) throws JellyTagException
{
>           URL rootURL = context.getRootURL();
>           URL currentURL = context.getCurrentURL();
>  -        if ( ! context.isCacheTags() ) {
>  -            clearTag();
>  -        }
>           try {
>  -            Tag tag = getTag();
>  +            Tag tag = getTag(context);
>               if ( tag == null ) {
>                   return;
>               }
>  @@ -295,15 +283,15 @@
>       /**
>        * @return the tag to be evaluated, creating it lazily if required.
>        */
>  -    public Tag getTag() throws JellyException {
>  -        Tag tag = (Tag) tagHolder.get();
>  +    public Tag getTag(JellyContext context) throws JellyException {
>  +        Tag tag = context.getTagOfTagScript(this);
>           if ( tag == null ) {
>               tag = createTag();
>               if ( tag != null ) {
>  -                tagHolder.set(tag);
>  +                context.setTagForScript(this,tag);
>               }
>           }
>  -        configureTag(tag);
>  +        configureTag(tag,context);
>           return tag;
>       }
>   
>  @@ -494,21 +482,20 @@
>           return null;
>       }
>   
>  -
>  +	
>       /**
>        * Compiles a newly created tag if required, sets its parent and body.
>        */
>  -    protected void configureTag(Tag tag) throws JellyException {
>  +    protected void configureTag(Tag tag, JellyContext context) throws JellyException
{
>           if (tag instanceof CompilableTag) {
>               ((CompilableTag) tag).compile();
>           }
>           Tag parentTag = null;
>           if ( parent != null ) {
>  -            parentTag = parent.getTag();
>  +            parentTag = parent.getTag(context);
>           }
>           tag.setParent( parentTag );
>  -        tag.setBody( new WeakReferenceWrapperScript(tagBody) );
>  -        //tag.setBody( tagBody );
>  +        tag.setBody( tagBody );
>   
>           if (tag instanceof NamespaceAwareTag) {
>               NamespaceAwareTag naTag = (NamespaceAwareTag) tag;
>  @@ -519,19 +506,13 @@
>           }
>       }
>   
>  -    /**
>  -     * Flushes the current cached tag so that it will be created, lazily, next invocation
>  -     */
>  -    protected void clearTag() {
>  -        tagHolder.set(null);
>  -    }
>   
>       /**
>        * Allows the script to set the tag instance to be used, such as in a StaticTagScript
>        * when a StaticTag is switched with a DynamicTag
>        */
>  -    protected void setTag(Tag tag) {
>  -        tagHolder.set(tag);
>  +    protected void setTag(Tag tag, JellyContext context) {
>  +        context.setTagForScript(this,tag);
>       }
>   
>       /**
>  
>  
>  
>  1.62      +39 -27    jakarta-commons/jelly/src/java/org/apache/commons/jelly/JellyContext.java
>  
>  Index: JellyContext.java
>  ===================================================================
>  RCS file: /home/cvs/jakarta-commons/jelly/src/java/org/apache/commons/jelly/JellyContext.java,v
>  retrieving revision 1.61
>  retrieving revision 1.62
>  diff -u -r1.61 -r1.62
>  --- JellyContext.java	9 Sep 2004 15:10:03 -0000	1.61
>  +++ JellyContext.java	28 Dec 2004 01:39:23 -0000	1.62
>  @@ -25,6 +25,7 @@
>   import java.util.Map;
>   
>   import org.apache.commons.jelly.parser.XMLParser;
>  +import org.apache.commons.jelly.impl.TagScript;
>   import org.apache.commons.jelly.util.ClassLoaderUtils;
>   import org.apache.commons.logging.Log;
>   import org.apache.commons.logging.LogFactory;
>  @@ -90,8 +91,15 @@
>       /** Should we export tag libraries to our parents context */
>       private boolean exportLibraries = true;
>   
>  -    /** Should we cache Tag instances, per thread, to reduce object contruction overhead?
*/
>  -    private boolean cacheTags = false;
>  +    /** A map that associates TagScript objects to Tag objects.
>  +      * Is made a WeakHashMap so that the storage is discarded if the 
>  +      * Script is discarded.
>  +      */
>  +    private Map tagHolderMap = new java.util.WeakHashMap(5);
>  +    // THINKME: Script objects are like Object (for equals and hashCode) I think.
>  +    //          It should be asy to optimize hash-map distribution, e.g. by
>  +    //          shifting the hashcode return value (presuming Object.hashcode()
>  +    //          is something like an address)
>   
>       /**
>        * Create a new context with the currentURL set to the rootURL
>  @@ -133,7 +141,6 @@
>           this.rootURL = parent.rootURL;
>           this.currentURL = parent.currentURL;
>           this.variables.put("parentScope", parent.variables);
>  -        this.cacheTags = parent.cacheTags;
>           init();
>       }
>   
>  @@ -377,7 +384,36 @@
>       public JellyContext newJellyContext() {
>           return createChildContext();
>       }
>  +    
>   
>  +    /** @return the tag associated to the given TagScript for the current run.
>  +      */
>  +    public Tag getTagOfTagScript(TagScript script) {
>  +        if( script == null )
>  +            return null;
>  +        return (Tag) tagHolderMap.get(script);
>  +    }
>  +	
>  +	/** @return the Map that associates the the Tags to Scripts */
>  +	public Map getTagHolderMap() {
>  +		return tagHolderMap;
>  +	}
>  +    
>  +    /** Associates, for the run of this context, the tag of this TagScript.
>  +      */
>  +    public void setTagForScript(TagScript script, Tag tag) {
>  +        tagHolderMap.put(script,tag);
>  +    }
>  +    
>  +    /** Clears both the script-to-tags association and the variables association.
>  +      */
>  +    public void clear() {
>  +        tagHolderMap.clear();
>  +        variables.clear();
>  +    }
>  +    
>  +    
>  +    
>       /** Registers the given tag library against the given namespace URI.
>        * This should be called before the parser is used.
>        */
>  @@ -787,30 +823,6 @@
>        */
>       public void setCurrentURL(URL currentURL) {
>           this.currentURL = currentURL;
>  -    }
>  -
>  -    /**
>  -     * Returns whether caching of Tag instances, per thread, is enabled.
>  -     * Caching Tags can boost performance, on some JVMs, by reducing the cost of
>  -     * object construction when running Jelly inside a multi-threaded application server
>  -     * such as a Servlet engine.
>  -     *
>  -     * @return whether caching of Tag instances is enabled.
>  -     */
>  -    public boolean isCacheTags() {
>  -        return cacheTags;
>  -    }
>  -
>  -    /**
>  -     * Sets whether caching of Tag instances, per thread, is enabled.
>  -     * Caching Tags can boost performance, on some JVMs, by reducing the cost of
>  -     * object construction when running Jelly inside a multi-threaded application server
>  -     * such as a Servlet engine.
>  -     *
>  -     * @param cacheTags Whether caching should be enabled or disabled.
>  -     */
>  -    public void setCacheTags(boolean cacheTags) {
>  -        this.cacheTags = cacheTags;
>       }
>   
>       /**
>  
>  
>  
>  1.6       +9 -6      jakarta-commons/jelly/jelly-tags/xml/src/test/org/apache/commons/jelly/tags/xml/TestParser.java
>  
>  Index: TestParser.java
>  ===================================================================
>  RCS file: /home/cvs/jakarta-commons/jelly/jelly-tags/xml/src/test/org/apache/commons/jelly/tags/xml/TestParser.java,v
>  retrieving revision 1.5
>  retrieving revision 1.6
>  diff -u -r1.5 -r1.6
>  --- TestParser.java	9 Sep 2004 12:24:41 -0000	1.5
>  +++ TestParser.java	28 Dec 2004 01:39:23 -0000	1.6
>  @@ -26,6 +26,7 @@
>   
>   import org.apache.commons.jelly.Script;
>   import org.apache.commons.jelly.Tag;
>  +import org.apache.commons.jelly.JellyContext;
>   import org.apache.commons.jelly.impl.ScriptBlock;
>   import org.apache.commons.jelly.impl.TagScript;
>   import org.apache.commons.jelly.parser.XMLParser;
>  @@ -66,26 +67,28 @@
>   
>           log.debug("Found: " + script);
>   
>  -        assertTagsHaveParent( script, null );
>  +        assertTagsHaveParent( script, null, null );
>       }
>   
>       /**
>        * Tests that the Tag in the TagScript has the given parent and then
>        * recurse to check its children has the correct parent and so forth.
>        */
>  -    protected void assertTagsHaveParent(Script script, Tag parent) throws Exception
{
>  +    protected void assertTagsHaveParent(Script script, Tag parent, JellyContext context)
throws Exception {
>  +        if ( context == null )
>  +            context = new JellyContext();
>           if ( script instanceof TagScript ) {
>               TagScript tagScript = (TagScript) script;
>  -            Tag tag = tagScript.getTag();
>  +            Tag tag = tagScript.getTag(context);
>   
>               assertEquals( "Tag: " + tag + " has the incorrect parent", parent, tag.getParent()
);
>   
>  -            assertTagsHaveParent( tag.getBody(), tag );
>  +            assertTagsHaveParent( tag.getBody(), tag, context );
>           }
>           else if ( script instanceof ScriptBlock ) {
>               ScriptBlock block = (ScriptBlock) script;
>               for ( Iterator iter = block.getScriptList().iterator(); iter.hasNext();
) {
>  -                assertTagsHaveParent( (Script) iter.next(), parent );
>  +                assertTagsHaveParent( (Script) iter.next(), parent, context );
>               }
>           }
>       }
>  
>  
>  
>  1.7       +4 -4      jakarta-commons/jelly/jelly-tags/xml/src/java/org/apache/commons/jelly/tags/xml/TransformTag.java
>  
>  Index: TransformTag.java
>  ===================================================================
>  RCS file: /home/cvs/jakarta-commons/jelly/jelly-tags/xml/src/java/org/apache/commons/jelly/tags/xml/TransformTag.java,v
>  retrieving revision 1.6
>  retrieving revision 1.7
>  diff -u -r1.6 -r1.7
>  --- TransformTag.java	27 Oct 2004 21:51:13 -0000	1.6
>  +++ TransformTag.java	28 Dec 2004 01:39:23 -0000	1.7
>  @@ -372,7 +372,7 @@
>   
>                       Tag tag = null;
>                       try {
>  -                        tag = ((TagScript) script).getTag();
>  +                        tag = ((TagScript) script).getTag(getContext());
>                       } catch (JellyException e) {
>                           throw new JellyTagException(e);
>                       }
>  @@ -414,7 +414,7 @@
>       
>                       Tag tag = null;
>                       try {
>  -                        tag = ((TagScript) script).getTag();
>  +                        tag = ((TagScript) script).getTag(getContext());
>                       } catch (JellyException e) {
>                           throw new JellyTagException(e);
>                       }
>  
>  
>  
>  1.1                  jakarta-commons/jelly/src/test/org/apache/commons/jelly/core/TestCoreMemoryLeak.java
>  
>  Index: TestCoreMemoryLeak.java
>  ===================================================================
>  /*
>   * Copyright 2002,2004 The Apache Software Foundation.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
>   * You may obtain a copy of the License at
>   *
>   *      http://www.apache.org/licenses/LICENSE-2.0
>   *
>   * Unless required by applicable law or agreed to in writing, software
>   * distributed under the License is distributed on an "AS IS" BASIS,
>   * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
>  package org.apache.commons.jelly.core;
>  
>  /** Tests for basic memory leaking in core tags. Runs a few test scripts many times.
>   * @author Hans Gilde
>   *
>   */
>  public class TestCoreMemoryLeak extends BaseMemoryLeakTest {
>  
>      /** The JUnit constructor.
>       * @param name
>       */
>      public TestCoreMemoryLeak(String name) {
>          super(name);
>      }
>      
>      public void testBasicScriptForLeak() throws Exception {
>          assertTrue(runScriptManyTimes("c.jelly", 10000) < 200000);
>      }
>  
>  }
>  
>  
>  
>  1.1                  jakarta-commons/jelly/src/test/org/apache/commons/jelly/core/BaseMemoryLeakTest.java
>  
>  Index: BaseMemoryLeakTest.java
>  ===================================================================
>  /*
>   * Copyright 2002,2004 The Apache Software Foundation.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
>   * You may obtain a copy of the License at
>   *
>   *      http://www.apache.org/licenses/LICENSE-2.0
>   *
>   * Unless required by applicable law or agreed to in writing, software
>   * distributed under the License is distributed on an "AS IS" BASIS,
>   * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>   * See the License for the specific language governing permissions and
>   * limitations under the License.
>   */
>  package org.apache.commons.jelly.core;
>  
>  import java.io.ByteArrayInputStream;
>  import java.io.IOException;
>  import java.io.InputStream;
>  import java.net.URL;
>  
>  import junit.framework.TestCase;
>  
>  import org.apache.commons.jelly.JellyContext;
>  import org.apache.commons.jelly.JellyException;
>  import org.apache.commons.jelly.Script;
>  import org.apache.commons.jelly.XMLOutput;
>  import org.apache.commons.jelly.parser.XMLParser;
>  import org.apache.commons.logging.Log;
>  import org.apache.commons.logging.LogFactory;
>  import org.xml.sax.InputSource;
>  import org.xml.sax.SAXException;
>  
>  /**
>   * Automates the basic process of testing a tag library for a memory leak.
>   * <p>
>   * To use it, extend it. Use the {@link runScriptManyTimes(String, int)}
>   * method in your unit tests.
>   * 
>   * @author Hans Gilde
>   *  
>   */
>  public class BaseMemoryLeakTest extends TestCase {
>      private final static Log log = LogFactory.getLog(BaseMemoryLeakTest.class);
>  
>      /**
>       * The JUnit constructor
>       * 
>       * @param name
>       */
>      public BaseMemoryLeakTest(String name) {
>          super(name);
>      }
>  
>      /** Runs a script count times and reports the number of bytes "leaked".
>       * Note that "leaked" means "not collected by the GC"
>       * and can easily be different between JVM's. This is because all 
>       * freed references may not be available for GC in the short time
>       * between their freeing and the completion of this test.
>       * <p/>
>       * However, running a 
>       * script 10,000 or 100,000 times should be a pretty good test
>       * for a memory leak. If there's not too much memory "leaked",
>       * you're probably OK.
>       * @param scriptName The path to the script, from the classloader of the current
class.
>       * @param count The number of times to run the script.
>       * @return The number of bytes "leaked"
>       * @throws IOException
>       * @throws SAXException
>       * @throws JellyException
>       */
>      public long runScriptManyTimes(String scriptName, int count)
>              throws IOException, SAXException, JellyException {
>          Runtime rt = Runtime.getRuntime();
>          JellyContext jc = new JellyContext();
>          jc.setClassLoader(getClass().getClassLoader());
>  
>          XMLOutput output = XMLOutput.createDummyXMLOutput();
>          
>          URL url = this.getClass().getResource(scriptName);
>  
>          String exturl = url.toExternalForm();
>          int lastSlash = exturl.lastIndexOf("/");
>          String extBase = exturl.substring(0,lastSlash+1);
>          URL baseurl = new URL(extBase);
>          jc.setCurrentURL(baseurl);
>          
>          InputStream is = url.openStream();
>          byte[] bytes = new byte[is.available()];
>          is.read(bytes);
>  
>          InputStream scriptIStream = new ByteArrayInputStream(bytes);
>          InputSource scriptISource = new InputSource(scriptIStream);
>  
>          is.close();
>          is = null;
>          bytes = null;
>  
>          rt.runFinalization();
>          rt.gc();
>  
>          long start = rt.totalMemory() - rt.freeMemory();
>          log.info("Starting memory test with used memory of " + start);
>  
>          XMLParser parser;
>          Script script;
>  
>          int outputEveryXIterations = outputEveryXIterations();
>  
>          for (int i = 0; i < count; i++) {
>              scriptIStream.reset();
>              parser = new XMLParser();
>  
>              script = parser.parse(scriptISource);
>              script.run(jc, output);
>  			log.info("TagHolderMap has " + jc.getTagHolderMap().size() + " entries.");
>  			// PL: I don't see why but removing the clear here 
>  			//     does make the test fail!
>  			//     As if the WeakHashMap wasn't weak enough...
>  			jc.clear();
>  
>              if (outputEveryXIterations != 0 && i % outputEveryXIterations ==
0) {
>                  parser = null;
>                  script = null;
>                  
>                  rt.runFinalization();
>                  rt.gc();
>                  long middle = rt.totalMemory() - rt.freeMemory();
>                  log.info("Memory test after " + i + " runs: "
>                          + (middle - start));
>              }
>          }
>          
>          rt.gc();
>  
>          jc = null;
>          output = null;
>          parser = null;
>          script = null;
>  
>          scriptIStream = null;
>          scriptISource = null;
>  
>          rt.runFinalization();
>          rt.gc();
>          
>          long nullsDone = rt.totalMemory() - rt.freeMemory();
>          log.info("Memory test completed, memory \"leaked\": " + (nullsDone - start));
>          
>          return nullsDone - start;
>      }
>  
>      protected int outputEveryXIterations() {
>          return 1000;
>      }
>  
>  }
>  
>  
>  
>  1.2       +1 -10     jakarta-commons/jelly/src/java/org/apache/commons/jelly/util/TagUtils.java
>  
>  Index: TagUtils.java
>  ===================================================================
>  RCS file: /home/cvs/jakarta-commons/jelly/src/java/org/apache/commons/jelly/util/TagUtils.java,v
>  retrieving revision 1.1
>  retrieving revision 1.2
>  diff -u -r1.1 -r1.2
>  --- TagUtils.java	27 Oct 2004 21:49:28 -0000	1.1
>  +++ TagUtils.java	28 Dec 2004 01:39:23 -0000	1.2
>  @@ -20,7 +20,6 @@
>   import org.apache.commons.jelly.impl.CompositeTextScriptBlock;
>   import org.apache.commons.jelly.impl.ScriptBlock;
>   import org.apache.commons.jelly.impl.TextScript;
>  -import org.apache.commons.jelly.impl.WeakReferenceWrapperScript;
>   
>   /** Contains static methods to help tag developers.
>    * @author Hans Gilde
>  @@ -36,15 +35,7 @@
>        */
>       public static void trimScript(Script body) {
>           synchronized(body) {
>  -            if (body instanceof WeakReferenceWrapperScript) {
>  -                WeakReferenceWrapperScript wrrs = (WeakReferenceWrapperScript) body;
>  -                try {
>  -                    wrrs.trimWhitespace();
>  -                } catch (JellyTagException e) {
>  -                    //TODO handle this exception once the Tag interface allows JellyTagException
to be thrown
>  -                    return;
>  -                }
>  -            } else if ( body instanceof CompositeTextScriptBlock ) {
>  +            if ( body instanceof CompositeTextScriptBlock ) {
>                   CompositeTextScriptBlock block = (CompositeTextScriptBlock) body;
>                   block.trimWhitespace();
>               }
>  
>  
>  
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>
>  
>


---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message