RFR(S): 8063112: Compiler diagnostic commands should have locking instead of safepoint
albert.noll at oracle.com
Mon Nov 10 17:06:53 UTC 2014
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*
> 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?
>> 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.
>>>>> 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/
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev