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

[David Holmes]

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.holmes at oracle.com
> <mailto: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>
>             <mailto: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
>
>
>