On 4/11/2015 6:17 PM, Thomas Stüfe wrote:
Hi David,
On Wed, Nov 4, 2015 at 5:08 AM, David Holmes <[email protected]
<mailto:[email protected]>> 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
