RFR(XS): 8024008: Nashorn V8 (Crypto) benchmark crashes with small CodeCache size

[Albert Noll]

Hi Valdimir,

thanks for you feedback. If the generation of the AdapterHandlerLibrary 
wrapper is optional, the following changes in compileBroker.cpp should work:


   // If the compiler is shut off due to code cache getting full
   // fail out now so blocking compiles dont hang the java thread
  // if ( !should_compile_new_jobs()) {
  //   CompilationPolicy::policy()->delay_compilation(method());
   //  return NULL;
  // }

   // do the compilation
   if (method->is_native()) {
     if (!PreferInterpreterNativeStubs || 
method->is_method_handle_intrinsic()) {
       // Acquire our lock.
     //  int compile_id;
     //  {
     //    MutexLocker locker(MethodCompileQueue_lock, THREAD);
     //    compile_id = assign_compile_id(method, standard_entry_bci);
      // }
      // (void) AdapterHandlerLibrary::create_native_wrapper(method, 
compile_id);
     } else {
       return NULL;
     }
   } else {
     compile_method_base(method, osr_bci, comp_level, hot_method, 
hot_count, comment, THREAD);
   }

However, with the changes above, I get the "i2c adapter must return to 
an interpreter frame" even
if the code cache is not full and compilation remains enabled. Something 
seems really broken here.

Irrespective of the current bug, it seems that checking whether we 
should compile new methods
in line 1304 in compileBroker.cpp does not make much sense, since this 
bit can change asynchronously
any time. It made more sense before 8020151, but now I think it is time 
to remove it.

Best,
Albert

On 03.10.2013 21:27, Vladimir Kozlov wrote:
> Albert,
>
> I don't see connection between not generating wrapper (which is fine, 
> it is only needed for hot calls to native method) and wrong return 
> address when calling i2c adapter (failed message). Can you find what 
> return address is?
>
> It could be a different problem (or not) from what this bug about. 
> Several things can go bad when codecache is full. Also attached to bug 
> report hs_err files shows different crushes.
>
> Was you able to reproduce it with fastdebug VM? Switch off 
> -XX:-CheckCompressedOops to get the same code generation as with 
> product 64-bit VM. And switch off -XX:-VerifyDependencies to speed 
> testing (10x, this test is very expensive).
>
> Also can you try to run with latest VM version which has fixes to 
> produce more info for call stack in hs_err. To have only next lines is 
> not useful:
>
> C  0x0000000648baace0
> [error occurred during error reporting (printing native stack), id 0xb]
>
> Thanks,
> Vladimir
>
> On 10/3/13 11:17 AM, Albert Noll wrote:
>> Hi,
>>
>> I think I found the reason for the crash. Please forget the previous
>> suggestions.
>>
>> See the following webrev:
>>
>> Problem:
>>    http://cr.openjdk.java.net/~anoll/8024008/webrev.02/
>>
>>    If the code cache gets full, this code in compileBroker.cpp is not
>> executed.
>>
>>    1318       (void)
>> AdapterHandlerLibrary::create_native_wrapper(method, compile_id);
>>
>>    As a result, we get the following error "i2c adapter must return to
>> an interpreter frame"
>>    if the JVM is called with "-XX:+UnlockDiagnosticVMOptions
>> -XX:+VerifyAdapterCalls"
>>
>>    I do not yet fully understand why that's the case, so please if you
>> know let me know.
>>
>> Solution:
>>    Move the code cache is full-check  after line 1318.
>>
>> Many thanks,
>> Albert
>>
>>
>> Am 11.09.2013 01:07, schrieb Vladimir Kozlov:
>>> Albert,
>>>
>>> I would suggest to run as many tests as possible to verify correctness
>>> of changes because this code is very important.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 9/10/13 5:52 AM, Albert Noll wrote:
>>>> Hi,
>>>>
>>>> the current version causes a serious performance regression.
>>>> I have to figure out what's wrong.
>>>>
>>>> Best,
>>>> Albert
>>>>
>>>> On 10.09.2013 09:03, Albert Noll wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> thanks for looking at this. I checked all call sites of
>>>>> 'make_not_entrant()' and 'make_zombie()'. I do not see any problems.
>>>>> The worst thing that can happen is that an nmethod for which the call
>>>>> returns fals gets removed later than expected.
>>>>>
>>>>> I updated the comment and did some minor code refactoring.
>>>>>
>>>>> Also I think that it is worth considering to backport this change,
>>>>> since the bug can appear any time
>>>>> the code cache fills up. If that bug appears, it is extremely hard to
>>>>> reproduce and debug.
>>>>>
>>>>> Here is the updated webrev.
>>>>> http://cr.openjdk.java.net/~anoll/8024008/webrev.01/
>>>>> <http://cr.openjdk.java.net/%7Eanoll/8024008/webrev.01/>
>>>>>
>>>>> Best,
>>>>> Albert
>>>>>
>>>>>
>>>>>
>>>>> On 09.09.2013 19:47, Vladimir Kozlov wrote:
>>>>>> Several places in sources do not check returned result of
>>>>>> make_not_entrant_or_zombie(). And they may assume that method marked
>>>>>> as non-entrant, for example, because before we returned 'false' only
>>>>>> when an other thread already changed the state to one we need.
>>>>>>
>>>>>> Can you check all call sites to make sure they work correctly with
>>>>>> this change?
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 9/9/13 6:59 AM, Albert Noll wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> thanks for reviewing this patch.
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8024008
>>>>>>> http://cr.openjdk.java.net/~anoll/8024008/webrev.00/
>>>>>>> <http://cr.openjdk.java.net/%7Eanoll/8024008/webrev.00/>
>>>>>>>
>>>>>>> Many thanks in advance,
>>>>>>> Albert
>>>>>>>
>>>>>>> Problem:
>>>>>>> The state of nmethods that are currently locked by the VM must not
>>>>>>> change.
>>>>>>> Some operations such as setting ICs (e.g.,
>>>>>>> SharedRuntime::resolve_sub_helper())
>>>>>>> rely on this fact. 'nmethod::make_not_entrant_or_zombie does not
>>>>>>> make this
>>>>>>> check.
>>>>>>>
>>>>>>> Solution:
>>>>>>> Do method state unchanged of nmethod is locked by the VM.
>>>>>>>
>>>>>>> Testing: Run Octane benchmarks on top of Nashorn with small code
>>>>>>> cache size (16m).
>>>>>>>
>>>>>
>>>>
>>