<div dir="ltr">Hi David,<div><br></div><div>looks good and works fine on AIX and Linux Power.</div><div><br></div><div>We could now get rid of the thread_<os>_inline.hpp files too, right?</div><div><br></div><div>Kind Regards, Thomas</div><div><br></div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 5, 2015 at 5:36 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Here's updated webrev:<br>
<br>
<a href="http://cr.openjdk.java.net/~dholmes/8132510/webrev.v3/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~dholmes/8132510/webrev.v3/</a><br>
<br>
Changes since v2:<br>
<br>
- include Thomas's AIX fixes<br>
- Add assertion for not-NULL into Thread::current()<br>
- 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().<br>
- Removed Thread::current_noinline() (it was only used in an assert in some G1 code, so the inline-or-not seems irrelevant)<br>
- Made Thread::clear_thread_current() private<br>
<br>
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.<br>
<br>
I still need some assistance from Aarch64 folk to write their get_thread function please!<br>
<br>
I still have footprint and performance measurements to make before proposing formal RFR.<br>
<br>
I also am still to determine whether to include the ability to hook in a pthread_ based implementation instead.<br>
<br>
Thanks,<br>
David<div class="HOEnZb"><div class="h5"><br>
<br>
On 4/11/2015 7:26 PM, David Holmes wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 4/11/2015 6:17 PM, Thomas Stüfe wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi David,<br>
<br>
On Wed, Nov 4, 2015 at 5:08 AM, David Holmes <<a href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a><br>
<mailto:<a href="mailto:david.holmes@oracle.com" target="_blank">david.holmes@oracle.com</a>>> wrote:<br>
<br>
    Hi Thomas,<br>
<br>
<br>
        One question about your changes:<br>
<br>
        Before, Thread::current() would assert instead of returning<br>
NULL if<br>
        called before Thread::initialize_thread_current() or after<br>
        Thead::clear_thread_current() . Now, we just return NULL. Is<br>
        this intended?<br>
<br>
<br>
    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<br>
    returning NULL because it can be intentionally called when a thread<br>
    is not attached. That means we won't directly catch calls to<br>
    Thread::current() from code that doesn't realize it is calling it<br>
    "too soon" - though there do exist numerous assertions in the<br>
    callers of Thread::current() that check the result is not NULL.<br>
<br>
    I could add the assert to Thread::current() and also add<br>
    Thread::current_or_null() to be used by code that needs to use it to<br>
    check for attachment (ie JNI code). I'd also have to examine all the<br>
    changed ThreadLocalStorage::thread/get_thread_slow call-sites to see<br>
    if any of those legitimately expect the thread may not be attached.<br>
<br>
    What do you think?<br>
<br>
<br>
I would prefer having Thread::current() to assert and to have a<br>
Thread::current_or_null() for cases where NULL could occurr. I tend to<br>
hit that assert a lot in development, it is useful. And the<br>
non-asserting version gets already used in a number of places, also in<br>
our (not OpenJDK) coding.<br>
</blockquote>
<br>
Yes I agree. Most of the TLS::thread() and TLS::get_thread_slow() should<br>
actually call Thread::current_or_null(). I also found a couple of<br>
existing Thread::current()'s that should be current_or_null(). :)<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
    I also need to look at the location of Thread::current in the .hpp<br>
    file rather than .inline.hpp and reconcile that with comments<br>
    regarding the noinline version (which is only used in<br>
    g1HotCardCache.hpp).<br>
<br>
<br>
Could we leave just the inline version in thread.hpp and remove the<br>
noinline version altogether? Now that Thread::current() is very simple,<br>
we may just as well keep it in the class body like the other accessors.<br>
</blockquote>
<br>
I'll see if the g1 code can tolerate that.<br>
<br>
I'll update a prepare a new webrev tomorrow.<br>
<br>
Thanks,<br>
David<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks, Thomas<br>
<br>
<br>
</blockquote></blockquote>
</div></div></blockquote></div><br></div></div>