RFR(S): 8229212 clear up CHECK_OWNER confusion in objectMonitor.cpp
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Aug 13 22:30:40 UTC 2019
On 8/13/19 5:14 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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 for confirming.
Dan
>
> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list