You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by sebb <se...@gmail.com> on 2012/08/20 22:41:44 UTC
More efficient JavaSampler implementation [was: svn commit: r1374946]
On 20 August 2012 20:36, Philippe Mouawad <ph...@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.
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.
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.]
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.
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.
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.
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 <se...@gmail.com> wrote:
>
>> On 20 August 2012 09:49, <pm...@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.
Re: More efficient JavaSampler implementation [was: svn commit: r1374946]
Posted by Philippe Mouawad <ph...@gmail.com>.
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 <se...@gmail.com> wrote:
> On 20 August 2012 22:02, Philippe Mouawad <ph...@gmail.com>
> wrote:
> > On Mon, Aug 20, 2012 at 10:41 PM, sebb <se...@gmail.com> wrote:
> >
> >> On 20 August 2012 20:36, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
> >> >
> >> >> On 20 August 2012 09:49, <pm...@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.
Re: More efficient JavaSampler implementation [was: svn commit: r1374946]
Posted by sebb <se...@gmail.com>.
On 20 August 2012 22:02, Philippe Mouawad <ph...@gmail.com> wrote:
> On Mon, Aug 20, 2012 at 10:41 PM, sebb <se...@gmail.com> wrote:
>
>> On 20 August 2012 20:36, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
>> >
>> >> On 20 August 2012 09:49, <pm...@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.
Re: More efficient JavaSampler implementation [was: svn commit: r1374946]
Posted by Philippe Mouawad <ph...@gmail.com>.
On Mon, Aug 20, 2012 at 10:41 PM, sebb <se...@gmail.com> wrote:
> On 20 August 2012 20:36, Philippe Mouawad <ph...@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 <se...@gmail.com> wrote:
> >
> >> On 20 August 2012 09:49, <pm...@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.