RFR(S) Contended Locking cleanup bucket (8062851)

[Daniel D. Daugherty]

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