RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash
Christian Thalinger
christian.thalinger at oracle.com
Thu Oct 10 14:24:33 PDT 2013
Looks good.
On Oct 8, 2013, at 10:00 PM, Albert Noll <albert.noll at oracle.com> 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/
>
> 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/20131010/eb370379/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list