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

[coleen.phillimore at oracle.com]

On 8/13/19 12:14 PM, Daniel D. Daugherty wrote:
> 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...

This makes sense!
>
>
>> 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?

Please, I think it would be clearer that way.  If you make this change, 
I do not need to see another webrev.

thanks,
Coleen
>
> 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
>