RFR(S) Contended Locking cleanup bucket (8062851)

[Coleen Phillimore]

While I'm not a huge fan of long macro names, it's still shorter than 
the thing it replaced:

-      add(Rmark, ObjectMonitor::owner_offset_in_bytes()-2, Rmark);
+      add(Rmark, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner), Rmark);

I like it!

Coleen

On 11/6/14, 2:01 PM, 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
>>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>