RFR(S) Contended Locking cleanup bucket (8062851)

[Daniel D. Daugherty]

Thanks for the re-review!

Dan


On 11/6/14 11:31 PM, David Holmes wrote:
> Still fine for me.
>
> Thanks,
> David
>
> On 7/11/2014 5:01 AM, Daniel D. Daugherty wrote:
>> Here's an updated webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8062851-webrev/1-jdk9-hs-rt/
>>
>> What I did to sanity check this is compare the patch files from
>> the two webrevs...
>>
>> Dan
>>
>>
>> On 11/6/14 11:19 AM, Daniel D. Daugherty wrote:
>>> I just reread the entire review thread.
>>>
>>> I'm going to tweak the macro name a little bit:
>>>
>>>     OM_OFFSET_NO_MONITOR_VALUE => OM_OFFSET_NO_MONITOR_VALUE_TAG
>>>
>>> I think that captures the intent quite nicely...
>>>
>>> Yes, I considered OM_OFFSET_WITHOUT_MONITOR_VALUE_TAG, but that
>>> made the macro even longer... :-)
>>>
>>> Dan
>>>
>>>
>>> On 11/6/14 11:13 AM, Daniel D. Daugherty wrote:
>>>> On 11/5/14 5:34 PM, David Holmes wrote:
>>>>> On 6/11/2014 10:23 AM, Coleen Phillimore wrote:
>>>>>>
>>>>>> On 11/5/14, 7:16 PM, David Holmes wrote:
>>>>>>> On 6/11/2014 1:49 AM, Claes Redestad wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 11/05/2014 05:34 AM, Daniel D. Daugherty wrote:
>>>>>>>>> Greetings,
>>>>>>>>>
>>>>>>>>> I have a Contended Locking cleanup bucket fix ready for review.
>>>>>>>>>
>>>>>>>>> This fix was spun off from the Contended Locking fast enter 
>>>>>>>>> bucket
>>>>>>>>> which was sent out for review late last week. This fix cleans up
>>>>>>>>> the computation of ObjectMonitor field pointers and gets rid of
>>>>>>>>> the use of literal '-2' in appropriate places. For example:
>>>>>>>>>
>>>>>>>>> -         ld_ptr(Rmark, ObjectMonitor::owner_offset_in_bytes() 
>>>>>>>>> - 2,
>>>>>>>>> Rscratch);
>>>>>>>>> +         ld_ptr(Rmark, OM_OFFSET_NO_MONITOR_VALUE(owner),
>>>>>>>>> Rscratch);
>>>>>>>>>
>>>>>>>>> The OM_OFFSET_NO_MONITOR_VALUE macro computes the offset to the
>>>>>>>>> specified field and subtracts markOopDesc:monitor_value (2).
>>>>>>>>> There's a nice comment in src/share/vm/runtime/objectMonitor.hpp.
>>>>>>>>
>>>>>>>> any reason not to add it as a function in objectMonitor.hpp
>>>>>>>> instead of a
>>>>>>>> macro? How about:
>>>>>>>>
>>>>>>>>    static int no_monitor_offset_in_bytes()  { return
>>>>>>>> offset_of(ObjectMonitor, _owner) - markOopDesc::monitor_value; }
>>>>>>>
>>>>>>> _owner is not the only field used so you would need a function for
>>>>>>> each one.
>>>>>>
>>>>>> I thought this would be better too.   There are only 6 functions (6
>>>>>> lines) max that need this.  It would look nicer.
>>>>>
>>>>> Only changes an upper case macro name to a lower case function name.
>>>>
>>>> As I mentioned in my reply to Claes, there are 12 offset_in_bytes()
>>>> functions
>>>> so for completeness we would add 12 new functions. I really don't
>>>> want to do
>>>> that.
>>>>
>>>>
>>>>>
>>>>>> My suggestion would be to make them static int
>>>>>> untagged_offset_in_bytes() or whatever monitor_value is. It's not a
>>>>>> very descriptive name so better to name the functions after what
>>>>>> it's for.
>>>>>
>>>>> You need the field name included in the function name:
>>>>>
>>>>> untagged_offset_of_owner()
>>>>> untagged_offset_of_xxx()
>>>>>
>>>>> but it is only untagged if the OM is currently inflated, so then:
>>>>>
>>>>> untagged_offset_of_XXX_for_inflated_om()
>>>>>
>>>>> I can live with Dan's macro (which is an improvement on the 
>>>>> original).
>>>>
>>>> Thanks! I'm planning to stick with the macro which has a much smaller
>>>> footprint for such a strange little task...
>>>>
>>>> Dan
>>>>
>>>>
>>>>>
>>>>> David
>>>>>
>>>>>> Coleen
>>>>>>
>>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> Example usage:
>>>>>>>>
>>>>>>>> -         ld_ptr(Rmark, ObjectMonitor::owner_offset_in_bytes() 
>>>>>>>> - 2,
>>>>>>>> Rscratch);
>>>>>>>> +         ld_ptr(Rmark, 
>>>>>>>> ObjectMonitor::no_monitor_offset_in_bytes(),
>>>>>>>> Rscratch);
>>>>>>>>
>>>>>>>>
>>>>>>>> Seems this should be inlined regardless and looks a bit cleaner
>>>>>>>> to me.
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> /Claes
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks to David Holmes for his comments on JDK-8061553 that
>>>>>>>>> motivated this (long overdue) cleanup.
>>>>>>>>>
>>>>>>>>> This work is being tracked by the following bug ID:
>>>>>>>>>
>>>>>>>>>     JDK-8062851 cleanup ObjectMonitor offset adjustments
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8062851
>>>>>>>>>
>>>>>>>>> Here is the webrev URL:
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~dcubed/8062851-webrev/0-jdk9-hs-rt/
>>>>>>>>>
>>>>>>>>> Here is the JEP link:
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8046133
>>>>>>>>>
>>>>>>>>> Testing:
>>>>>>>>>
>>>>>>>>> - JPRT test jobs (since this is only syntax and comment cleanup)
>>>>>>>>>
>>>>>>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>>>>>>
>>>>>>>>> Dan
>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>