RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash
Albert Noll
albert.noll at oracle.com
Thu Oct 3 05:59:30 PDT 2013
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
>>>>>>
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20131003/3b0b38d4/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list