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 Mon, 20 Aug 2012 21:02:11 GMT
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

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

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

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


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

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

>
> 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.
> >>
> >> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message