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

[Daniel D. Daugherty]

Hi Coleen,

Thanks for the review!  More below...


On 8/13/19 11:57 AM, coleen.phillimore at oracle.com wrote:
>
> This change looks good.

Thanks.


> I was wondering why check_owner had to return boolean until the last 
> source file change.

check_owner() returns boolean like the original check() function did so that
hasn't really changed.

I just didn't like having check(), check_slow() and the CHECK_OWNER macro.
In CR0, I got rid of all three and used a ridiculously named
check_owner_and_throw_IMSE_if_not() function. That quickly got us past the
bike shedding over names. (Funny how that works...)

David talked me into bringing back the CHECK_OWNER macro so that we didn't
duplicate the same code in three places...


> http://cr.openjdk.java.net/~dcubed/8229212-webrev/1_for_jdk14.full/test/hotspot/jtreg/runtime/Monitor/NonOwnerOps.java.html 
>
>
> This took me way too long to figure out why there's a "before" and 
> "after" condition and what the point was.
>
> Why not just have:
>
> 40         try {
>   41             obj.wait();
>> 53             System.err.println("ERROR: wait() by non-owner thread 
>> did not " +
>> 54                                "throw 
>> IllegalMonitorStateException.");
>> 55             error_count++;

I can do that. By habit, I try to do very little in "try { ... } catch" 
blocks
so just setting a flag felt right...

Do you want me to make that change (in the three sub-tests)?
And do you want a new webrev?

Dan


>
>  43         } catch (InterruptedException ie) {
>   44             System.err.println("ERROR: wait() by non-owner thread 
> threw " +
>   45                                "InterruptedException which is not 
> expected.");
>   46             error_count++;
>   47         } catch (IllegalMonitorStateException imse) {
>   48             System.out.println("wait() by non-owner thread threw 
> the " +
>   49                                "expected 
> IllegalMonitorStateException:");
>   50             System.out.println("    " + imse);
>   51         }
>
>
>
> On 8/13/19 9:49 AM, Daniel D. Daugherty wrote:
>> I still need a second review... any takers?
>>
>> Dan
>>
>>
>> On 8/12/19 8:57 AM, Daniel D. Daugherty wrote:
>>> David, thanks for the re-review!
>>>
>>> I need a second review... any takers?
>>>
>>> Dan
>>>
>>>
>>>
>>> On 8/12/19 12:00 AM, David Holmes wrote:
>>>> Looks good to me!
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>> On 10/08/2019 5:30 am, Daniel D. Daugherty wrote:
>>>>> Greetings,
>>>>>
>>>>> Updated the fix based on David H's review.
>>>>>
>>>>> Incremental webrev URL:
>>>>>
>>>>> http://cr.openjdk.java.net/~dcubed/8229212-webrev/1_for_jdk14.inc/
>>>>>
>>>>> Full webrev URL:
>>>>>
>>>>> http://cr.openjdk.java.net/~dcubed/8229212-webrev/1_for_jdk14.full/
>>>>>
>>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 8/8/19 10: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()
>>>>>>
>>>>>>     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
>>>>>>
>>>>>>
>>>>>
>>>
>>>
>>
>