On Wed, 9 Sep 2020 14:06:02 GMT, Kim Barrett <kbarr...@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? Note: I included the link the webrev had me on, but I have no idea what file that is... 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. 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? 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. The craziness in the JavaThread::pd_get_top_frame() functions made my head hurt. Thanks for cleaning that up! ------------- PR: https://git.openjdk.java.net/jdk/pull/37
