RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash
Albert Noll
albert.noll at oracle.com
Tue Oct 8 22:00:42 PDT 2013
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
+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.
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
>>>>>>>>
>>>>>>
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20131009/0b780f01/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list