You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by "maxxedit@gmail.com" <ma...@gmail.com> on 2011/01/21 04:27:59 UTC
bug in TC6 ContainerBase class?
In TC6 catalina core, ContainerBase class defines this field to stop
the background processor thread:
private boolean threadDone = false;
Shouldn't it be a volatile? Because the background processor refers to
a classloader, could it prevent a (weak) classloader from being gc'ed?
-m
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: bug in TC6 ContainerBase class?
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Christopher Schultz [mailto:chris@christopherschultz.net]
> Subject: Re: bug in TC6 ContainerBase class?
> Are you saying that effectively most method calls have
> pre- and post-memory barriers?
Essentially yes - except in the presence of a JIT. Since a JIT can examine the called method and analyze its behavior and consequent side effects, it can eliminate or reorder writes or reads in both the caller and callee that the original compiler generated. So as a general rule, it's not safe to depend on that behavior in Java. In the specific case under discussion, the methods called by the looping run() method are too complex for any current JIT technology to fully analyze.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
Re: bug in TC6 ContainerBase class?
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Chuck,
On 1/21/2011 10:02 AM, Caldarale, Charles R wrote:
>> From: sebb [mailto:sebbaz@gmail.com] Subject: Re: bug in TC6
>> ContainerBase class?
>
>> There is no requirement for the compiler to reload variables
>> unless the variable is volatile, or there is a shared lock between
>> threads.
>
> Correct - with one caveat: if the compiler cannot prove there is no
> synchronization, it must assume that there is. Also, the method
> calls can have side effects not discernible by the compiler,
> including updating of fields not specified in the argument list.
Are you saying that effectively most method calls have pre- and post-
memory barriers?
-chris
RE: bug in TC6 ContainerBase class?
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: sebb [mailto:sebbaz@gmail.com]
> Subject: Re: bug in TC6 ContainerBase class?
> Do you mean:
> * the compiler must assume that there is no synch, or
> * the compiler must assume that there is synch.
The latter; in the absence of information to the contrary, the compiler must assume the worst case - that synchronization can occur as a side effect of the method call.
> > Nope, it's required to be reloaded due to the constraints
> > mentioned above.
> If any reloading is required, it surely only applies to the
> current thread.
Correct. The one under discussion is the one with the "while (!threadDone)" loop.
> So as far as I can tell, calling non-synch. methods only affects
> compiler optimisation - it does not affect visibility of changes in
> other threads.
True - but the javac compiler cannot be certain that there's no synchronization going on inside the called methods or any nested method calls therein. A JIT could, in theory, have access to that information (and all other side effects), and optimize out non-volatile memory references, but the overhead of doing so is impractical.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: bug in TC6 ContainerBase class?
Posted by sebb <se...@gmail.com>.
On 21 January 2011 15:02, Caldarale, Charles R
<Ch...@unisys.com> wrote:
>> From: sebb [mailto:sebbaz@gmail.com]
>> Subject: Re: bug in TC6 ContainerBase class?
>
>> There is no requirement for the compiler to reload variables unless
>> the variable is volatile, or there is a shared lock between threads.
>
> Correct - with one caveat: if the compiler cannot prove there is no synchronization, it must assume that there is.
Do you mean:
* the compiler must assume that there is no synch, or
* the compiler must assume that there is synch.
I originally read your statement as the latter, but I think the former
is correct, i.e. the compiler cannot assume there is any synch.
> Also, the method calls can have side effects not discernible by the compiler, including updating of fields not specified in the argument list.
True.
>> In the absence of volatile or sync. on a shared lock there is no
>> guarantee that the updated threadDone variable will *ever* be seen by
>> the background thread.
>
> Nope, it's required to be reloaded due to the constraints mentioned above.
If any reloading is required, it surely only applies to the current thread.
The JMM only guarantees as-if-sequential semantics for each thread
independently,
So as far as I can tell, calling non-synch. methods only affects
compiler optimisation - it does not affect visibility of changes in
other threads.
>> In particular, in this case the compiler may notice that the
>> threadDone variable is loop-invariant, and haul it out of the loop;
>> see for example:
>>
>> http://java.dzone.com/articles/multithreading-and-java-memory
>>
>> In which case, the background thread run loop will never terminate.
>
> Note that in all the cited examples, there are no method calls inside the loop.
Point taken.
> - Chuck
>
>
> THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: bug in TC6 ContainerBase class?
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: sebb [mailto:sebbaz@gmail.com]
> Subject: Re: bug in TC6 ContainerBase class?
> There is no requirement for the compiler to reload variables unless
> the variable is volatile, or there is a shared lock between threads.
Correct - with one caveat: if the compiler cannot prove there is no synchronization, it must assume that there is. Also, the method calls can have side effects not discernible by the compiler, including updating of fields not specified in the argument list.
> In the absence of volatile or sync. on a shared lock there is no
> guarantee that the updated threadDone variable will *ever* be seen by
> the background thread.
Nope, it's required to be reloaded due to the constraints mentioned above.
> In particular, in this case the compiler may notice that the
> threadDone variable is loop-invariant, and haul it out of the loop;
> see for example:
>
> http://java.dzone.com/articles/multithreading-and-java-memory
>
> In which case, the background thread run loop will never terminate.
Note that in all the cited examples, there are no method calls inside the loop.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: bug in TC6 ContainerBase class?
Posted by sebb <se...@gmail.com>.
On 21 January 2011 06:36, Caldarale, Charles R
<Ch...@unisys.com> wrote:
>> From: sebb [mailto:sebbaz@gmail.com]
>> Subject: Re: bug in TC6 ContainerBase class?
>
>> What method calls will force a reload? Why must the compiler
>> assume it has changed?
>
> Any other than Thread.sleep(). Again, we are discussing what happens in practice, not in theory. Since the javac compiler does not, in this case, have knowledge of the internal operations of the called methods, it must pessimistically assume that synchronization may occur and fields have been updated. (On the other hand, a JIT, with its runtime knowledge, could determine whether or not synchronization happens and optimize out the reload. But that would require either calls to very simplistic methods or a very deeply probing JIT.) For methods that javac does have knowledge of (e.g., Thread.sleep(), as defined by 17.9 of the language spec, or non-overridable ones in the class), the compiler does not have to be pessimistic.
I really don't understand that.
The JMM specifically allows for caching of variables in local memory
or registers, and in the absence of synch. is only required to
maintain as-if-sequential semantics for a single thread. There is no
requirement for different threads to even maintain the same ordering,
so long as each thread appears to be be sequential.
There is no requirement for the compiler to reload variables unless
the variable is volatile, or there is a shared lock between threads.
So saying that the compiler must reload the value in the absence of
volatile/synch goes against one of the principles of the JMM, which is
to allow the compiler and JVM freedom to cache and re-order operations
to improve performance.
>> Surely that is what volatile is for?
>
> Volatile insures that ordering is observed in lieu of explicit synchronization.
Yes, it effectively ensures that the value written be one thread will
be seen by another that executes later.
In the absence of volatile or sync. on a shared lock there is no
guarantee that the updated threadDone variable will *ever* be seen by
the background thread.
In particular, in this case the compiler may notice that the
threadDone variable is loop-invariant, and haul it out of the loop;
see for example:
http://java.dzone.com/articles/multithreading-and-java-memory
In which case, the background thread run loop will never terminate.
> - Chuck
>
>
> THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: bug in TC6 ContainerBase class?
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: sebb [mailto:sebbaz@gmail.com]
> Subject: Re: bug in TC6 ContainerBase class?
> > but, to my knowledge, there's never been any instances
> > of that reported.
> But if it does happen, it will be a nuisance for all, so
> why not fix it now?
I have no objection to declaring the field to be volatile; my only problem is with the assumption that this could in any way affect observable behavior.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: bug in TC6 ContainerBase class?
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: sebb [mailto:sebbaz@gmail.com]
> Subject: Re: bug in TC6 ContainerBase class?
> What method calls will force a reload? Why must the compiler
> assume it has changed?
Any other than Thread.sleep(). Again, we are discussing what happens in practice, not in theory. Since the javac compiler does not, in this case, have knowledge of the internal operations of the called methods, it must pessimistically assume that synchronization may occur and fields have been updated. (On the other hand, a JIT, with its runtime knowledge, could determine whether or not synchronization happens and optimize out the reload. But that would require either calls to very simplistic methods or a very deeply probing JIT.) For methods that javac does have knowledge of (e.g., Thread.sleep(), as defined by 17.9 of the language spec, or non-overridable ones in the class), the compiler does not have to be pessimistic.
> Surely that is what volatile is for?
Volatile insures that ordering is observed in lieu of explicit synchronization.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: bug in TC6 ContainerBase class?
Posted by sebb <se...@gmail.com>.
On 21 January 2011 03:59, Caldarale, Charles R
<Ch...@unisys.com> wrote:
>> From: maxxedit@gmail.com [mailto:maxxedit@gmail.com]
>> Subject: bug in TC6 ContainerBase class?
>
>> In TC6 catalina core, ContainerBase class defines this field to stop
>> the background processor thread:
>>
>> private boolean threadDone = false;
>>
>> Shouldn't it be a volatile?
>
> In theory, the field should be marked volatile. In practice, it won't make any difference, since the method calls will force a reload of the value upon return and the compiler must assume that the value could have changed.
Huh? What method calls will force a reload? Why must the compiler
assume it has changed? Surely that is what volatile is for?
threadStart() is OK, because it sets threadDone before starting the
thread, and a thread start acts as a memory barrier.
> The worst that could happen is that the thread never sees threadDone as true, and thus would never terminate
Or it would take a while to be published to the other thread.
> but, to my knowledge, there's never been any instances of that reported.
But if it does happen, it will be a nuisance for all, so why not fix it now?
> - Chuck
>
>
> THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: bug in TC6 ContainerBase class?
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: maxxedit@gmail.com [mailto:maxxedit@gmail.com]
> Subject: bug in TC6 ContainerBase class?
> In TC6 catalina core, ContainerBase class defines this field to stop
> the background processor thread:
>
> private boolean threadDone = false;
>
> Shouldn't it be a volatile?
In theory, the field should be marked volatile. In practice, it won't make any difference, since the method calls will force a reload of the value upon return and the compiler must assume that the value could have changed. The worst that could happen is that the thread never sees threadDone as true, and thus would never terminate - but, to my knowledge, there's never been any instances of that reported.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: bug in TC6 ContainerBase class?
Posted by sebb <se...@gmail.com>.
On 21 January 2011 15:52, Konstantin Kolinko <kn...@gmail.com> wrote:
> 2011/1/21 maxxedit@gmail.com <ma...@gmail.com>:
>> In TC6 catalina core, ContainerBase class defines this field to stop
>> the background processor thread:
>>
>> private boolean threadDone = false;
>>
>> Shouldn't it be a volatile? Because the background processor refers to
>> a classloader, could it prevent a (weak) classloader from being gc'ed?
>>
>
> Reading Chapter 17 of JLS 3rd edition, that defines the current (aka
> "new") memory model [1], Ch. 17.9 "Sleep and Yield" explicitly
> mentions that sleep does not have "any synchronization semantics", but
> interrupt() has synchronization semantics per Ch.17.4.4.
>
> I think that using interrupt() is the reason why no issue have been
> observed here and why "volatile" is not necessary. (Though I wouldn't
> object if "volatile" were added to that field.)
>
> [1] http://java.sun.com/docs/books/jls/third_edition/html/memory.html
>
Thanks, very useful.
I suggest either adding volatile or at least adding a comment to say
that interrupt() provides the necessary synch.
>
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: bug in TC6 ContainerBase class?
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/1/21 maxxedit@gmail.com <ma...@gmail.com>:
> In TC6 catalina core, ContainerBase class defines this field to stop
> the background processor thread:
>
> private boolean threadDone = false;
>
> Shouldn't it be a volatile? Because the background processor refers to
> a classloader, could it prevent a (weak) classloader from being gc'ed?
>
Reading Chapter 17 of JLS 3rd edition, that defines the current (aka
"new") memory model [1], Ch. 17.9 "Sleep and Yield" explicitly
mentions that sleep does not have "any synchronization semantics", but
interrupt() has synchronization semantics per Ch.17.4.4.
I think that using interrupt() is the reason why no issue have been
observed here and why "volatile" is not necessary. (Though I wouldn't
object if "volatile" were added to that field.)
[1] http://java.sun.com/docs/books/jls/third_edition/html/memory.html
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org