RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash
Albert Noll
albert.noll at oracle.com
Wed Oct 2 02:47:46 PDT 2013
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/20131002/d306c519/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list