RFR (XXS): 8023461: Thread holding lock at safepoint that vm can block on: MethodCompileQueue_lock

[Vladimir Kozlov]

On 5/13/14 2:34 PM, Vladimir Ivanov wrote:
>> Relaxing the check is not the fix. ATP::select_task() should be fixed -
>> exclude safepoints in it.
> OK. I planned to fix select_task separately (filed RFE to track that),
> since people complains about intermittent failures during testing due to
> this bug and I have some concerns with the fix I had. But I'll try to
> fix it as part of this change then.
>
>> The update_rate() could be not called if there are no MethodCounters
>> (other places may need to be fixed for that). Actually I don't
>> understand how we have compilation request for a method without
>> MethodCounters. Counters should be created before that.
> It's not always the case. MDO can be created first (e.g. compilation can
> force MDO creation [1]) and there's no need in MethodCounters (see
> InterpreterGenerator::generate_counter_incr [2]).

I think this is contradiction in tiered implementation. We don't 
allocate MethodCounters for TieredCompilation as you pointed but tiered 
code uses them.

>
>> And removing stale tasks could be done in an other place, outside MCQ
>> lock.
> I looked again closely at the code and I think I found a sweet spot.
>
> Updated webrev:
> http://cr.openjdk.java.net/~vlivanov/8023461/webrev.03/

Very good. Add small comment explaining your trick to avoid conflicts 
with other compiler threads after lock is released:

+     CompileTask* head = _first_stale;
+     _first_stale = NULL;

Thanks,
Vladimir

>
> Testing: JPRT, VM testbase, bigapps, jtreg
>
> Best regards,
> Vladimir Ivanov
>
> [1]
> Method::build_interpreter_method_data(methodHandle, Thread*)
> ciMethod::ensure_method_data(methodHandle)
> ciMethod::ensure_method_data()
> GraphBuilder::try_inline_full(ciMethod*, bool, Bytecodes::Code,
> Instruction*)
>
> [2]
> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/tip/src/cpu/x86/vm/templateInterpreter_x86_32.cpp#l340
>
>
>> Thanks,
>> Vladimir K
>>
>> On 5/12/14 11:15 AM, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~vlivanov/8023461/webrev.02/
>>> https://bugs.openjdk.java.net/browse/JDK-8023461
>>>
>>> CompileQueue::get() acquires MethodCompileQueue lock before extracting a
>>> task from the compilation queue. In Tiered mode it calls
>>> AdvancedThresholdPolicy::select_task, which can safepoint in multiple
>>> places. It fires an assert which checks there are no VM locks held VM
>>> thread can block on.
>>>
>>> I checked the code and didn't find any scenarios where VM acquires MCQ
>>> lock and submits a compilation task from a safepoint, so I decided to
>>> relax the assert in Thread::check_for_valid_safepoint_state for now and
>>> exclude MethodCompileQueue in Tiered mode from the check.
>>>
>>> I tried to rewrite ATP::select_task to avoid safepoints, but didn't
>>> succeed. I filed a RFE to further investigate that path (JDK-8042925
>>> [3]: Consider avoiding safepoints in ...::select_task). Eventually I
>>> want ATP::select_task to become safepoint-free and then strengthen the
>>> check back.
>>>
>>> For the problems spotted during analysis, I filed 2 followup bugs:
>>>    [1] JDK-8042924: Possible memory leak during MethodCounters
>>> initialization
>>>    [2] JDK-8042926: AdvancedThresholdPolicy::update_rate should use
>>> handles for Method*
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8042924
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8042926
>>> [3] https://bugs.openjdk.java.net/browse/JDK-8042925