Here's updated webrev:
http://cr.openjdk.java.net/~dholmes/8132510/webrev.v3/
Changes since v2:
- include Thomas's AIX fixes
- Add assertion for not-NULL into Thread::current()
- Add Thread::current_or_null() for when NULL can be expected, or for
where failing an assert would cause more problems (eg crash reporting).
Most uses of ThreadLocalStorage::thread()/get_thread_slow() now call
current_or_null().
- Removed Thread::current_noinline() (it was only used in an assert in
some G1 code, so the inline-or-not seems irrelevant)
- Made Thread::clear_thread_current() private
I'm debating whether the get_thread implementations should call
Thread::current() or Thread::current_or_null(). We should never get NULL
but seems unnecessary overhead to check that with an assert in this
code. Opinions welcomed.
I still need some assistance from Aarch64 folk to write their get_thread
function please!
I still have footprint and performance measurements to make before
proposing formal RFR.
I also am still to determine whether to include the ability to hook in a
pthread_ based implementation instead.
Thanks,
David
On 4/11/2015 7:26 PM, David Holmes wrote:
On 4/11/2015 6:17 PM, Thomas Stüfe wrote:
Hi David,
On Wed, Nov 4, 2015 at 5:08 AM, David Holmes <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>> wrote:
Hi Thomas,
One question about your changes:
Before, Thread::current() would assert instead of returning
NULL if
called before Thread::initialize_thread_current() or after
Thead::clear_thread_current() . Now, we just return NULL. Is
this intended?
Ah great question ... so before we have a mix of calls to:
- Thread::current() (asserts on NULL as does JavaThread::current)
- ThreadLocalStorage::thread() (can return NULL)
- ThreadLocalStorage::get_thread_slow() (can return NULL)
and now we only have Thread::current() which means we have to allow
returning NULL because it can be intentionally called when a thread
is not attached. That means we won't directly catch calls to
Thread::current() from code that doesn't realize it is calling it
"too soon" - though there do exist numerous assertions in the
callers of Thread::current() that check the result is not NULL.
I could add the assert to Thread::current() and also add
Thread::current_or_null() to be used by code that needs to use it to
check for attachment (ie JNI code). I'd also have to examine all the
changed ThreadLocalStorage::thread/get_thread_slow call-sites to see
if any of those legitimately expect the thread may not be attached.
What do you think?
I would prefer having Thread::current() to assert and to have a
Thread::current_or_null() for cases where NULL could occurr. I tend to
hit that assert a lot in development, it is useful. And the
non-asserting version gets already used in a number of places, also in
our (not OpenJDK) coding.
Yes I agree. Most of the TLS::thread() and TLS::get_thread_slow() should
actually call Thread::current_or_null(). I also found a couple of
existing Thread::current()'s that should be current_or_null(). :)
I also need to look at the location of Thread::current in the .hpp
file rather than .inline.hpp and reconcile that with comments
regarding the noinline version (which is only used in
g1HotCardCache.hpp).
Could we leave just the inline version in thread.hpp and remove the
noinline version altogether? Now that Thread::current() is very simple,
we may just as well keep it in the class body like the other accessors.
I'll see if the g1 code can tolerate that.
I'll update a prepare a new webrev tomorrow.
Thanks,
David
Thanks, Thomas
