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

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Oct 2 09:49:44 PDT 2013 

Albert,

In disable_compilation_forever() the comment does not correspond to code.

In should_perform_shutdown() the _compiler_state could be changed before other threads reach "if 
(comp->is_state_failed())" check in init_compiler_runtime().
Yes, decrementing with lock is fine, you are right.

The message should say why we do shutdown (no space to initialize compiler runtime):
warning("Shutting down compiler

Please, explain what references:

+    // We could delete compiler runtimes also. However,
+    // there are references to the compiler runtime(s)
+    // (e.g., by nmethods) which then fail. This can be

It would be nice to have regression test for this.

Thanks,
Vladimir

On 10/2/13 2:47 AM, Albert Noll wrote:
> Hi Vladimir,
>
> thanks again for the feedback. I tested the patch as you proposed, i.e., make sure that
> the patch works if the C1 and/or C2 compiler fails to initialize. I tested with up to 256 compiler
> threads.
>
> To make sure that all compiler threads exit, I had to do additional modifications in CompileQueue::get()
> and the caller of this method. With these changes, one can also exit compiler threads if -XX:-UseCodeCacheFlushing
> is set, which is also contained in this webrev.
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~anoll/8023014/webrev.03/ <http://cr.openjdk.java.net/%7Eanoll/8023014/webrev.03/>
>
> See more comments inline:
>
>
> On 01.10.2013 18:17, Vladimir Kozlov wrote:
>> 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.
>>
> Done.
>> 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--;
>>
> I simplified this function. In fact it is fine if the last thread (i.e., if _num_compiler_threads == 0)
> does the shutdown. The while() statement is really not necessary. However, we have to decrement
> _num_compiler_threads when we hold the lock. Otherwise we are not guaranteed to reach 0
> (-- is not atomic).
>
>> comp->set_shut_down(); and delete _compilers[0]; should be done by one thread I think.
> Done.
>
>>
>> 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.
>>
>>
> Fixed.
>
> Thanks again,
> Albert
>
>> 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
>>>>>
>>>
>

More information about the hotspot-compiler-dev mailing list