RFR: 8262910: Cleanup THREAD/TRAPS/naming and typing issues in ObjectMonitor and related code [v4]

[David Holmes]

Hi Serguei,

Thanks for taking a look.

On 10/03/2021 9:53 am, Serguei Spitsyn wrote:
> On Tue, 9 Mar 2021 18:52:27 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:
> 
>>> David Holmes has updated the pull request incrementally with three additional commits since the last revision:
>>>
>>>   - Fix typo
>>>   - Fixed up BiasedLocking code in ObjectSynchronizer::enter
>>>   - iFixed alignment in macro
>>
>> JVMTI changes look good.
> 
> Hi David,
> 
> Nice unification, looks great.
> A couple of comments.
> 
> src/hotspot/share/runtime/synchronizer.cpp
> 
>   638 int ObjectSynchronizer::wait(Handle obj, jlong millis, TRAPS) {
>   639   JavaThread* current = THREAD->as_Java_thread();
>   . . .
>   652   DTRACE_MONITOR_WAIT_PROBE(monitor, obj(), current, millis);
>   653   monitor->wait(millis, true, THREAD); // Not CHECK as we need following code
> 
> Is it intentional to use `THREAD` instead of `current` at line 653?

Yes, purely as a documentation aid - the "THREAD" usage indicates the 
wait() can lead to an exception. (Once TRAPS is defined using JavaThread 
the "current" variable won't be needed.)

> 1141 bool ObjectSynchronizer::request_deflate_idle_monitors() {
> 1142   Thread* current = Thread::current();
>   . . .
> 1158     if (current->is_Java_thread()) {
> 1159       // JavaThread has to honor the blocking protocol.
> 1160       ThreadBlockInVM tbivm(current->as_Java_thread());
>   . . .
>   
>   Would it better to define `current` as at the line 639?
>   There can be similar cases, e.g. the `deflate_idle_monitors()`.

I'm not sure what you mean - the current thread in these calls need not 
be a JavaThread ... hmmm actually ... since  JDK-8254029 
request_deflate_idle_monitors is only used by WhiteBox for testing, so 
the current thread must be a JavaThread (previously it could be VMThread 
during VM shutdown). But not so for deflate_idle_monitors

// This function is called by the MonitorDeflationThread to deflate
// ObjectMonitors. It is also called via do_final_audit_and_print_stats()
// by the VMThread.
size_t ObjectSynchronizer::deflate_idle_monitors() {

I'll make an adjustment to request_deflate and re-test.

Thanks,
David

>   
>   -Serguei
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/2802
>