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

[Albert Noll]

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/20141110/33414336/attachment.html>