(L) Prelim RFR: 8132510: Replace ThreadLocalStorage with compiler/language-based thread-local variables
david.holmes at oracle.com
Thu Nov 5 22:58:51 UTC 2015
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.
> 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:
> 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 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.
> 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
> 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
> - 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
> 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
> 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