David, It looks good and safe. Is there anything to do for the closed code as well? Thanks, Serguei On 5/10/16 19:17, David Holmes wrote:
On 11/05/2016 6:28 AM, Brian Gardner wrote:
I really like the rename of java_start to native_thread_entry, it makes things make more sense. It looks like os_bsd.cpp and os_linux.cpp are both missing the clear_thread_current logic after thread->run().
Doh! Thanks. That's what I get for trying to multi-task :)
Webrev updated in place.
Can I get another review from someone in hotspot please. Dan is unfortunately (for me) away.
Thanks, David
+ // If a thread has not deleted itself ("delete this") as part of its + // termination sequence, we have to ensure thread-local-storage is + // cleared before we actually terminate. No threads should ever be + // deleted asynchronously with respect to their termination. + if (Thread::current_or_null_safe() != NULL) { + assert(Thread::current_or_null_safe() == thread, "current thread is wrong"); + thread->clear_thread_current(); + } +
On May 9, 2016, at 5:33 PM, David Holmes <david.holmes@oracle.com <mailto:david.holmes@oracle.com>> wrote:
Okay here is version 2:
http://cr.openjdk.java.net/~dholmes/8154715/webrev.v2/
Lots of cosmetic changes but only a couple of functional ones:
- After thread->run() returns we clear the TLS by calling clear_thread_current(), but only for threads where it has not already been cleared - as those threads may already have been deleted so we can't dereference 'thread'
- No asynchronous thread deletion is permitted, and we avoid races with VM termination. This means the VMThread no longer gets deleted - that should not be an issue as many threads do not get deleted when the VM terminates. I added destructors for the VMThread and WatcherThread so anyone introducing their deletion is informed by a guarantee(false)
Cosmetic changes:
- renamed java_start to thread_native_entry (it is used by all threads not just "java" ones, so this avoids potential confusion)
- updated os::free_thread to always assume it works on the current thread (and add assert to verify that)
Thanks, David