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

[Albert Noll]

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