RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash

Tue Oct 1 09:17:32 PDT 2013

Albert,

In general it looks good. Please, test it extensively with Tiered with codecache size enough to initialize one compiler 
but not an other to verify your code.

I think you need to swap lock and decrement otherwise it will deadlock:

+bool AbstractCompiler::should_perform_shutdown() {
+  MutexLocker only_one(CompileThread_lock);
+  _num_compiler_threads--;

comp->set_shut_down(); and delete _compilers[0]; should be done by one thread I think.

Small note, you accidentally removed nl in c2compiler.cpp:

- * Copyright (c) 1999, 2011, Oracle and/or its affiliates. All rights reserved.
+/* * Copyright (c) 1999, 2011, Oracle and/or its affiliates. All rights reserved.

Thanks,
Vladimir

On 10/1/13 12:27 AM, Albert Noll wrote:
> Hi Vladimir,
>
> thanks again for looking at the patch. See comments inline:
> webrev: http://cr.openjdk.java.net/~anoll/8023014/webrev.02/ <http://cr.openjdk.java.net/%7Eanoll/8023014/webrev.02/>
>
> On 26.09.2013 19:41, Vladimir Kozlov wrote:
>> Albert,
>>
>> There is small window between check and set_state where state can became initialized or still be uninitialized:
>> +    if (!is_initialized()) {
>> +      set_state(failed);
>>
>> so you may hit assert in set_state().
>>
>> I think you need to call set_state(failed) only when C1 runtime is initialized after lock:
>>
>>    if (should_perform_init()) {
>>      if (buffer_blob == NULL) {
>>        set_state(failed);
>>      } else {
>>        init_c1_runtime();
>>        set_state(initialized);
>>      }
>>    }
>>
> You are right. I adapted the patch according to your suggestion.
>
>> In this case it will match behavior for C2 which means that you need to shutdown compilation since you was not able to
>> initialize compilation runtime. It will require more code to do proper shutdown not just exiting thread (free
>> compilation queues, resetting flags). An it will be more complicated for Tiered case (I think we can use simple
>> approach to shutdown all compilations even if only one Compiler is not initialized since it is rare case).
>>
> I added code to shut-down compilation. If compiler runtime initialization fails, a flag is set which disables
> compilation forever. In the current
> version, all compilers are disabled if C1 and/or C2 runtime initialization fails (also in case of tiered compilation). I
> think this solution is fine for
> now, since a compiler runtime initialization failure should not happen in practice. If it ever turns out to be a
> problem, i.e., it is required that we
> we use one compiler if the other compiler failed to initialize, I suggest we fix this in another bug. The current bug
> appears only due to an artificial
> setting of the code cache size.
>
> Compiler shut-down is performed by one thread. This thread waits until all compiler threads have exited to compiler
> loop. The thread then
> deletes all tasks from the compile queue, deletes the compiler objects, and sets the objects to NULL.
>
> Please let me know if you think this solution is fine, or if there are other things that need to be considered.
> The current version passed jprt.
>
> Many thanks again,
> Albert
>
>> If C1 runtime was initialized but we was not able to allocate buffer for some threads your current code in
>> compileBroker.cpp will handle such case. So you don't need to set state to 'failed'.
>>
>> thanks,
>> Vladimir
>>
>> On 9/26/13 8:25 AM, Albert Noll wrote:
>>> Hi Vladimir,
>>>
>>> thanks for looking at the patch.
>>> Here is the new webrev:
>>> http://cr.openjdk.java.net/~anoll/8023014/webrev.01/
>>> <http://cr.openjdk.java.net/%7Eanoll/8023014/webrev.01/>
>>>
>>> See comments inline:
>>>
>>> On 26.09.2013 00:48, Vladimir Kozlov wrote:
>>>> Thank you, Albert
>>>>
>>>> I would suggest to do TieredStartAtLevel changes separately, it is not
>>>> related to this bug.
>>>>
>>> Done.
>>>> Second, how we can guard the initialization with a field
>>>> (_compiler_state and original _is_initialized) which is not static?
>>>>
>>>> You don't need separate local do_init in C*_Compiler.cpp since it is
>>>> used only once:
>>>>
>>> Done.
>>>> +  if (should_perform_init()) {
>>>>
>>>> abstractCompiler.cpp:
>>>> You removed ThreadInVMfromNative and ResetNoHandleMark from
>>>> should_perform_init() and set_state(). Why? There is assert in
>>>> check_prelock_state() called from Monitor::lock():
>>>>
>>>>   assert((!thread->is_Java_thread() || ((JavaThread
>>>> *)thread)->thread_state() == _thread_in_vm)
>>>>          || rank() == Mutex::special, "wrong thread state for using
>>>> locks");
>>>>
>>>> Compiler threads are Java threads, so you should be in VM state when
>>>> lock. How you did not hit this assert?
>>>>
>>> The state transition from native to VM (and ResetNoHandleMark) is now in
>>> compileBroker.cpp:1543.
>>>>
>>>> Typo in abstractCompiler.hpp 'methos':
>>>>
>>>> +  // This method returns true for the first compiler thread that
>>>> reaches that methos.
>>>>
>>>> It is not 'compiler object', it is 'compiler runtime' (yes it was
>>>> before but it was not accurate, it could be mistaking for Compile
>>>> object created for each compilation):
>>>>
>>>> +  // This thread will initialize the compiler object.
>>>>
>>>> Swap lines (and use 'runtime' in the comment):
>>>>
>>>> !   bool is_initialized     ()       { return _compiler_state ==
>>>> initialized; }
>>>> !   // Get/set state of compiler objects
>>>>
>>>>
>>>> c2compiler.cpp:
>>>>
>>>> C2Compiler:: is not needed:
>>>>
>>>> +    bool successful = C2Compiler::init_c2_runtime();
>>>>
>>>>
>>> Fixed the issues above.
>>>> Should we exit Compiler thread which failed local buffer allocation
>>>> instead parking? Why keep resources allocated for it?
>>>>
>>> You are right, there is no reason the keep these threads around.
>>> Compiler threads wo are not initialized correctly
>>> now exit the VM.
>>>
>>> The patch is currently in west queue.
>>>
>>> Many thanks again,
>>> Albert
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 9/25/13 1:05 PM, Albert Noll wrote:
>>>>> Hi,
>>>>>
>>>>> could I have a review for this patch?
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8023014
>>>>> webrev: http://cr.openjdk.java.net/~anoll/8023014/webrev.00/
>>>>> <http://cr.openjdk.java.net/%7Eanoll/8023014/webrev.00/>
>>>>>
>>>>> Problem: C1/C2 compiler initialization can fail if there is not enough
>>>>> memory in the code cache to generate all stubs.
>>>>> This problem occurs only with a small code cache size.
>>>>>
>>>>> Solution: Check correctness of (parts of) C1/C2 initialization and
>>>>> disable compiler (C1 and/or C2) if the compiler / thread
>>>>> was not initialized correctly.
>>>>>
>>>>> Testing: jprt, test case, benchmarks
>>>>>
>>>>> This patch also contains a refactored version of compiler object/thread
>>>>> initialization. In the new version, a compiler object/thread is
>>>>> initialized prior entering the main compiler loop. In the old version,
>>>>> the check if the compiler/thread is initialized was
>>>>> done in the main compiler loop (and hence before every compilation).
>>>>>
>>>>> To continue execution with a limited code cache size, the code types
>>>>> 'AdapterBlob' and 'MethodHandlesAdapterBlob' must
>>>>> be 'critical' allocations in the code cache, i.e., they must use space
>>>>> reserved by CodeCacheMinimumFreeSpace.
>>>>>
>>>>> In addition I removed some unued code.
>>>>>
>>>>> Many thanks in advance for reviewing.
>>>>>
>>>>> Best,
>>>>> Albert
>>>
>