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

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Aug 13 20:10:22 UTC 2019


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/

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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>



More information about the hotspot-runtime-dev mailing list