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

[Daniel D. Daugherty]

I missed a couple of comments from David H. below...

On 1/24/19 9:05 AM, Daniel D. Daugherty wrote:
> 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.

Yup. That's my thinking. :-)


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

The fact that we have Recycle(), clear() and individual clearing of
fields scattered throughout the code is a mess. I obviously made
small clean ups with this fix, but there's still a lot of craziness.


>> - in particular why clear() asserts that _header != NULL !

That's clear()'s way of saying "make sure the ObjectMonitor that
we're clearing was actually in use". I'll have to double check
that my switch from set_owner(NULL) (which was not needed) to
set_header(NULL) (which is needed) won't mess things up. Nothing
showed up in testing or in stress testing, but...


>> Some explanatory comments on those methods would be appreciated at 
>> some point during this project.

Hmmm... I thinking cleaning up/clarifying Recycle(), clear() and
individual clearing of fields would be enough work/analysis to
warrant its own bug fix. Might be done as part of this overall
project, might be pushed separately (and earlier).

Dan

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