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

[Nils Eliasson]

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.

>
> 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/71af59f6/attachment.html>