RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Oct 3 15:10:35 PDT 2013
c1_Compiler.cpp. Can you have next in header file since they are empty?
And why you need them at all (C2Compiler does not have them)?:
+Compiler::Compiler () {}
+Compiler::~Compiler() {}
I think we can keep compiler objects (Compiler and c2Compiler) since
they should be very small. You can keep your comment in
shutdown_compiler_runtime() but remove commented code.
Indention in CompileQueue::delete_all():
}
_first = NULL;
}
>> In should_perform_shutdown() the _compiler_state could be changed
>> before other threads reach "if (comp->is_state_failed())" check in
>> init_compiler_runtime().
> True. I added a function should_exit(), which returns true if the state
> if failed, or shutting down.
You actually don't need to change state to shutting_shown. This state is
not checked anywhere (you removed waiting loop). The 'failed' state can
stay until the last thread change it to shut_down. So you don't need new
method should_exit().
Looking more on get_buffer_blob() == null case and I think you don't
need special C1 check in init_compiler_runtime() because it will not
reach it. Compiler::initialize() set state to 'failed' if buffer is NULL
so the first condition in init_compiler_runtime() will be true.
The test is fine. At least we have something.
Thanks,
Vladimir
On 10/3/13 5:59 AM, Albert Noll wrote:
> Hi Vladimir,
>
> thanks for looking at the patch. See comments inline.
> Here is the updated webrev:
> http://cr.openjdk.java.net/~anoll/8023014/webrev.04/
> <http://cr.openjdk.java.net/%7Eanoll/8023014/webrev.04/>
>
> On 02.10.2013 18:49, Vladimir Kozlov wrote:
>> Albert,
>>
>> In disable_compilation_forever() the comment does not correspond to code.
>>
> I removed the comment.
>> In should_perform_shutdown() the _compiler_state could be changed
>> before other threads reach "if (comp->is_state_failed())" check in
>> init_compiler_runtime().
> True. I added a function should_exit(), which returns true if the state
> if failed, or shutting down.
>> 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
>>
> The warning messages are now:
> - "Shutting down compiler %s (no space to run compiler)" or
> - "Initialization of %s thread failed (no space to run compiler thread)"
> if a C1 compiler thread fails to allocate memory.
>> 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
>>
> Done.
>> It would be nice to have regression test for this.
> I added a regression test, which checks that compiler threads exit, if
> there is not sufficient free code cache
> to init all threads. Testing a failing initialization of C2 is tricky,
> since a C2 initialization failure depends on the
> scheduling of the compiler threads.
>
> Thanks again,
> Albert
>>
>> 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