RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash
Albert Noll
albert.noll at oracle.com
Tue Oct 1 00:27:59 PDT 2013
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/20131001/8dabf887/attachment-0001.html
More information about the hotspot-compiler-dev
mailing list