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.hol...@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.hol...@oracle.com >>> <mailto:david.hol...@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 >>> >>> >>>
