Hi David, On Wed, Nov 4, 2015 at 5:08 AM, David Holmes <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. > 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. Thanks, Thomas >
