<div dir="ltr">Hi David,<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 4, 2015 at 5:08 AM, David Holmes <span dir="ltr"><<a href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Thomas,<span class=""><br>
<br></span><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br>
One question about your changes:<br>
<br>
Before, Thread::current() would assert instead of returning NULL if<br>
called before Thread::initialize_thread_current() or after<br>
Thead::clear_thread_current() . Now, we just return NULL. Is this intended?<br>
</blockquote>
<br></span>
Ah great question ... so before we have a mix of calls to:<br>
<br>
- Thread::current()  (asserts on NULL as does JavaThread::current)<br>
- ThreadLocalStorage::thread() (can return NULL)<br>
- ThreadLocalStorage::get_thread_slow() (can return NULL)<br>
<br>
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.<br>
<br>
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.<br>
<br>
What do you think?<br>
<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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).<br>
<br></blockquote><div><br></div><div>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. </div><div><br></div><div>Thanks, Thomas</div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br></blockquote></div></div></div>