On 25 August 2012 14:17, <[email protected]> wrote: > Author: pmouawad > Date: Sat Aug 25 13:17:19 2012 > New Revision: 1377291 > > URL: http://svn.apache.org/viewvc?rev=1377291&view=rev > Log: > Bug 53782 - Enhance JavaSampler handling of JavaSamplerClient cleanup to use > less memory > Only register instance of JavaSamplerClient that have overriden or > implemented teardownTest > Bugzilla Id: 53782
-1 Sorry, but I don't think the fixes entirely solve the problem. See below for details. I think the first thing to do is to capture the existing behaviour in some unit test cases. For this the current changes need to be reverted. Once the test cases are set up correctly, changes can be made to try and implement the performance improvements. > Modified: > > jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java > jmeter/trunk/xdocs/changes.xml > jmeter/trunk/xdocs/usermanual/component_reference.xml > > Modified: > jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java > URL: > http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java?rev=1377291&r1=1377290&r2=1377291&view=diff > ============================================================================== > --- > jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java > (original) > +++ > jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java > Sat Aug 25 13:17:19 2012 > @@ -18,10 +18,14 @@ > > package org.apache.jmeter.protocol.java.sampler; > > +import java.lang.reflect.Method; > import java.util.Arrays; > import java.util.HashSet; > +import java.util.Map; > import java.util.Set; > +import java.util.concurrent.ConcurrentHashMap; > > +import org.apache.commons.lang3.exception.ExceptionUtils; > import org.apache.jmeter.config.Arguments; > import org.apache.jmeter.config.ConfigTestElement; > import org.apache.jmeter.samplers.AbstractSampler; > @@ -74,6 +78,13 @@ public class JavaSampler extends Abstrac > private transient JavaSamplerContext context = null; > > /** > + * Cache of classname, boolean that holds information about a class and > wether or not it should > + * be registered for cleanup. > + * This is done to avoid using reflection on each registration > + */ > + private transient Map<String, Boolean> isToBeRegisteredCache = new > ConcurrentHashMap<String, Boolean>(); This is not needed. There is only ever one class involved, which is the javaClient, so the value can be deternined when the class is instantiated. > + > + /** > * Set used to register all JavaSamplerClient and JavaSamplerContext. > * This is used so that the JavaSamplerClient can be notified when the > test ends. > */ > @@ -137,27 +148,72 @@ public class JavaSampler extends Abstrac > * @param entry > * the Entry for this sample > * @return test SampleResult > + * @throws NoSuchMethodException > + * @throws SecurityException The new code should not cause additional exceptions. > */ > - public SampleResult sample(Entry entry) { > - Arguments args = getArguments(); > - args.addArgument(TestElement.NAME, getName()); // Allow Sampler > access > - // to test element > name > - context = new JavaSamplerContext(args); > - if (javaClient == null) { > - log.debug(whoAmI() + "\tCreating Java Client"); > - createJavaClient(); > - javaClientAndContextSet.add(new Object[]{javaClient, context}); > - javaClient.setupTest(context); > + public SampleResult sample(Entry entry) { > + try { > + Arguments args = getArguments(); > + args.addArgument(TestElement.NAME, getName()); // Allow Sampler > access > + // to test > element name > + context = new JavaSamplerContext(args); > + if (javaClient == null) { > + log.debug(whoAmI() + "\tCreating Java Client"); > + createJavaClient(); > + registerForCleanup(javaClient, context); > + javaClient.setupTest(context); > + } > + > + SampleResult result = javaClient.runTest(context); > + > + // Only set the default label if it has not been set > + if (result != null && result.getSampleLabel().length() == 0) { > + result.setSampleLabel(getName()); > + } > + > + return result; > + } catch(Exception ex) { > + SampleResult sampleResultIfError = new SampleResult(); > + sampleResultIfError.setSampleLabel(getName()); > + sampleResultIfError.setSuccessful(false); > + sampleResultIfError.setResponseCode("500"); // $NON-NLS-1$ > + > sampleResultIfError.setResponseMessage(ExceptionUtils.getRootCauseMessage(ex)); > + > sampleResultIfError.setResponseData(ExceptionUtils.getStackTrace(ex), > "UTF-8"); > + return sampleResultIfError; There is already a special ErrorSamplerClient for such cases. > } > + } > > - SampleResult result = javaClient.runTest(context); > - > - // Only set the default label if it has not been set > - if (result != null && result.getSampleLabel().length() == 0) { > - result.setSampleLabel(getName()); > + /** > + * Only register jsClient if it contains a custom teardownTest method > + * @param jsClient JavaSamplerClient > + * @param jsContext JavaSamplerContext > + * @throws NoSuchMethodException > + * @throws SecurityException > + */ > + private final void registerForCleanup(JavaSamplerClient jsClient, > + JavaSamplerContext jsContext) throws SecurityException, > NoSuchMethodException { > + if(isToBeRegistered(jsClient.getClass())) { > + javaClientAndContextSet.add(new Object[]{jsClient, jsContext}); This is only called if sample() is called. This changes the behaviour in some edge cases. > } > + } > > - return result; > + /** > + * Tests clazz to see if a custom teardown method has been written and > caches the test result. > + * If classes uses {@link > AbstractJavaSamplerClient#teardownTest(JavaSamplerContext)} then it won't > + * be registered for cleanup as it does nothing. > + * @param clazz Class to be verified > + * @return true if clazz should be registered for cleanup > + * @throws SecurityException > + * @throws NoSuchMethodException Should not throw NoSuchMethodException. Not sure it should throw SecurityException either. > + */ > + private boolean isToBeRegistered(Class<? extends JavaSamplerClient> > clazz) throws SecurityException, NoSuchMethodException { > + Boolean isToBeRegistered = > isToBeRegisteredCache.get(clazz.getName()); > + if(isToBeRegistered == null) { > + Method method = clazz.getMethod("teardownTest", new > Class[]{JavaSamplerContext.class}); This does not deal with the case where a JavaSampler implementation does not sub-class AbstractJavaSamplerClient and does not define tearDownTest. We need a test for that. If the method is not found, clearly there is no need to register the class. > + isToBeRegistered = > Boolean.valueOf(!method.getDeclaringClass().equals(AbstractJavaSamplerClient.class)); > + isToBeRegisteredCache.put(clazz.getName(), isToBeRegistered); > + } > + return isToBeRegistered.booleanValue(); > } > > /** > @@ -244,6 +300,7 @@ public class JavaSampler extends Abstrac > } > javaClientAndContextSet.clear(); > } > + isToBeRegisteredCache.clear(); > } > > /* Implements TestStateListener.testEnded(String) */ > > Modified: jmeter/trunk/xdocs/changes.xml > URL: > http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1377291&r1=1377290&r2=1377291&view=diff > ============================================================================== > --- jmeter/trunk/xdocs/changes.xml (original) > +++ jmeter/trunk/xdocs/changes.xml Sat Aug 25 13:17:19 2012 > @@ -135,6 +135,7 @@ Shortcut for Function Helper Dialog is n > <ul> > <li><bugzilla>55310</bugzilla> - TestAction should implement > Interruptible</li> > <li><bugzilla>53318</bugzilla> - Add Embedded URL Filter to HTTP Request > Defaults Control </li> > +<li><bugzilla>53782</bugzilla> - Enhance JavaSampler handling of > JavaSamplerClient cleanup to use less memory</li> > </ul> > > <h3>Controllers</h3> > > Modified: jmeter/trunk/xdocs/usermanual/component_reference.xml > URL: > http://svn.apache.org/viewvc/jmeter/trunk/xdocs/usermanual/component_reference.xml?rev=1377291&r1=1377290&r2=1377291&view=diff > ============================================================================== > --- jmeter/trunk/xdocs/usermanual/component_reference.xml (original) > +++ jmeter/trunk/xdocs/usermanual/component_reference.xml Sat Aug 25 13:17:19 > 2012 > @@ -566,6 +566,9 @@ The fields allow variables to be used, s > </p> > </description> > > +<note>Since JMeter 2.8, if method teardownTest is not overriden by > subclasses of AbstractJavaSamplerClient, then the method will not be called > to optimise JMeter memory behaviour. > +This will not have any impact on existing Test plans. > +</note> I don't think the note should be added to component reference (it will just be confusing to readers); it belongs in changes. > <note>The Add/Delete buttons don't serve any purpose at present.</note> > > <properties> > >
