jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: More efficient JavaSampler implementation [was: svn commit: r1374946]
Date Mon, 20 Aug 2012 21:38:50 GMT
On 20 August 2012 22:02, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> On Mon, Aug 20, 2012 at 10:41 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 20 August 2012 20:36, Philippe Mouawad <philippe.mouawad@gmail.com>
>> wrote:
>> > Then what if we implement testEnded the same way.
>> > Looking at previous implementation I don't see why it was done this way.
>>
>> It's possible that it was intentional for every instance to get
>> notification of test ended.
>>
>
> It seems strange to me, because why make one JavaSampler handle teardown
> for all others and not each one handle it for its JavaSamplerClient

Because testEnded is only called once per instance in the test plan,
not once per instance in each thread.

The list of TestListeners is created by the JMeter engine before the
test plan is cloned for each thread.

Otherwise it would be huge as well.

>>
>> But whatever the reason, changing the behaviour may break clients.
>>
>> > In my opinion implemented as I did is the best for performances as it
>> > cleaned up as soon as it becomes useless, I agree it breaks contract but
>> we
>> > could introduce a new parameter :
>> >
>> >    - java.samplerclient.teardown_at_thread_end=true => Would clean at end
>> >    of thread
>>
>> Using a property to control this would potentially cause issues if
>> there are multiple classes requiring different behaviour.
>>
>
> In my thinking, users will set
> java.samplerclient.teardown_at_thread_end=false until they migrate their
> JavaSamplerClient to new behaviour

Why should we force users to do this?
They may have multiple clients which require different behaviour.
They may have no control over some or all of the clients (could be bought-in).

>>
>> It would be better to support ThreadListener, though it would increase
>> the size of the corresponding engine list regardless of whether the
>> client implements it. [Unless it's possible to dynamically update that
>> list if the client implements it.]
>>
>
> I don't understand what you mean.

I'd assumed that ThreadListener was implemented by creating a list
from the test plan classes; however I see that the classes are
dynamically scanned.
So supporting ThreadListener would not require additional memory, just
additional processing time.

>>
>> This should be treated as a separate enhancement.
>>
>> >    - java.samplerclient.teardown_at_thread_end=false => Would clean at
>> end
>> >    of test which would work as today but we would change impl to remove
>> static
>> >    collection and just call releaseJavaClient()
>>
>> That would only call the teardownTest method once, rather than per
>> instance.
>>
>
> Sorry I am not understanding, I may be missing something, can you explain a
> little more ?

See above - testEnded is only invoked on the classes  which were
present before the test plan was cloned.

>
>> The Javadoc is not clear on this, unlike the Javadoc for testEnded, so
>> this would potentially be a change in behaviour.
>>
>> I think it's a bit risky to change this now.
>>
>
>> However I think it may be possible to support existing behaviour for
>> teardownTest whilst still reducing the memory footprint.
>> The idea is as follows:
>>
>> AbstractJavaSampler implements setupTest and teardownTest, so there is
>> no need for subclass implementations to do so.
>> JavaSampler could check whether the actual class (or one of its
>> superclasses, apart from AbstractJavaSampler) implements teardownTest
>> before storing a reference.
>>
> you mean AbstractJavaSamplerClient ?

Oops, yes.

>>
>> To ensure that even AbstractJavaSampler#teardownTest was called at
>> least once, the reference could be added to the set in testEnded()
>> before processing the set.
>>
>
> Sorry not clear for me.

If we don't save references to subclasses that don't themselves
implement teardownTest, then AbstractJavaSamplerClient#teardownTest
won't be called at all.

>>
>> I think that this would preserve the current behaviour as far as the
>> client class is concerned - classes should not notice any change in
>> behaviour.
>> If so, it could be implemented without making the behaviour optional,
>> though we should note the change just in case.
>> However, it would not help for 3rd party classes that don't extend
>> AbstractJavaSampler but directly implement JavaSamplerClient.
>>
>> It might also be possible to support TestListener in client classes,
>> in which case they could use that instead of implementing
>> teardownTest/setupTest.
>> Implementing testEnded() would be trivial; testStarted() would require
>> instantiating the client class earlier in the cycle.
>> The client classname is one of the fixed strings presented in the
>> drop-down list, so will not change once a test starts.
>> I don't think we make any guarantees as to when the client class is
>> initialised, so we would just need to note this in the changes.
>>
>> Again, maybe that should be treated as a separate enhancement.
>>
>> >
>> >
>> >
>> > What do you think ?
>> > Regards
>> > Philippe
>> > On Mon, Aug 20, 2012 at 6:13 PM, sebb <sebbaz@gmail.com> wrote:
>> >
>> >> On 20 August 2012 09:49,  <pmouawad@apache.org> wrote:
>> >> > Author: pmouawad
>> >> > Date: Mon Aug 20 08:49:59 2012
>> >> > New Revision: 1374946
>> >> >
>> >> > URL: http://svn.apache.org/viewvc?rev=1374946&view=rev
>> >> > Log:
>> >> > Bug 53743 - JavaSamplers.allSamplers static Set keeps references even
>> >> after thread has ended
>> >> > Bugzilla Id: 53743
>> >>
>> >> -1, please revert.

PING

>> >>
>> >> This does solve the memory issue, but it breaks the contract for
>> >>
>> >> .JavaSamplerClient#teardownTest(JavaSamplerContext)
>> >>
>> >> which says:
>> >>
>> >> "Do any clean-up required by this test at the end of a test run."
>> >>
>> >> teardownTest is now called at end of thread, not end of test.
>> >>
>> >> This may break some implementations.
>> >>
>> >> > Modified:
>> >> >
>> >>
>> jmeter/trunk/src/protocol/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java
>> >> >     jmeter/trunk/xdocs/changes.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=1374946&r1=1374945&r2=1374946&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
>> >> Mon Aug 20 08:49:59 2012
>> >> > @@ -20,7 +20,6 @@ package org.apache.jmeter.protocol.java.
>> >> >
>> >> >  import java.util.Arrays;
>> >> >  import java.util.HashSet;
>> >> > -import java.util.Iterator;
>> >> >  import java.util.Set;
>> >> >
>> >> >  import org.apache.jmeter.config.Arguments;
>> >> > @@ -31,6 +30,7 @@ import org.apache.jmeter.samplers.Entry;
>> >> >  import org.apache.jmeter.samplers.SampleResult;
>> >> >  import org.apache.jmeter.testelement.TestElement;
>> >> >  import org.apache.jmeter.testelement.TestListener;
>> >> > +import org.apache.jmeter.testelement.ThreadListener;
>> >> >  import org.apache.jmeter.testelement.property.TestElementProperty;
>> >> >  import org.apache.jorphan.logging.LoggingManager;
>> >> >  import org.apache.log.Logger;
>> >> > @@ -41,7 +41,7 @@ import org.apache.log.Logger;
>> >> >   * information on writing Java code to be executed by this sampler.
>> >> >   *
>> >> >   */
>> >> > -public class JavaSampler extends AbstractSampler implements
>> >> TestListener {
>> >> > +public class JavaSampler extends AbstractSampler implements
>> >> TestListener, ThreadListener {
>> >> >
>> >> >      private static final Logger log =
>> >> LoggingManager.getLoggerForClass();
>> >> >
>> >> > @@ -76,19 +76,10 @@ public class JavaSampler extends Abstrac
>> >> >      private transient JavaSamplerContext context = null;
>> >> >
>> >> >      /**
>> >> > -     * Set used to register all active JavaSamplers. This is used
so
>> >> that the
>> >> > -     * samplers can be notified when the test ends.
>> >> > -     */
>> >> > -    private static final Set<JavaSampler> allSamplers = new
>> >> HashSet<JavaSampler>();
>> >> > -
>> >> > -    /**
>> >> >       * Create a JavaSampler.
>> >> >       */
>> >> >      public JavaSampler() {
>> >> >          setArguments(new Arguments());
>> >> > -        synchronized (allSamplers) {
>> >> > -            allSamplers.add(this);
>> >> > -        }
>> >> >      }
>> >> >
>> >> >      /**
>> >> > @@ -240,23 +231,9 @@ public class JavaSampler extends Abstrac
>> >> >          log.debug(whoAmI() + "\ttestStarted(" + host + ")");
>> >> >      }
>> >> >
>> >> > -    /**
>> >> > -     * Method called at the end of the test. This is called only on
>> one
>> >> instance
>> >> > -     * of JavaSampler. This method will loop through all of the other
>> >> > -     * JavaSamplers which have been registered (automatically in the
>> >> > -     * constructor) and notify them that the test has ended, allowing
>> >> the
>> >> > -     * JavaSamplerClients to cleanup.
>> >> > -     */
>> >> > +    /* Implements TestListener.testEnded() */
>> >> >      public void testEnded() {
>> >> >          log.debug(whoAmI() + "\ttestEnded");
>> >> > -        synchronized (allSamplers) {
>> >> > -            Iterator<JavaSampler> i = allSamplers.iterator();
>> >> > -            while (i.hasNext()) {
>> >> > -                JavaSampler sampler = i.next();
>> >> > -                sampler.releaseJavaClient();
>> >> > -                i.remove();
>> >> > -            }
>> >> > -        }
>> >> >      }
>> >> >
>> >> >      /* Implements TestListener.testEnded(String) */
>> >> > @@ -300,4 +277,15 @@ public class JavaSampler extends Abstrac
>> >> >          String guiClass =
>> >> configElement.getProperty(TestElement.GUI_CLASS).getStringValue();
>> >> >          return APPLIABLE_CONFIG_CLASSES.contains(guiClass);
>> >> >      }
>> >> > +
>> >> > +    public void threadStarted() {
>> >> > +        // NOOP
>> >> > +    }
>> >> > +
>> >> > +    /**
>> >> > +     * Cleanup java client
>> >> > +     */
>> >> > +    public void threadFinished() {
>> >> > +        releaseJavaClient();
>> >> > +    }
>> >> >  }
>> >> >
>> >> > Modified: jmeter/trunk/xdocs/changes.xml
>> >> > URL:
>> >>
>> http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1374946&r1=1374945&r2=1374946&view=diff
>> >> >
>> >>
>> ==============================================================================
>> >> > --- jmeter/trunk/xdocs/changes.xml (original)
>> >> > +++ jmeter/trunk/xdocs/changes.xml Mon Aug 20 08:49:59 2012
>> >> > @@ -84,6 +84,7 @@ JSR223 Test Elements using Script file a
>> >> >  <li><bugzilla>53357</bugzilla> - JMS Point to Point
reports too high
>> >> response times in Request Response Mode</li>
>> >> >  <li><bugzilla>53440</bugzilla> - SSL connection
leads to
>> >> ArrayStoreException on JDK 6 with some KeyManagerFactory SPI</li>
>> >> >  <li><bugzilla>53511</bugzilla> - access log sampler
SessionFilter
>> >> throws NullPointerException - cookie manager not initialized
>> properly</li>
>> >> > +<li><bugzilla>53743</bugzilla> - JavaSamplers.allSamplers
static Set
>> >> keeps references even after thread has ended</li>
>> >> >  </ul>
>> >> >
>> >> >  <h3>Controllers</h3>
>> >> >
>> >> >
>> >>
>> >
>> >
>> >
>> > --
>> > Cordialement.
>> > Philippe Mouawad.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message