RFR(S) Contended Locking cleanup bucket (8062851)

[Daniel D. Daugherty]

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