RFR(S) Contended Locking cleanup bucket (8062851)

[Daniel D. Daugherty]

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