On 28 August 2012 20:59, Philippe Mouawad <[email protected]> wrote: > Hello sebb, > I fixed issues you mentioned so you can figure out what I mean.
OK. > 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. Done. I think this is now complete. > 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.
