RFR(XS): 8024008: Nashorn V8 (Crypto) benchmark crashes with small CodeCache size
Albert Noll
albert.noll at oracle.com
Thu Oct 17 13:09:19 PDT 2013
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).
>>>>>>>>
>>>>>>
>>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20131017/3511f4ad/attachment.html
More information about the hotspot-compiler-dev
mailing list