RFR(M): 8023014: closed/com/oracle/jfr/compiler/CodeSweeperSweepNoFlushTest.java fails with HS crash

[Vladimir Kozlov]

 > Second, how we can guard the initialization with a field
 > (_compiler_state and original _is_initialized) which is not static?

Answering my own question. There is only one C2Compiler and one Compiler 
(for C1) objects used by all compiler threads. They are stored in static 
AbstractCompiler* _compilers[2].

Regards,
Vladimir

On 9/25/13 3:48 PM, Vladimir Kozlov wrote:
> Thank you, Albert
>
> I would suggest to do TieredStartAtLevel changes separately, it is not
> related to this bug.
>
> 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:
>
> +  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?
>
>
> 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();
>
>
> Should we exit Compiler thread which failed local buffer allocation
> instead parking? Why keep resources allocated for it?
>
> 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