On Wed, 21 Oct 2020 08:40:47 GMT, Robbin Ehn <r...@openjdk.org> wrote:
>> The main point of this change-set is to make it easier to implement S/R on
>> top of handshakes.
>> Which is a prerequisite for removing _suspend_flag (which duplicates the
>> handshake functionality).
>> But we also remove some complicated S/R methods.
>>
>> We basically just put in everything in the handshake closure, so the diff
>> just looks much worse than what it is.
>>
>> TraceSuspendDebugBits have an ifdef, but in both cases it now just returns.
>> But I was unsure if I should remove now or when is_ext_suspend_completed()
>> is removed.
>>
>> Passes multiple t1-5 runs, locally it passes many
>> jck:vm/nsk_jvmti/nsk_jdi/jdk-jdi runs.
>
> Robbin Ehn has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains seven commits:
>
> - Fixed merge miss
> - Merge branch 'master' into
> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
> - Merge fix from Richard
> - Merge branch 'master' into
> 8223312-Utilize-handshakes-instead-of-is_thread_fully_suspended
> - Removed TraceSuspendDebugBits
> - Removed unused method is_ext_suspend_completed_with_lock
> - Utilize handshakes instead of is_thread_fully_suspended
I don't think I have any "must fix" comments here.
I'm going to assume that my confusion about why there
is code from @reinrich's EscapeBarrier work here is
because of the merging of conflicts...
src/hotspot/share/prims/jvmtiEnv.cpp line 1646:
> 1644: // java_thread - pre-checked
> 1645: jvmtiError
> 1646: JvmtiEnv::PopFrame(JavaThread* java_thread) {
So I'm a bit confused why I'm seeing PopFrame() changes here that are
related to @reinrich's EscapeBarrier work. I've seen mention of picking
up a patch during this review from @reinrich so may that's why. I don't
see anything wrong with the changes, but I am confused why they are
here in this review.
src/hotspot/share/prims/jvmtiEnv.cpp line 1715:
> 1713: }
> 1714:
> 1715: SetFramePopClosure op(this, state, depth);
The new closure is `SetFramePopClosure`, but the function
we are in is `NotifyFramePop()` so that seems like a mismatch.
Update: Okay, that just a move of existing code so this "mismatch"
is pre-existing.
src/hotspot/share/prims/jvmtiEnv.cpp line 1718:
> 1716: MutexLocker mu(JvmtiThreadState_lock);
> 1717: if (java_thread == JavaThread::current()) {
> 1718: op.doit(java_thread, true);
Please add a comment after the `true` parameter to
indicate the name of the doit() function's parameter,
e.g., `true /* self */`.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 56:
> 54: #include "runtime/threadSMR.hpp"
> 55: #include "runtime/vframe.hpp"
> 56: #include "runtime/vframe.inline.hpp"
When you add `foo.inline.hpp` you delete `foo.hpp` because
the `foo.inline.hpp` file always includes the `foo.hpp` file.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1311:
> 1309: // It is to keep a ret_ob_h handle alive after return to the caller.
> 1310: jvmtiError
> 1311: JvmtiEnvBase::check_top_frame(Thread* current_thread, JavaThread*
> java_thread,
Again, it is not clear why these changes to `check_top_frame` are here since
they
appear to be related to @reinrich's work.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1398:
> 1396: SetForceEarlyReturn op(state, value, tos);
> 1397: if (java_thread == current_thread) {
> 1398: op.doit(java_thread, true);
Please add a comment after the true parameter to
indicate the name of the doit() function's parameter,
e.g., `true /* self */`.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1570:
> 1568:
> 1569: ResourceMark rm(current_thread);
> 1570: // Check if there are more than one Java frame in this thread, that
> the top two frames
typo: s/are more/is more/
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1543:
> 1541: HandleMark hm(current_thread);
> 1542: JavaThread* java_thread = target->as_Java_thread();
> 1543:
This would be useful here:
`assert(_state->get_thread() == java_thread, "Must be");`
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1642:
> 1640:
> 1641: if (!self) {
> 1642: if (!java_thread->is_external_suspend()) {
You could join these two if-statements with `&&` and have
one less indenting level...
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1661:
> 1659: assert(vf->frame_pointer() != NULL, "frame pointer mustn't be NULL");
> 1660: if (java_thread->is_exiting() || java_thread->threadObj() == NULL) {
> 1661: return;
What's the `_result` value if this `return` executes?
src/hotspot/share/prims/jvmtiEnvBase.hpp line 361:
> 359: _tos(tos) {}
> 360: void do_thread(Thread *target) {
> 361: doit(target, false);
Please add a comment after the true parameter to
indicate the name of the doit() function's parameter,
e.g., `false /* self */`.
src/hotspot/share/prims/jvmtiEnvBase.hpp line 395:
> 393: _depth(depth) {}
> 394: void do_thread(Thread *target) {
> 395: doit(target, false);
Please add a comment after the true parameter to
indicate the name of the doit() function's parameter,
e.g., `false /* self */`.
src/hotspot/share/runtime/deoptimization.cpp line 1755:
> 1753: thread->is_handshake_safe_for(Thread::current()) ||
> 1754: SafepointSynchronize::is_at_safepoint(),
> 1755: "can only deoptimize other thread at a safepoint");
Should that now be: `safepoint/handshake`??
src/hotspot/share/runtime/thread.cpp line 567:
> 565: //
> 566: // _thread_in_native -> _thread_in_native_trans -> _thread_blocked
> 567: //
This code should not be needed with the much simpler suspension mechanism.
(My agreement may come back to bite me if we have to change a suspend/resume
bug in the near future. :-) )
src/hotspot/share/runtime/thread.cpp line 698:
> 696: RememberProcessedThread rpt(this);
> 697: oops_do_no_frames(f, cf);
> 698: oops_do_frames(f, cf);
In the comment above:
// ... If we were
// called by wait_for_ext_suspend_completion(), then it
// will be doing the retries so we don't have to.
`wait_for_ext_suspend_completion()` has been deleted so the
comment needs work.
-------------
Marked as reviewed by dcubed (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/729
