jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: More efficient JavaSampler implementation [was: svn commit: r1374946]
Date Sat, 25 Aug 2012 13:21:23 GMT
Hello Sebb,
I have implemented in
https://issues.apache.org/bugzilla/show_bug.cgi?id=53782  what I think I
have understood from your mail (at least I hope so).

Note that to be useful in your tests, we should not override teardownTest
in SleepTest or JavaTest as they do nothing but logging.

I didn't implement this part:

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

As I don't understand why we would have to do that.

Please review this to be sure I understood what you meant.


Regards
Philippe


On Mon, Aug 20, 2012 at 11:38 PM, sebb <sebbaz@gmail.com> wrote:

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



-- 
Cordialement.
Philippe Mouawad.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message