RFR(S) Contended Locking cleanup bucket (8062851)

[David Holmes]

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.

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

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