Hello sebb, I fixed issues you mentioned so you can figure out what I mean.
I didn't rollback as in my opinion there is really little chance of breaking something and if it happens we should get some bug report to fix it. If you want to, feel free to do it but in my opinion this change is worth regarding the benefit it brings in terms of memory. The second change I would make is to remove implementations of teardownTest methods in JavaTest and SleepTest as they are useless and the way they are coded now make them register for cleanup. Regards Philippe On Tue, Aug 28, 2012 at 9:10 PM, Philippe Mouawad < [email protected]> wrote: > 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. > > > > -- Cordialement. Philippe Mouawad.
