RFR(S): 8063112: Compiler diagnostic commands should have locking instead of safepoint

[Nils Eliasson]

Not now at least. I'll investigate the asserts some more.

//N

On 2014-11-10 18:06, Albert Noll wrote:
> Hi Nils,
> thanks for the explanations. Please see comment inline:
> On 11/10/2014 03:36 PM, Nils Eliasson wrote:
>> Hi Albert,
>>
>> On 2014-11-10 12:57, Albert Noll wrote:
>>> Hi Nils,
>>>
>> [...]
>>>>
>>> Here are two things that I don't understand:
>>> 1) assert_locked_or_safepoint(CodeCache_lock) checks if we are at a 
>>> safepoint and/or we own the 'CodeCache_lock'. If we are at a 
>>> safepoint, how can another thread possibly modify data structures in 
>>> the code cache?
>>
>> Looking at the code one more time the contract seem to be lock and 
>> no_safepoint for mutators. But that is not what's asserted. We lock 
>> with _no_safepoint_check - but that just means we don't verify 
>> anything. We want to verify the no_safepoint.
>>
>> Many of the asserts are also not really helpful. Example: 
>> CodeCache::allocate - assert_locked_or_safepoint(CodeCache_lock) -  
>> The lock guarantees correctness for multiple threads, the safepoint 
>> don't. But we have locking at all the code paths that call on 
>> CodeCache::allocate, and we never do it at a safepoint. So it works ok.
>>
>> Writers should do: assert_locked_and_verify_no_safepoint()
>> Readers: assert locked_or_safepoint().
>>
>>>
>>> 2) Why is it safe to use assert_locked_or_safepoint(CodeCache_lock) 
>>> in so many places except for CodeCache::print_codelist(outputStream* 
>>> st)?
>>
>> On second thought is should be enough with 
>> assert_locked_or_safepoint(CodeCache_lock) on all readers if we 
>> assert locked_and_verify_not_at_safepoint on all writers. So the old 
>> implementation was correct.
> Does that mean you do no intend to push this change?
>
> Best,
> Albert
>
>>
>>>
>>> 3) To make sure that no other thread modifies the code cache, 
>>> wouldn't you you have to make sure that every modification to the 
>>> code cache is guarded by assert(CodeCache_lock->owned_by_self(), 
>>> "Checking") instead of assert_locked_or_safepoint(CodeCache_lock)? 
>>> I.e., a lock is only useful if every modification to the code cache 
>>> is guarded by that lock.
>>
>> We should assert on lock and verify not at safepoint at the places 
>> modifying the code cache if we want to keep the contract that it is 
>> ok to read at safepoints.
>>
>> Thanks,
>> Nils
>>
>>> Best,
>>> Albert
>>>
>>>> Regards,
>>>> Nils
>>>>
>>>>>
>>>>> Thanks,
>>>>> Albert
>>>>>> Tested with jtreg tests in Hotspot and JDK that exercise this code.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8063112
>>>>>> Webrev: http://cr.openjdk.java.net/~neliasso/8063112/webrev.01/
>>>>>>
>>>>>> Thanks,
>>>>>> Nils
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20141111/87f0b591/attachment.html>