RFR(S): 8217658 baseline_cleanups from Async Monitor Deflation project

[Daniel D. Daugherty]

On 1/23/19 11:18 PM, David Holmes wrote:
> Hi Dan,
>
> This all seems fine. Some comments below.

Thanks for the review!


>
> One suggested addition to your cleanup :)
>
> ./share/runtime/basicLock.cpp
>
> // [RGV] The next line appears to do nothing!
>   intptr_t dh = (intptr_t) displaced_header();
>   dest->set_displaced_header(displaced_header());

Sure. I was confused for a second since I couldn't remember touching
share/runtime/basicLock.cpp in my current project. Of course, you
sending me a cleanup out of the blue reminds me of this other one
you sent me a very long ago:

./share/vm/prims/jvmtiExport.cpp:                     ("JVMTI [%s] 
montior contended enter event triggered",
./share/vm/prims/jvmtiExport.cpp:                     ("JVMTI [%s] 
montior contended entered event triggered",
./share/vm/prims/jvmtiExport.cpp:                     ("JVMTI [%s] 
montior wait event triggered",
./share/vm/prims/jvmtiExport.cpp:                     ("JVMTI [%s] 
montior waited event triggered",

that's been hanging around in my cleanup queue for a while...

I'll take care of both of these with 8217658.

Dan


>
> On 24/01/2019 6:07 am, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I have a (S)mall patch extracted from the Async Monitor Deflation
>> project that is ready for code review.
>>
>> The short version of what this patch is about:
>>
>>      This sub-task captures updates to the baseline code that are not
>>      directly related to Async Monitor Deflation.
>>
>> The details are in the bug report:
>>
>>      JDK-8217658 baseline_cleanups from Async Monitor Deflation project
>>      https://bugs.openjdk.java.net/browse/JDK-8217658
>>
>> Here's the webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8217658-webrev/0-for-jdk13/
>
> To quote your main semantic changes from the bug report:
>
>> - add missing "else" to logical if-else-statement in
>>   ObjectSynchronizer::get_lock_owner()
>
> Okay - the two conditions are mutually exclusive.
>
>> - guarantee that header() is NULL when
>>   ObjectSynchronizer::omRelease() is called
>> - Replace a set_owner(NULL) call with a set_header(NULL)
>>   call in the "CASE: neutral" part of ObjectSynchronizer::inflate()
>>   when the object->cas_set_mark() fails. This is a rare race. 
>
> These seem okay but did get me thinking about the different roles of 
> ObjectMonitor::Recycle() and ObjectMonitor::clear() and the individual 
> clearing of fields! I find it all somewhat confusing - in particular 
> why clear() asserts that _header != NULL ! Some explanatory comments 
> on those methods would be appreciated at some point during this project.
>
> Thanks,
> David
> -----
>
>
>>
>> This patch along with the other two patches for Async Monitor Deflation
>> project have been through Mach5 
>> builds-tier1,hs-tier1,jdk-tier1,hs-tier2,hs-tier3
>> testing and I'm currently running my stress testing kits on my Linux-X64
>> and Solaris-X64 servers.
>>
>> Thanks, in advance, for any questions, comments or suggestions.
>>
>> Dan
>>