RFR: 8252406: Introduce Thread::as_Java_thread() convenience function [v7]

[David Holmes]

Hi Dan,

Thanks for looking at this one.

On 10/09/2020 9:26 am, Daniel D.Daugherty wrote:
> On Wed, 9 Sep 2020 14:06:02 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
> 
>>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    Reverted to the original code as the explicit assertion is preferred.
>>
>> Marked as reviewed by kbarrett (Reviewer).
> 
> This is a really nice set of cleanup changes.
> 
> I have a few comments.
> 
> https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-33
>    51   if (thread->is_Java_thread())
>    52     return thread->as_Java_thread()->thread_state() == _thread_in_vm;
>    53   else
>    54     return thread->is_VM_thread();
>        nit - this code needs braces. Not your bug, but since you've touched this
>        code, do you mind fixing it?

Yes will add braces.

So glad you picked on this though as I messed up one of my commits and 
rolled back to the v1 version, forgetting that it was broken in v1. The 
original line is:

     return true;  //something like this: thread->is_VM_thread();

so I tried:

     return thread->is_VM_thread();

but it causes the assertion to fail as GC threads also execute this. So 
I've restored to the original "return true;" but updated the comment.

> Note: I included the link the webrev had me on, but I have no idea what
> file that is...

Yeah those links are pretty obscure, and even the webrev frames view it 
takes you to doesn't give the name of the file. :(

> https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-80
> 
> 276   guarantee(current == get_thread() || current == get_thread()->active_handshaker(),
> 277                     "must be current thread or direct handshake");
> 
>      nit - the indent on L277 looks wrong in the webrev, but it looks right in
>      this paste. I don't know which is telling the truth.

It was wrong - fixed.

> https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-101
> 
>   358     this->as_Java_thread()->set_stack_overflow_limit();
>   359     this->as_Java_thread()->set_reserved_stack_activation(stack_base());
>      nit - do you really need 'this->' here?

Nope. Fixed.

> https://openjdk.github.io/cr/?repo=jdk&pr=37&range=06#frames-107
> 
> old code:
> 2074   if (thread_id == 0) {
> 2075     // current thread
> 2076     if (THREAD->is_Java_thread()) {
> 2077       return ((JavaThread*)THREAD)->cooked_allocated_bytes();
> 2078     }
> 2079     return -1;
> 
> new code:
> 2074   if (thread_id == 0) { // current thread
> 2075     return thread->cooked_allocated_bytes();
> 
> If the calling thread is not a JavaThread and passes a thread_id ==0,
> I don't think the returns are equivalent.

This code is in a JVM_ENTRY - so both THREAD and thread refer to 
JavaThread::current(), so we can never hit the "return -1;".

> The craziness in the JavaThread::pd_get_top_frame() functions made my head hurt.
> Thanks for cleaning that up!

Thanks again for the review. v8 will be appearing shortly.

David

> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/37
>