RFR(S) Contended Locking cleanup bucket (8062851)

[Daniel D. Daugherty]

On 11/5/14 11:37 AM, Coleen Phillimore wrote:
>
> Dan,  I had a look at this change too.

Thanks for reviewing!


> On 11/5/14, 10:29 AM, Daniel D. Daugherty wrote:
>> On 11/5/14 3:42 AM, David Holmes wrote:
>>> Hi Dan,
>>>
>>> Reviewed.
>>
>> Thanks!
>>
>>
>>> I find the name OM_OFFSET_NO_MONITOR_VALUE somewhat awkward but have 
>>> no better suggestion.
>>
>> Understood. I didn't like the original "OFFSET_SKEWED" name
>> especially since I was moving it to objectMonitor.hpp...
>>
>> If you think of a better, let me know... we can always change it.
>>
>
> So the -2 was a tag?  Then maybe a better name is UNTAGGED_OM_OFFSET 
> ..  Weird stuff anyway.

Yes, the '2' is one of the markWord encodings so we have to remove it
in order to have a proper pointer. Definitely weird stuff.


> In 
> http://cr.openjdk.java.net/~dcubed/8062851-webrev/0-jdk9-hs-rt/src/cpu/x86/vm/macroAssembler_x86.cpp.udiff.html
>
> Can you make the whitespace changes to the lines you've changed:
>
> +         movptr(tmpReg, Address (tmpReg, 
> OM_OFFSET_NO_MONITOR_VALUE(owner)));   // rax, = m->_owner
>
>
> to
>
> +         movptr(tmpReg, Address(tmpReg, 
> OM_OFFSET_NO_MONITOR_VALUE(owner)));   // rax, = m->_owner

Yes, I'll make some whitespace cleanups to the lines that I touch.


> In general, this looks like a great improvement not subtracting two 
> from seemingly random places in assembly code.

Thanks goes to David H. for noticing the cleanup note that was
left in the code previously. My only tweak to it was putting the
macro in place where it could be shared by different CPU impls
and hopefully I improved the comment. :-)

Dan


>
> thanks,
> Coleen
>
>>
>>
>>> In fact I have to ask what _is_ the object monitor tagging 
>>> mechanism? I can't see it defined in the objectMonitor.* files. ??
>>
>> That would be this code:
>>
>> src/share/vm/oops/markOop.hpp:
>>
>>     317   static markOop encode(ObjectMonitor* monitor) {
>>     318     intptr_t tmp = (intptr_t) monitor;
>>     319     return (markOop) (tmp | monitor_value);
>>     320   }
>>
>> and the other methods in that file that have to account for
>> the monitor_value being set...
>>
>> Dan
>>
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 5/11/2014 2:34 PM, 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.
>>>>
>>>> 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
>>
>
>