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

[David Holmes]

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
>
>