You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by UBIK LOAD PACK Support <su...@ubikloadpack.com> on 2014/10/14 20:57:25 UTC
Why do Functions that only have values as instance variable
synchronize execute ?
Hi,
Reviewing code of Functions in JMeter we don't understand why Functions
that only have values as instance variable (so Beanshell or StringFromFile
or IterationCounter (which could be improved to avoid it) are not
concerned) synchronize on execute.
It appears "values" instance variable is initialized within
StandardJMeterEngine thread and then only accessed in read mode by
JMeterThread
But it is true that if synchronized is removed then CompoundVariable is
accessed by multi threads but it seems it uses thread related data so it
should be OK.
--
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Vladimir Sitnikov <si...@gmail.com>.
Thank you for the kind words.
>I reviewed the patch, look OK for me, but why don't you also synchronize
>LogFunction2#execute since you do for LogFunction#execute ?
I should have put the reasoning why the synchronization is required.
It _is_ required in LogFunction to protect printDetails(System.out, s, t,
c); kind of calls.
LogFunction2 just delegates, thus there is no need to protect LogFunction2
itself.
If multiple threads execute LogFunction concurrently, the result will be
some garbage in the System.out.
I think it is better to have solid messages at cost of synchronization
rather than unreadable mix of multiple messages.
I guess we might move synchronization from LogFunction.execute to
LogFunction.logDetails to mitigate the contention (if any).
> Just to know, are you familiar with JMeter code or did you start looking
at it within this thread ?
I have some experience with the code base: I develop some plugins (timers,
csv readers, etc).
It was me who asked to create a git mirror a while ago:
https://issues.apache.org/jira/browse/INFRA-2570
This particular part (functions) is an easy starter. It is way much easier
than the controller-sampler flow.
Vladimir Sitnikov
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Philippe Mouawad <ph...@gmail.com>.
Hi,
One other note regarding a comment you put in code of Random.java regarding
the use of ThreadLocalRandom:
- https://issues.apache.org/bugzilla/show_bug.cgi?id=54453
Unfortunately my tests don't show performance improvement after adding it.
This disturbs me
Regards
On Sun, Oct 19, 2014 at 9:42 PM, Philippe Mouawad <
philippe.mouawad@gmail.com> wrote:
> Hello Vladimir,
> Thanks for contribution.
> For 2, 3 you can open a bugzilla and attach patch.
>
>
> I reviewed the patch, look OK for me, but why don't you also synchronize
> LogFunction2#execute since you do for LogFunction#execute ?
>
> Thanks
>
> On Sat, Oct 18, 2014 at 4:44 PM, Vladimir Sitnikov <
> sitnikov.vladimir@gmail.com> wrote:
>
>> Thanks for creating a bug id. I've attached the patch.
>>
>> What is the preferred way to handle small improvements like 2 and 3?
>>
>> Vladimir Sitnikov
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.
>
>
>
--
Cordialement.
Philippe Mouawad.
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Philippe Mouawad <ph...@gmail.com>.
Hi,
I commited patch with exception of LogFunction2 synchronized removal on
execute.
@All, as this change is touchy, I suggest you all do a code review and
possibly tests to ensure we don't break anything with it.
My analysis on this leads me to think it is OK, tests I ran on Javascript
function didn't show any issue except the following which seems to be
related to Eclipse Debugger/ Or Java (Tried Last java7, Last Java 8 and
Last JAva 6):
- I ran test, in debug mode, with new code, at end of test CPU is at 11%
and does not decrease. Running it in Run Mode , it is OK. Doing thread dump
(with debug mode) , I don't see anything in JMeter internals.
Regards
Philippe
On Sun, Oct 19, 2014 at 9:42 PM, Philippe Mouawad <
philippe.mouawad@gmail.com> wrote:
> Hello Vladimir,
> Thanks for contribution.
> For 2, 3 you can open a bugzilla and attach patch.
>
>
> I reviewed the patch, look OK for me, but why don't you also synchronize
> LogFunction2#execute since you do for LogFunction#execute ?
>
> Thanks
>
> On Sat, Oct 18, 2014 at 4:44 PM, Vladimir Sitnikov <
> sitnikov.vladimir@gmail.com> wrote:
>
>> Thanks for creating a bug id. I've attached the patch.
>>
>> What is the preferred way to handle small improvements like 2 and 3?
>>
>> Vladimir Sitnikov
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.
>
>
>
--
Cordialement.
Philippe Mouawad.
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Philippe Mouawad <ph...@gmail.com>.
Hello Vladimir,
Thanks for contribution.
For 2, 3 you can open a bugzilla and attach patch.
I reviewed the patch, look OK for me, but why don't you also synchronize
LogFunction2#execute since you do for LogFunction#execute ?
Thanks
On Sat, Oct 18, 2014 at 4:44 PM, Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:
> Thanks for creating a bug id. I've attached the patch.
>
> What is the preferred way to handle small improvements like 2 and 3?
>
> Vladimir Sitnikov
>
--
Cordialement.
Philippe Mouawad.
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Vladimir Sitnikov <si...@gmail.com>.
Thanks for creating a bug id. I've attached the patch.
What is the preferred way to handle small improvements like 2 and 3?
Vladimir Sitnikov
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Philippe Mouawad <ph...@gmail.com>.
Hello ,
Thanks for contribution, unfortunately mailing list does not allow
attachments.
So I opened this bugzilla where you can attach your patch:
https://issues.apache.org/bugzilla/show_bug.cgi?id=57114
Regards
Philippe M.
On Fri, Oct 17, 2014 at 10:00 AM, Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:
> I went through sub-classes of AbstractFunction and removed synchronization
> where it is not required.
> Basically, if a function does not modify state and if it accesses
> thread-safe objects only, then synchronization is not required.
>
> Please find the attached patch on top of e9228ccf / trunk@1632410.
>
> Notes:
> 0) The patch contains just synchronization removal + some corrections to
> CV. All the tests pass except
> testPropfile(org.apache.jmeter.save.TestSaveService)junit.framework.AssertionFailedError:
> Property File Version mismatch, ensure you update SaveService#FILEVERSION
> field with revision id of saveservice.properties.
>
> testPropfile fails without patch as well, so I did not investigate that.
>
> 1) I have no idea what to do with BeanShell function. bsh.Interpreter is
> thread safe, however bshInterpreter.set("vars",...) should not be accessed
> concurrently as that will just overwrite the value.
> I think it makes sense to wrap bsh interpreter in a ThreadLocal, so
> synchronization can be removed from BeanShell.execute
>
> 2) There are lots of cases when Thread.currentThread().getName() is used.
> This method always creates new String, thus it has performance impact (see
> [1]).
>
> 2.1) ThreadNumber function even tries to parse thread name. It would be
> great if ThreadNumber can be implemented
> via JMeterContextService.getContext().getThreadNum().
>
> 2.2) Other usages of currentThread.getName() might probably be replaced
> with JMeterContextService.getContext().getThread().getThreadName()
>
> 3) __time function parses regexp "/\\d+" in the execute. I guess it might
> make sense to move that to setParameters.
>
> [1]: https://bugs.openjdk.java.net/browse/JDK-8059677
>
> Vladimir Sitnikov
>
--
Cordialement.
Philippe Mouawad.
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Vladimir Sitnikov <si...@gmail.com>.
I went through sub-classes of AbstractFunction and removed synchronization
where it is not required.
Basically, if a function does not modify state and if it accesses
thread-safe objects only, then synchronization is not required.
Please find the attached patch on top of e9228ccf / trunk@1632410.
Notes:
0) The patch contains just synchronization removal + some corrections to
CV. All the tests pass except
testPropfile(org.apache.jmeter.save.TestSaveService)junit.framework.AssertionFailedError:
Property File Version mismatch, ensure you update SaveService#FILEVERSION
field with revision id of saveservice.properties.
testPropfile fails without patch as well, so I did not investigate that.
1) I have no idea what to do with BeanShell function. bsh.Interpreter is
thread safe, however bshInterpreter.set("vars",...) should not be accessed
concurrently as that will just overwrite the value.
I think it makes sense to wrap bsh interpreter in a ThreadLocal, so
synchronization can be removed from BeanShell.execute
2) There are lots of cases when Thread.currentThread().getName() is used.
This method always creates new String, thus it has performance impact (see
[1]).
2.1) ThreadNumber function even tries to parse thread name. It would be
great if ThreadNumber can be implemented
via JMeterContextService.getContext().getThreadNum().
2.2) Other usages of currentThread.getName() might probably be replaced
with JMeterContextService.getContext().getThread().getThreadName()
3) __time function parses regexp "/\\d+" in the execute. I guess it might
make sense to move that to setParameters.
[1]: https://bugs.openjdk.java.net/browse/JDK-8059677
Vladimir Sitnikov
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Philippe Mouawad <ph...@gmail.com>.
On Thursday, October 16, 2014, Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:
> > know wether CompoundVariables
> >which are in AbstractFunction subclasses values
You didn't read correctly:)
I think the main issue in this topic is to know wether CompoundVariables
which are in AbstractFunction subclasses values are thread safe (to the
exception of the non thread safe functions).
I am saying values field is array of compoundvariables , values field is in
AbstractFunction subclasses.
Of course CompoundVariables do not extend AbstractFunction !
>
> I am looking at commit e9228ccf8cca9b130c8730771693cbda024f8bb0
> git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1632410 and
> it reads that CompoundVariable does not extend AbstractFunction.
>
> >Today with synchronized we ensure that:
> >- While execute is being called by one thread, setParameters and other
> >execute() (from other threads) are blocked.
>
> I hope we can forbid setParameters usage after JMeterThread start.
> I understand bad things can happen if setParameters and execute are
> performed concurrently.
> I have very little idea how JMeter works internally, however I hope it does
> not do such things.
>
> >And due to the fact that it seems we never enter this condition of
> >CompoundVariable#execute() which alters an instance variable :
>
> 1) If the CV in question is dynamic (in terms of isDynamic==true after each
> execution), then execute is thread safe modulo thread safety of embedded
> functions/variables.
>
> 2) If the CV in question is not dynamic (e.g. it does not contain
> Functions/SimpleVariable), then CV is NOT thread-safe: isDynamic=false &
> permanentResults =... are not ordered, thus if multiple threads execute
> CV.execute concurrently, then a thread can observe isDynamic==false and
> stale value in permanentResults (== it would return wrong results)
This is what needs to be checked, but it requires reading jmeter code, I
can understand it can take time :)
>
> I see the following options to fix that (in order of my preference):
> 2.1) move "isDynamic" initialization to setParameters. It looks like it is
> just a instanceof check. If we can't calculate initial value in
> setParameters method (i.e. we must compute the value at the first execution
> of execute method), then initial value of permanentResults can be set to
> null and the check in execute() should read "if (isDynamic ||
> permanentResults==null)".
>
> 2.2) You can make isDynamic volatile and write code as
> "permanentResults=results.toString();
> isDynamic=false" (in this order!). This is a minimal change.
>
>
> PS. I would replace LinkedList compiledElements with ArrayList: it is
> faster, more memory efficient, etc and I would use indexed iteration in
> execute (to avoid creation of Iterator). It does not hurt readability in
> that particular method from my point of view.
>
>
> Vladimir
>
--
Cordialement.
Philippe Mouawad.
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Vladimir Sitnikov <si...@gmail.com>.
> know wether CompoundVariables
>which are in AbstractFunction subclasses values
I am looking at commit e9228ccf8cca9b130c8730771693cbda024f8bb0
git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1632410 and
it reads that CompoundVariable does not extend AbstractFunction.
>Today with synchronized we ensure that:
>- While execute is being called by one thread, setParameters and other
>execute() (from other threads) are blocked.
I hope we can forbid setParameters usage after JMeterThread start.
I understand bad things can happen if setParameters and execute are
performed concurrently.
I have very little idea how JMeter works internally, however I hope it does
not do such things.
>And due to the fact that it seems we never enter this condition of
>CompoundVariable#execute() which alters an instance variable :
1) If the CV in question is dynamic (in terms of isDynamic==true after each
execution), then execute is thread safe modulo thread safety of embedded
functions/variables.
2) If the CV in question is not dynamic (e.g. it does not contain
Functions/SimpleVariable), then CV is NOT thread-safe: isDynamic=false &
permanentResults =... are not ordered, thus if multiple threads execute
CV.execute concurrently, then a thread can observe isDynamic==false and
stale value in permanentResults (== it would return wrong results)
I see the following options to fix that (in order of my preference):
2.1) move "isDynamic" initialization to setParameters. It looks like it is
just a instanceof check. If we can't calculate initial value in
setParameters method (i.e. we must compute the value at the first execution
of execute method), then initial value of permanentResults can be set to
null and the check in execute() should read "if (isDynamic ||
permanentResults==null)".
2.2) You can make isDynamic volatile and write code as
"permanentResults=results.toString();
isDynamic=false" (in this order!). This is a minimal change.
PS. I would replace LinkedList compiledElements with ArrayList: it is
faster, more memory efficient, etc and I would use indexed iteration in
execute (to avoid creation of Iterator). It does not hurt readability in
that particular method from my point of view.
Vladimir
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Philippe Mouawad <p....@ubik-ingenierie.com>.
Hello sebb, Vladimir,
@Vladimir, as you seem to be a Threading expert, how about a critical
review of those functions ? An external eye would be welcome :-)
I think the main issue in this topic is to know wether CompoundVariables
which are in AbstractFunction subclasses values are thread safe (to the
exception of the non thread safe functions).
Today with synchronized we ensure that:
- While execute is being called by one thread, setParameters and other
execute() (from other threads) are blocked.
Currenlty:
- setParameters is called by 1 thread StandardJMeterEngine
- Execute is called by ThreadGroup X-X
If we remove the synchronized from execute then:
- CompoundVariable instance will be called by Many threads at the same time
My analysis is that it should be ok looking at CompoundVariable#execute()
and knowing the instance variables are initialized within
StandardJMeterEngine single thread:
private boolean hasFunction, isDynamic;
private String permanentResults = ""; // $NON-NLS-1$
private LinkedList<Object> compiledComponents = new
LinkedList<Object>();
And due to the fact that it seems we never enter this condition of
CompoundVariable#execute() which alters an instance variable :
if (!testDynamic) {
isDynamic = false;
permanentResults = results.toString();
}
But this needs to be double checked.
Regards
On Thu, Oct 16, 2014 at 8:15 PM, sebb <se...@gmail.com> wrote:
>
> On 16 October 2014 19:00, Vladimir Sitnikov <si...@gmail.com>
> wrote:
> >>I'm not sure I agree with that; the value of the mutable field needs
> >>to be published safely as well.
> >
> > Do I read it properly? "you are not sure if java.lang.String is safe"
>
> No, I meant that Globals.state is not published safely.
>
> > It is safe since java 1.5.
> > This is ensured by "17.5 final fields semantics" was added to JMM.
> >
> >> But Globals.state is not final or volatile.
> >
> > Here's "safe publication" via data race (Globals.state is not
> > volatile/synchronized/etc)
> > Thread 1:
> > class Globals { public static String state; }
> > Globals.state = new String(new char[]{'h','e','l','l','o'}); // no
> > volatile fields involved
> >
> > Thread 2:
> > System.out.println(Globals.state); // this _must_ print either null or
> > "hello".
> >
> > Note how thread 2 must not print h/he/hel/hell/, even though thread 1
> > copies characters from one array to another one.
> >
> >>How does that illustrate what you wrote here: "Just synchronization on
> the
> > same lock is not sufficient"
> >
> > Ok, here's example with synchronizations:
> >
> > Thread 1:
> > Globals.str = new StringFromFile();
> > str.setParameters(...) // this is method is synchronized
> >
> > Thread 2:
> > Globals.str.execute() // this method is synchronized
> >
> > There is no guarantee that execute will observe proper state of
> > StringFromFile.
> > For instance, str.execute might be executed before setParameters, then
> > "setParameters would not happens-before execute", thus there would be no
> > "safe publication from setParameters to execute".
>
> Of course not.
> I never meant to imply that synchronization affects execution order.
>
> >> True, but that would also affect single threaded code.
> >
> > The original question was to avoid contention between multiple threads. I
> > do not think we need spend time on discussing single-threaded executions.
>
> I was merely pointing out that if execute() is invoked before
> setParameters() then execute() won't see the new value in a
> single-threaded context either.
>
> That seemed so obvious that it did not need stating.
> Assuming that the methods are called in the correct sequence, there is
> still the possibility of unsafe publication in the absence of
> volatile/synch/etc when multiple threads are involved.
>
> > Once again my main point: I am not sure if volatile/synchronized is
> > required. I would like to dodge non-required synchronization.
>
> Fair enough.
>
> > Vladimir
>
--
Cordialement.
Philippe M.
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by sebb <se...@gmail.com>.
On 16 October 2014 19:00, Vladimir Sitnikov <si...@gmail.com> wrote:
>>I'm not sure I agree with that; the value of the mutable field needs
>>to be published safely as well.
>
> Do I read it properly? "you are not sure if java.lang.String is safe"
No, I meant that Globals.state is not published safely.
> It is safe since java 1.5.
> This is ensured by "17.5 final fields semantics" was added to JMM.
>
>> But Globals.state is not final or volatile.
>
> Here's "safe publication" via data race (Globals.state is not
> volatile/synchronized/etc)
> Thread 1:
> class Globals { public static String state; }
> Globals.state = new String(new char[]{'h','e','l','l','o'}); // no
> volatile fields involved
>
> Thread 2:
> System.out.println(Globals.state); // this _must_ print either null or
> "hello".
>
> Note how thread 2 must not print h/he/hel/hell/, even though thread 1
> copies characters from one array to another one.
>
>>How does that illustrate what you wrote here: "Just synchronization on the
> same lock is not sufficient"
>
> Ok, here's example with synchronizations:
>
> Thread 1:
> Globals.str = new StringFromFile();
> str.setParameters(...) // this is method is synchronized
>
> Thread 2:
> Globals.str.execute() // this method is synchronized
>
> There is no guarantee that execute will observe proper state of
> StringFromFile.
> For instance, str.execute might be executed before setParameters, then
> "setParameters would not happens-before execute", thus there would be no
> "safe publication from setParameters to execute".
Of course not.
I never meant to imply that synchronization affects execution order.
>> True, but that would also affect single threaded code.
>
> The original question was to avoid contention between multiple threads. I
> do not think we need spend time on discussing single-threaded executions.
I was merely pointing out that if execute() is invoked before
setParameters() then execute() won't see the new value in a
single-threaded context either.
That seemed so obvious that it did not need stating.
Assuming that the methods are called in the correct sequence, there is
still the possibility of unsafe publication in the absence of
volatile/synch/etc when multiple threads are involved.
> Once again my main point: I am not sure if volatile/synchronized is
> required. I would like to dodge non-required synchronization.
Fair enough.
> Vladimir
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Vladimir Sitnikov <si...@gmail.com>.
>I'm not sure I agree with that; the value of the mutable field needs
>to be published safely as well.
Do I read it properly? "you are not sure if java.lang.String is safe"
It is safe since java 1.5.
This is ensured by "17.5 final fields semantics" was added to JMM.
> But Globals.state is not final or volatile.
Here's "safe publication" via data race (Globals.state is not
volatile/synchronized/etc)
Thread 1:
class Globals { public static String state; }
Globals.state = new String(new char[]{'h','e','l','l','o'}); // no
volatile fields involved
Thread 2:
System.out.println(Globals.state); // this _must_ print either null or
"hello".
Note how thread 2 must not print h/he/hel/hell/, even though thread 1
copies characters from one array to another one.
>How does that illustrate what you wrote here: "Just synchronization on the
same lock is not sufficient"
Ok, here's example with synchronizations:
Thread 1:
Globals.str = new StringFromFile();
str.setParameters(...) // this is method is synchronized
Thread 2:
Globals.str.execute() // this method is synchronized
There is no guarantee that execute will observe proper state of
StringFromFile.
For instance, str.execute might be executed before setParameters, then
"setParameters would not happens-before execute", thus there would be no
"safe publication from setParameters to execute".
> True, but that would also affect single threaded code.
The original question was to avoid contention between multiple threads. I
do not think we need spend time on discussing single-threaded executions.
Once again my main point: I am not sure if volatile/synchronized is
required. I would like to dodge non-required synchronization.
Vladimir
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by sebb <se...@gmail.com>.
On 16 October 2014 17:34, Vladimir Sitnikov <si...@gmail.com> wrote:
>>> 2) Just synchronization on the same lock is not sufficient (see
>>> "synchronizes-with" in the JMM)
>>I don't follow that.
>
> Here's example:
> Thread 1:
> // Globals.state is a static non-volatile field
> Globals.state = new AtomicInteger(42); // 42 is stored in a volatile
> field, you can check that in java sources
But Globals.state is not final or volatile.
> Thread 2:
> System.out.println(Globals.state);
>
> Thread 2 _can_ print 0 and it would be _acceptable_ by JMM (including JMM8).
> This is an example that shows that "not every volatile read synchronizes
> with volatile write".
How does that illustrate what you wrote here:
"Just synchronization on the same lock is not sufficient"
> There's a basic explanation: if a reader happened to "perform
> synchronization" earlier than writer, then no happens-before appears.
Yes, I took it as read that the volatile write has to happen before
the read in order to publish the value safely.
> More advanced one: to make things safe, you need happens-before.
> "synchronization on the lock" does not induce happens before. "unlock of a
> monitor happens-before _subsequent_ lock of the same monitor".
Yes, I know.
> This is why I say: "synchronization in execute" does nothing. There is no
> way this synchronization ensures that this method would be executed after
> the init method.
True, but that would also affect single threaded code.
> See more detailed explanation in [1]
>
>> (* apart from final, but I did write "mutable fields", which excludes
> final)
>
> Even mutable field can be safely published if it is wrapped in a object
> with a final field.
> For instance, java.lang.String is always safe, however it contains char[]
> that is both mutable and unsafe.
>
> Even if you store java.lang.String to a mutable field, it will still be
> completely safe.
I'm not sure I agree with that; the value of the mutable field needs
to be published safely as well.
> Nothing is specific to String, you can do the same with your own classes
> and still get safe results (see [2])
>
>>However these are external (*) to the class; changes to the app design
>>may invalidate any such assumptions.
>
> Very true, however you must state threading contract in this kind of
> classes.
> JMeter is multithreaded by its nature, thus I would expect "guidelines for
> writing a custom function" to be accompanied by a JMM semantics that is
> ensured by JMeter engine.
> For instance, "execute function might be called from multiple
> threads simultaneously, however init is guaranteed to happens-before any
> call to execute".
>
> Don't you mean that threading semantics of JMeter can change at any point
> in time?
>
>> OK, but as already written, that is something over which the class has no
> control.
>
> I did review some of the threading code in JMeter and I did notice some
> code that already relied on the discussed JMeterThread.start happens-before.
>
> I do think it is worth to note in some developer's guide.
>
> However, I find it very natural that "JMeterThread state should not be
> altered outside of the thread".
> So any kind of "let's start the thread, then update some of its variables"
> looks like a creepy code. Even if that is not yet forbidden explicitly in
> the developer's guide.
Agreed.
> [1]
> http://shipilev.net/blog/2014/jmm-pragmatics/#_happens_before_publication
> [2] http://www.slideshare.net/VladimirSitnikv/final-field-semantics
>
> Vladimir Sitnikov
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Vladimir Sitnikov <si...@gmail.com>.
>> 2) Just synchronization on the same lock is not sufficient (see
>> "synchronizes-with" in the JMM)
>I don't follow that.
Here's example:
Thread 1:
// Globals.state is a static non-volatile field
Globals.state = new AtomicInteger(42); // 42 is stored in a volatile
field, you can check that in java sources
Thread 2:
System.out.println(Globals.state);
Thread 2 _can_ print 0 and it would be _acceptable_ by JMM (including JMM8).
This is an example that shows that "not every volatile read synchronizes
with volatile write".
There's a basic explanation: if a reader happened to "perform
synchronization" earlier than writer, then no happens-before appears.
More advanced one: to make things safe, you need happens-before.
"synchronization on the lock" does not induce happens before. "unlock of a
monitor happens-before _subsequent_ lock of the same monitor".
This is why I say: "synchronization in execute" does nothing. There is no
way this synchronization ensures that this method would be executed after
the init method.
See more detailed explanation in [1]
> (* apart from final, but I did write "mutable fields", which excludes
final)
Even mutable field can be safely published if it is wrapped in a object
with a final field.
For instance, java.lang.String is always safe, however it contains char[]
that is both mutable and unsafe.
Even if you store java.lang.String to a mutable field, it will still be
completely safe.
Nothing is specific to String, you can do the same with your own classes
and still get safe results (see [2])
>However these are external (*) to the class; changes to the app design
>may invalidate any such assumptions.
Very true, however you must state threading contract in this kind of
classes.
JMeter is multithreaded by its nature, thus I would expect "guidelines for
writing a custom function" to be accompanied by a JMM semantics that is
ensured by JMeter engine.
For instance, "execute function might be called from multiple
threads simultaneously, however init is guaranteed to happens-before any
call to execute".
Don't you mean that threading semantics of JMeter can change at any point
in time?
> OK, but as already written, that is something over which the class has no
control.
I did review some of the threading code in JMeter and I did notice some
code that already relied on the discussed JMeterThread.start happens-before.
I do think it is worth to note in some developer's guide.
However, I find it very natural that "JMeterThread state should not be
altered outside of the thread".
So any kind of "let's start the thread, then update some of its variables"
looks like a creepy code. Even if that is not yet forbidden explicitly in
the developer's guide.
[1]
http://shipilev.net/blog/2014/jmm-pragmatics/#_happens_before_publication
[2] http://www.slideshare.net/VladimirSitnikv/final-field-semantics
Vladimir Sitnikov
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by sebb <se...@gmail.com>.
On 16 October 2014 16:32, Vladimir Sitnikov <si...@gmail.com> wrote:
>>JMeter was originally written for Java
>>1.4 (or earlier); I think volatile did not offer the safe publication
>>guarantee then.
>
> That is correct, however see below.
>
>> No, because the Java Memory Model only guarantees safe publication of
>> a mutable field if the writer and reader synchronize on the same lock.
>
> This is plain wrong. Here are a couple of "whys":
> 1) There are lots of cases when "safe publication" is guaranteed without
> synchronization and/or volatiles (thread start/stop, final fields, etc)
OK, perhaps "only" is too strong.
However these are external (*) to the class; changes to the app design
may invalidate any such assumptions.
(* apart from final, but I did write "mutable fields", which excludes final)
> 2) Just synchronization on the same lock is not sufficient (see
> "synchronizes-with" in the JMM)
I don't follow that.
> See 17.4.5 of the java language specification [1]: A call to start() on a
> thread happens-before any actions in the started thread.
Yes, I knew that.
> Since JMeterThreads are started after all the variables are set/cloned (as
> in program order of the main thread), then you are guaranteed to see a safe
> view when running in JMeterThread.
OK, but as already written, that is something over which the class has
no control.
Since it is something that could potentially change, this assumption
would need to be carefully documented
> This is safe even without volatiles and synchronization, even in java 1.4.
>
> [1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.5
>
> Vladimir Sitnikov
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by Vladimir Sitnikov <si...@gmail.com>.
>JMeter was originally written for Java
>1.4 (or earlier); I think volatile did not offer the safe publication
>guarantee then.
That is correct, however see below.
> No, because the Java Memory Model only guarantees safe publication of
> a mutable field if the writer and reader synchronize on the same lock.
This is plain wrong. Here are a couple of "whys":
1) There are lots of cases when "safe publication" is guaranteed without
synchronization and/or volatiles (thread start/stop, final fields, etc)
2) Just synchronization on the same lock is not sufficient (see
"synchronizes-with" in the JMM)
See 17.4.5 of the java language specification [1]: A call to start() on a
thread happens-before any actions in the started thread.
Since JMeterThreads are started after all the variables are set/cloned (as
in program order of the main thread), then you are guaranteed to see a safe
view when running in JMeterThread.
This is safe even without volatiles and synchronization, even in java 1.4.
[1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.5
Vladimir Sitnikov
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by sebb <se...@gmail.com>.
Please create a Bugzilla issue.
On 16 October 2014 16:04, UBIK LOAD PACK Support
<su...@ubikloadpack.com> wrote:
> Hi sebb,
> Thanks for answers.
>
> This should be investigated as the gain of removing synchronized and
> switching to volatile is huge 25% more throughput on Javascript function
> for example with 10 threads. The more you add threads the more you gain.
> And we have customers reporting locking due to _javascript , Thread dump
> shows 1 thread blocking others on execute.
>
> Regards
>
>
> On Thu, Oct 16, 2014 at 4:55 PM, sebb <se...@gmail.com> wrote:
>
>>
>> On 14 October 2014 19:57, UBIK LOAD PACK Support
>> <su...@ubikloadpack.com> wrote:
>> > Hi,
>> > Reviewing code of Functions in JMeter we don't understand why Functions
>> > that only have values as instance variable (so Beanshell or
>> StringFromFile
>> > or IterationCounter (which could be improved to avoid it) are not
>> > concerned) synchronize on execute.
>> >
>> > It appears "values" instance variable is initialized within
>> > StandardJMeterEngine thread and then only accessed in read mode by
>> > JMeterThread
>>
>> i.e. it is written in one thread and read in another
>>
>> > But it is true that if synchronized is removed then CompoundVariable is
>> > accessed by multi threads but it seems it uses thread related data so it
>> > should be OK.
>>
>> No, because the Java Memory Model only guarantees safe publication of
>> a mutable field if the writer and reader synchronize on the same lock.
>> Or the field must be volatile. JMeter was originally written for Java
>> 1.4 (or earlier); I think volatile did not offer the safe publication
>> guarantee then.
>>
>> It does look like the values field could now be made volatile, which
>> would mean the synch. was not needed to ensure safe pub.
>> However, it might still be needed for other fields or mutual exclusion
>> purposes.
>>
>> >
>> >
>> > --
>>
>
>
>
> --
>
> Regards
> Ubik Load Pack <http://ubikloadpack.com> Team
> Follow us on Twitter <http://twitter.com/ubikloadpack>
>
>
> Cordialement
> L'équipe Ubik Load Pack <http://ubikloadpack.com>
> Suivez-nous sur Twitter <http://twitter.com/ubikloadpack>
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by UBIK LOAD PACK Support <su...@ubikloadpack.com>.
Hi sebb,
Thanks for answers.
This should be investigated as the gain of removing synchronized and
switching to volatile is huge 25% more throughput on Javascript function
for example with 10 threads. The more you add threads the more you gain.
And we have customers reporting locking due to _javascript , Thread dump
shows 1 thread blocking others on execute.
Regards
On Thu, Oct 16, 2014 at 4:55 PM, sebb <se...@gmail.com> wrote:
>
> On 14 October 2014 19:57, UBIK LOAD PACK Support
> <su...@ubikloadpack.com> wrote:
> > Hi,
> > Reviewing code of Functions in JMeter we don't understand why Functions
> > that only have values as instance variable (so Beanshell or
> StringFromFile
> > or IterationCounter (which could be improved to avoid it) are not
> > concerned) synchronize on execute.
> >
> > It appears "values" instance variable is initialized within
> > StandardJMeterEngine thread and then only accessed in read mode by
> > JMeterThread
>
> i.e. it is written in one thread and read in another
>
> > But it is true that if synchronized is removed then CompoundVariable is
> > accessed by multi threads but it seems it uses thread related data so it
> > should be OK.
>
> No, because the Java Memory Model only guarantees safe publication of
> a mutable field if the writer and reader synchronize on the same lock.
> Or the field must be volatile. JMeter was originally written for Java
> 1.4 (or earlier); I think volatile did not offer the safe publication
> guarantee then.
>
> It does look like the values field could now be made volatile, which
> would mean the synch. was not needed to ensure safe pub.
> However, it might still be needed for other fields or mutual exclusion
> purposes.
>
> >
> >
> > --
>
--
Regards
Ubik Load Pack <http://ubikloadpack.com> Team
Follow us on Twitter <http://twitter.com/ubikloadpack>
Cordialement
L'équipe Ubik Load Pack <http://ubikloadpack.com>
Suivez-nous sur Twitter <http://twitter.com/ubikloadpack>
Re: Why do Functions that only have values as instance variable
synchronize execute ?
Posted by sebb <se...@gmail.com>.
On 14 October 2014 19:57, UBIK LOAD PACK Support
<su...@ubikloadpack.com> wrote:
> Hi,
> Reviewing code of Functions in JMeter we don't understand why Functions
> that only have values as instance variable (so Beanshell or StringFromFile
> or IterationCounter (which could be improved to avoid it) are not
> concerned) synchronize on execute.
>
> It appears "values" instance variable is initialized within
> StandardJMeterEngine thread and then only accessed in read mode by
> JMeterThread
i.e. it is written in one thread and read in another
> But it is true that if synchronized is removed then CompoundVariable is
> accessed by multi threads but it seems it uses thread related data so it
> should be OK.
No, because the Java Memory Model only guarantees safe publication of
a mutable field if the writer and reader synchronize on the same lock.
Or the field must be volatile. JMeter was originally written for Java
1.4 (or earlier); I think volatile did not offer the safe publication
guarantee then.
It does look like the values field could now be made volatile, which
would mean the synch. was not needed to ensure safe pub.
However, it might still be needed for other fields or mutual exclusion purposes.
>
>
> --