Hello sebb, My answers below. Regards Philippe
On Tue, Aug 28, 2012 at 7:58 PM, sebb <[email protected]> wrote: > 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. > > Sorry this should have been static. In this case it is needed as it will avoid testing classes many times. > > + > > + /** > > * 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. > OK I will fix it. > > > */ > > - 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. > > Ok will use it. > > } > > + } > > > > - 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. > > This is called only if javaClient is created , what would be this case ? In case instance is cloned javaClient will also be null. > > } > > + } > > > > - 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. > It should but as I will fix previous SampleResult as you propose it will be OK. > > > + */ > > + 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. > I am not sure to understand, as method must implement JavaSamplerClient, it must define tearDownTest. Am I missing something ? > > 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. > > Ok I can remove it. > > <note>The Add/Delete buttons don't serve any purpose at present.</note> > > > > <properties> > > > > > -- Cordialement. Philippe Mouawad.
