On 5/11/2015 8:18 PM, Thomas Stüfe wrote:
Hi David,
looks good and works fine on AIX and Linux Power.
We could now get rid of the thread_<os>_inline.hpp files too, right?
Right. I was waiting to see if anyone would comment on that ("leave them
in just in case we need some os-specific thread stuff ..."). But I will
go ahead and remove them before the official RFR.
Thanks,
David
Kind Regards, Thomas
On Thu, Nov 5, 2015 at 5:36 AM, David Holmes <david.hol...@oracle.com
<mailto:david.hol...@oracle.com>> wrote:
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>
<mailto: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
