RFR(XS): 8024008: Nashorn V8 (Crypto) benchmark crashes with small CodeCache size
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Oct 17 15:09:08 PDT 2013
Albert,
You are mistaking. AdapterBlob::create(&buffer) is used only during code
generation (the same as temp buffer for C2 and C1).
The nmethod::new() nmethod(), called from nmethod::new_native_nmethod(),
will relocate the code to CodeCache. So we need space in CodeCache for
adapters and native wrappers.
Vladimir
On 10/17/13 1:09 PM, Albert Noll wrote:
> Hi,
>
> here is an updated webrev: let me explain the changes here:
> I strongly assume that these changes will solve
> https://bugs.openjdk.java.net/browse/JDK-8026407
>
> CompileBroker.cpp:
>
> * if (!should_compile_new_jobs()) {*
> * CompilationPolicy::policy()->delay_compilation(method());*
> * return NULL;*
> * }*
>
> I think these lines are wrong and we must create the wrapper.
>
> * AdapterHandlerLibrary::create_native_wrapper(method, compile_id);*
>
>
> We need to create the wrapper to be able to call a native method from
> compiled code.
> Furthermore, the wrapper is not allocated form the code cache. It is
> allocated from
> a pre-allocated piece of memory.
>
> sharedRuntime.cpp
>
> * // CodeCache is full, disable compilation*
> * // Ought to log this but compile log is only per compile thread*
> * // and we're some non descript Java thread.*
> * MutexUnlocker mu(AdapterHandlerLibrary_lock);*
> *! CompileBroker::handle_full_code_cache();*
> *- return NULL; // Out of CodeCache space*
>
>
> This code is useless, since *AdapterBlob::create(&buffer) *allocates from
> a pre-allocated chunk of memory and not from the 'normal code cache'. In
> addition, this code caused some errors, since handle_full_code_cache() calls
> the sweeper. The calling thread needs to be in the correct state and is
> not allowed to
> own certain locks. This can cause problems.
>
> Same here:
>
> * } else {*
> * // CodeCache is full, disable compilation*
> * CompileBroker::handle_full_code_cache();*
>
>
> In addition, I introduced a new product flag, AdapterHandlerLibrarySize .
> The reason is that when running nashorn with very small code cache size
> (e.g., <5m),
> the error message that there is not enoug memory the hold adapters is
> quite frequently.
> Also, I think that having that value hidden in sharedRuntime.cpp is not
> the best way. I doubled
> the default size to 32K. This is sufficient to execute nashorn with a
> code cache size of 3500k.
>
>
> On 07.10.2013 20:15, Albert Noll wrote:
>> 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).
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
>
More information about the hotspot-compiler-dev
mailing list