On Wed, 21 Oct 2020 16:54:48 GMT, Daniel D. Daugherty <dcu...@openjdk.org>
wrote:
>> 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
>
> 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 */`.
Fixed
> 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.
Fixed
> 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.
Long story:
Before async handshakes was integrate I had a patch which does the same as this.
This change was accidentally slipped into async handshakes change-set and was
integrated.
Richard notice this, I told him he could revert it in his change-set for EB, so
he did.
But now we need this change, so here it comes once more!
VM thread is allowed to execute these handshakes, thus when calling
check_top_frame() from SetForceEarlyReturn::doit() it's just a Thread*.
> 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 */`.
Fixed
> 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");`
Fixed.
> 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/
Fixed
> 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...
Fixed
> 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 */`.
Fixed
> 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 */`.
Fixed
> 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`??
Fixed
> 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.
I just delete the no longer relevant parts.
-------------
PR: https://git.openjdk.java.net/jdk/pull/729
