jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1377291 - in /jmeter/trunk: src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java xdocs/changes.xml xdocs/usermanual/component_reference.xml
Date Tue, 28 Aug 2012 19:57:16 GMT
On 28 August 2012 20:10, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> Hello sebb,
> My answers below.
>
> Regards
> Philippe
>
> On Tue, Aug 28, 2012 at 7:58 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 25 August 2012 14:17,  <pmouawad@apache.org> 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

The Set javaClientAndContextSet could contain JavaSampler instances as
before (but not all instances).
The instance contains javaClient and context, which are all that is
needed to call the teardownTest method.
That would save extra references, and simplify the code.

>> >      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.

It could be avoided by setting it up in the testStarted() method as
that is done before the class is cloned.
The testStarted() method could create the class (but not instantiate
it) and check for the method implementation.

The check would still be repeated for each instance of the class in a
test plan, but caching that seems unnecessary.

>> > +
>> > +    /**
>> >       * 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.

Sorry, I'd overlooked the fact that the method is not called if the
client has not been created yet.

>> >          }
>> > +    }
>> >
>> > -        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 ?

Sorry, I'd overlooked 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.
>>
>> Ok I can remove it.
>
>> >  <note>The Add/Delete buttons don't serve any purpose at present.</note>
>> >
>> >  <properties>
>> >
>> >
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message