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

[Daniel D. Daugherty]

On 1/25/19 8:43 AM, Daniel D. Daugherty wrote:
> 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...

Closing the loop on whether I messed this up...

Here's the block where I added set_header(NULL) at L1463:

1462     if (object->cas_set_mark(markOopDesc::encode(m), mark) != mark) {
1463       m->set_header(NULL);
1464       m->set_object(NULL);
1465       m->Recycle();
1466       omRelease(Self, m, true);
1467       m = NULL;
1468       continue;

For this block, we call Recycle() and omRelease(). The set_header(NULL)
is needed to make the new omRelease() guarantee() on L1167 happy:

1165 void ObjectSynchronizer::omRelease(Thread * Self, ObjectMonitor * m,
1166                                    bool fromPerThreadAlloc) {
1167   guarantee(m->header() == NULL, "invariant");
1168   guarantee(m->object() == NULL, "invariant");

so my addition can't run afoul of clear() because we use Recycle()
instead. Right now I'm leaning toward describing these as:

Recycle() - recycle a partially used ObjectMonitor for putting
             it back on the free list.
clear()   - clear a fully used ObjectMonitor for putting it
             back on the free list.

Of course, I'd have to go visit all the uses of these two and make
sure that description holds. That will be a reasonable amount of
work to do in a separate issue.

Dan


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