(L) Prelim RFR: 8132510: Replace ThreadLocalStorage with compiler/language-based thread-local variables
david.holmes at oracle.com
Thu Nov 5 04:36:32 UTC 2015
Here's updated webrev:
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
- 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
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.
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.holmes at oracle.com
>> <mailto:david.holmes at 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
>> 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, Thomas
More information about the hotspot-dev