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

[coleen.phillimore at oracle.com]

On 8/13/19 4:10 PM, Daniel D. Daugherty wrote:
> On 8/13/19 1:30 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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!
>
> Thanks.
>
>
>>>
>>>
>>>> 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.
>
> This ended up changing quite a few lines in the test so I went ahead
> and generated new webrevs. The incremental webrev is also more clear
> than the "hg diff" output.
>
> Incremental webrev URL:
>
> http://cr.openjdk.java.net/~dcubed/8229212-webrev/2_for_jdk14.inc/

Good, this is what I expected it would look like.

thanks,
Coleen
>
> Full webrev URL:
>
> http://cr.openjdk.java.net/~dcubed/8229212-webrev/2_for_jdk14.full/
>
>
> Dan
>
>
>>
>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>