RFR(S): 8229212 clear up CHECK_OWNER confusion in objectMonitor.cpp

[Daniel D. Daugherty]

David,

Thanks for the review!


On 8/8/19 7:09 PM, David Holmes wrote:
> Hi Dan,
>
> I see what you are doing but I found things a little confusing ...

So much for my hopeful bug title: "clear up CHECK_OWNER confusion..."
More below...


> On 9/08/2019 12:25 am, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> Discussions with David Holmes about ObjectMonitors revealed a bit of
>> a mess with the CHECK_OWNER macro and the ObjectMonitor::check() and
>> ObjectMonitor::check_slow() functions. I have a small fix to clean up
>> this bit of messy code.
>>
>> The bug URL:
>>
>>      JDK-8229212 clear up CHECK_OWNER confusion in objectMonitor.cpp
>>      https://bugs.openjdk.java.net/browse/JDK-8229212
>>
>> The webrev URL:
>>
>> http://cr.openjdk.java.net/~dcubed/8229212-webrev/0_for_jdk14/
>>
>> Summary of the fix:
>>
>>      Merge the CHECK_OWNER macro and the ObjectMonitor::check() and
>>      ObjectMonitor::check_slow() functions into a new function:
>>
>>          ObjectMonitor::check_owner_and_throw_IMSE_if_not()
>
> Bikeshed: sorry but that is far too long a name. check_owner() 
> suffices and the comments for it state what it does in regard to 
> throwing IMSE. We don't name methods in a way that includes what 
> exceptions they may result in - that's what documentation is for.

I figured that name would get your attention. :-) And it did...

I'm fine with 'check_owner()'.


> src/hotspot/share/runtime/objectMonitor.cpp
>
> ! // function is not called with CHECK because we want the IMSE to be the
> ! // only exception that is thrown from the call site when false is 
> returned.
>
> I didn't understand that comment until I saw the actual call site code.
>
>   if (!check_owner_and_throw_IMSE_if_not(THREAD)) {
>      assert(HAS_PENDING_EXCEPTION, "expected a pending exception here.");
>      return;
>    }
>
> This piece of code is repeated three times so seems to be screaming 
> out to still be a macro.

I guess my dislike for macros got the better of me...


> I'm inclined to make some simpler adjustments to the macro and its 
> commentary:
>
> // Checks that the current THREAD owns this monitor and causes
> // an immediate return if it doesn't. We don't use the CHECK macro
> // because we want the IMSE to be the only exception that is thrown from
> // the call site when false is returned. Any other pending exception is
> // ignored.
> #defined CHECK_OWNER()          \
>   do {                          \
>     if (!check_owner(THREAD)) { \
>        assert(HAS_PENDING_EXCEPTION, "expected a pending IMSE here."); \
>        return;                  \
>      }                          \
>   } while (false)
>
> then have the new implementation:
>
> // Returns true if the specified thread owns the ObjectMonitor.
> // Otherwise returns false and throws IllegalMonitorStateException
> // (IMSE). If there is a pending exception and the specified thread
> // is not the owner, that exception will be replaced by the IMSE.
> bool ObjectMonitor::check_owner(Thread* THREAD) {
>   if (_owner == THREAD) {
>     return true;
>   }
>   if (THREAD->is_lock_owned((address)_owner)) {
>     _owner = THREAD;  // convert from BasicLock addr to Thread addr
>     _recursions = 0;
>     return true;
>   }
>   THROW_MSG_(vmSymbols::java_lang_IllegalMonitorStateException(),
>              "current thread is not owner", false);
> }

I can update the patch to match the above.


> The call sites then remain the same, other than fixing this comment:
>
> 1200   // Throw IMSX or IEX.
>
> I don't know what IEX is meant to be :)

Since we're in wait(), I'm guessing IEX is meant to be InterruptedException.
However, even in the original code, we did not throw InterruptedException
from that location.

I'll likely have another round out later today.

Thanks again for the review.

Dan


>
> Thanks,
> David
>
>>      Update the CHECK_OWNER and check() call sites to use
>>      check_owner_and_throw_IMSE_if_not(). Update the comments
>>      to make the intended semantics clear.
>>
>> Also included a new test for when Object.wait(), Object.notify()
>> and Object.notifyAll() are called a thread that doesn't own the
>> Java monitor.
>>
>> Test with Mach5 Tier[1-3] on the usual Oracle platforms. Verified
>> that the new test passes on all Oracle platforms. It's a Tier1
>> test but its duration is < 1s on every platform.
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>