RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash
Albert Noll
albert.noll at oracle.com
Wed Oct 9 12:20:09 PDT 2013
Thank you, Vladimir.
Best,
Albert
Von meinem iPhone gesendet
> Am 09.10.2013 um 18:02 schrieb Vladimir Kozlov <vladimir.kozlov at oracle.com>:
>
> Looks good.
>
> Albert, I agree with your comments. See below.
>
> Thanks,
> Vladimir
>
>> On 10/8/13 10:00 PM, Albert Noll wrote:
>> Hi Vladimir, hi Christian,
>>
>> thanks for looking at the patch.
>> @Christian: I fixed the issues you brought up.
>> @Vladimir: See comments inline:
>>
>> Here is the updated webrev:
>> http://cr.openjdk.java.net/~anoll/8023014/webrev.05/
>> <http://cr.openjdk.java.net/%7Eanoll/8023014/webrev.05/>
>>
>>> On 04.10.2013 00:10, Vladimir Kozlov wrote:
>>> 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 need Compiler::Compiler() {}, otherwise I get the following error:
>>
>> Error: failed /export/anoll/alt_export/jre/lib/amd64/server/libjvm.so,
>> because /export/anoll/alt_export/jre/lib/amd64/server/libjvm.so:
>> undefined symbol: _ZN8CompilerC1Ev
>
> Okay.
>
>>
>> +Compiler::~Compiler() {} can be removed.
>>
>>> 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.
>> Done.
>>> Indention in CompileQueue::delete_all():
>>>
>>> }
>>> _first = NULL;
>>> }
>> Done.
>>> >> 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.
>> I think we need this check, since compiler initialization can be
>> successful and there is enough free
>> space in the code cache to allocate the buffer blob for the first couple
>> of threads, but not for all threads.
>> The test specifically tests this exit path.
>
> Yes, you are right. Only initialization thread will set the state to 'failed'.
>
>>
>> Many thanks again,
>> Albert
>>
>>> 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