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 <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?
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
