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

Tobias Hartmann tobias.hartmann at oracle.com
Wed Oct 15 08:09:20 UTC 2014


Hi David,

On 15.10.2014 10:02, David Holmes wrote:
> 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.

Okay, thanks for pointing that out.

Best,
Tobias

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


More information about the hotspot-compiler-dev mailing list