On Tue, 11 May 2021 17:08:30 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 incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains two additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into 8265753
>> - Removed manual transitions
>
> src/hotspot/share/prims/jvmtiRawMonitor.hpp line 42:
>
>> 40: // Important note:
>> 41: // Raw monitors can be used in callbacks which happen during safepoint
>> by the VM
>> 42: // thread (e.g. heapRootCallback). This means we may not
>> tranisition/safepoint
>
> nit typo: s/e.g./e.g.,/
> nit typo: s/tranisition/transition/
Fixed
> src/hotspot/share/prims/jvmtiRawMonitor.hpp line 43:
>
>> 41: // Raw monitors can be used in callbacks which happen during safepoint
>> by the VM
>> 42: // thread (e.g. heapRootCallback). This means we may not
>> tranisition/safepoint
>> 43: // poll in many cases, else the agent JavaThread can deadlock with VM
>> thread,
>
> nit typo: s/VM thread/the VM thread/
Fixed
> src/hotspot/share/prims/jvmtiRawMonitor.hpp line 52:
>
>> 50: // native for all operations. However we need to honor suspend request,
>> not
>> 51: // entering a monitor if suspended, and check for interrupts. Honoring
>> suspened
>> 52: // and reading interrupt flag must be done from VM state (a safepoint
>> unsafe
>
> nit typo: s/suspend request/a suspend request/
> nit typo: s/Honoring suspened/Honoring a suspend request/
> nit typo: s/interrupt flag/the interrupt flag/
> (You'll probably want to reformat the changed lines a bit.)
Fixed
> src/hotspot/share/runtime/interfaceSupport.inline.hpp line 206:
>
>> 204: ~ThreadInVMfromNative() {
>> 205: assert(!_thread->has_last_Java_frame() ||
>> _thread->frame_anchor()->walkable(), "Unwalkable stack in vm->native
>> transition");
>> 206: _thread->set_thread_state(_thread_in_native);
>
> You're losing the assertion that we're transitioning from
> `_thread_in_vm` to `_thread_in_native` here. Although,
> the constructor did verify that we were in `_thread_in_native`
> when we transitioned to `_thread_in_vm` so at least the
> first half is still verified.
>
> Can you an assertion that we're in `_thread_in_vm` here?
Fixed
> src/hotspot/share/runtime/objectMonitor.hpp line 309:
>
>> 307: protected:
>> 308: ObjectMonitor* _om;
>> 309: bool _om_exit;
>
> Instead of `_om_exit` maybe this should be more generic?
> Perhaps `_om_op_done` or something like that?
Fixed
-------------
PR: https://git.openjdk.java.net/jdk/pull/3875
