[9] RFR(S): 8059846: InstanceKlass should use MutexLockerEx to acquire OsrList_lock

[David Holmes]

On 15/10/2014 4:48 PM, Tobias Hartmann wrote:
> Hi David,
>
> On 15.10.2014 02:02, David Holmes wrote:
>> Tobias's change is fine but I get worried when I see:
>>
>> // This is a short non-blocking critical region, so the no safepoint
>> check is ok.
>>
>> as that is only one of the conditions needed to allow locking without
>> safepoint
>> checks!
>
> What other conditions have to hold? Do you see a problem here? I think
> it should be fine as this is very old code that never failed before.

If any use of a lock avoids the safepoint check then "all"* uses of the 
lock must avoid the safepoint check, and it must be impossible to block 
at a safepoint while the lock is held.

*There can be exceptions if the lock is only used by specific threads in 
specific circumstances.

It seems the only uses of this lock are in the code you changed so it 
appears ok - just wanted to highlight a common misconception about when 
it is okay to skip the safepoint checks. Though really any such code 
should have a No_Safepoint_Verifier in the locked region (at least in 
debug mode). Curiously the uses of the MemberNameTable_lock in the same 
file all have the No_Safepoint_Verifier but don't elide the safepoint 
check when locking.

Cheers,
David

> Thanks,
> Tobias
>
>> David H.
>>
>> On 15/10/2014 12:58 AM, Vladimir Kozlov wrote:
>>> Looks good. Should be reviewed by runtime too since changes are in
>>> instanceKlass.cpp
>>>
>>> Thanks,
>>> Vlaidmir
>>>
>>> On 10/14/14 5:54 AM, David Chase wrote:
>>>> Hello Tobias,
>>>>
>>>> Not a Reviewer here, but it looks correct to me.
>>>> One question — above I see a NEEDS_CLEANUP notation.
>>>> Do we know (does someone know?) if it refers to this change?
>>>> If so, we could remove that :-).
>>>>
>>>> David
>>>>
>>>> On 2014-10-14, at 3:55 AM, Tobias Hartmann
>>>> <tobias.hartmann at oracle.com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> please review this small code cleanup that replaces the explicit
>>>>> locking of OsrList_lock by a MutexLockerEx instantiation.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8059846
>>>>> Webrev: http://cr.openjdk.java.net/~thartmann/8059846/webrev.00/
>>>>>
>>>>> Thanks,
>>>>> Tobias
>>>>