RFR(S/T): 8230876: baseline cleanups from Async Monitor Deflation v2.0[789]

[Daniel D. Daugherty]

Vladimir,

Thanks for the review!


On 11/19/19 2:38 PM, Vladimir Kozlov wrote:
> macroAssembler_x86.cpp changes are only in comments which looks good 
> to me.

Thanks.


> Fast_Lock/Fast_Unlock were names before I moved code from .ad files to 
> macroAssembler_x86.cpp (8033805).

Yup. I figured we should match the current names.


> About "Without cast to int32_t this style of movptr will destroy r10 
> which is typically obj."
> 64-bit version of movptr() has 2 versions of second argument, intptr_t 
> and int32_t:
>
> http://hg.openjdk.java.net/jdk/jdk/file/faac483dfb30/src/hotspot/cpu/x86/macroAssembler_x86.cpp#l728 
>
>
> intptr_t version use rscratch1 (R10) which is most likely is used and 
> have valid value in it (in C2 generated code):
>
> http://hg.openjdk.java.net/jdk/jdk/file/faac483dfb30/src/hotspot/cpu/x86/x86_64.ad#l134 
>

Thanks for confirmation that the comment is still valid.

Again, thanks for the review!

Dan


>
> Vladimir
>
> On 11/19/19 6:41 AM, Daniel D. Daugherty wrote:
>> Hi David,
>>
>> Thanks for the quick review!
>>
>> On 11/18/19 9:23 PM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> Given:
>>>
>>>  volatile intptr_t _recursions;
>>>
>>> The change to the print statements to use INTX_FORMAT instead of the 
>>> existing INTPTR_FORMAT seems a little odd - but obviously you don't 
>>> want it printed in hex.
>>
>> Yup. I first noticed it when I wrote and tested my initial version of
>> ObjectMonitor::print_debug_style_on() and then I realized that it was 
>> all
>> over with other prints of that field.
>>
>>
>>> That seems fine, but can we then make the simple change to redefine 
>>> _recursions as intx as well - which is a semantic no-op given:
>>>
>>> typedef intptr_t  intx;
>>
>> No problem. I'll make that change before I push. Not sure why that
>> didn't occur to me before.
>>
>>
>>> Otherwise all seems okay.
>>
>> Thanks! Now, if we can just get a quick review of macroAssembler_x86.cpp
>> from Vladimir K... :-)
>>
>> Dan
>>
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 19/11/2019 8:23 am, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I have another round of baseline cleanup changes from the Async 
>>>> Monitor
>>>> Deflation project (8153224). Like previous sub-tasks of 8153224, these
>>>> changes are small and/or trivial. These changes have previously been
>>>> reviewed as a (very) small part of 8153224 (CR8/v2.08/11-for-jdk14).
>>>>
>>>> Vladimir K., if you could sanity check the cleanups in 
>>>> macroAssembler_x86.cpp
>>>> that would be appreciated (only comments were changed). I recommend 
>>>> the
>>>> Udiff view...
>>>>
>>>> Please see the bug for details about the changes in this webrev:
>>>>
>>>>      JDK-8230876 baseline cleanups from Async Monitor Deflation 
>>>> v2.0[789]
>>>>      https://bugs.openjdk.java.net/browse/JDK-8230876
>>>>
>>>> Here's the webrev URL:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8230876-webrev/0-for-jdk14/
>>>>
>>>> These changes have been included in my recent rounds of Mach5 
>>>> Tier[1-8]
>>>> and other associated stress and/or performance testing. I have also 
>>>> done
>>>> a Mach5 Tier[1-3] run with just this patch to make sure that I got all
>>>> the pieces that are needed.
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> Dan
>>