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

[Vladimir Kozlov]

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);
      }
    }

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).

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
>