(L) Prelim RFR: 8132510: Replace ThreadLocalStorage with compiler/language-based thread-local variables

[Thomas Stüfe]

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?

Kind Regards, Thomas



On Thu, Nov 5, 2015 at 5:36 AM, David Holmes <david.holmes at 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.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
>>>     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
>>>
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/porters-dev/attachments/20151105/27776070/attachment.html>