RFR(one-liner): 8231321: compiler/codecache/stress/UnexpectedDeoptimizationAllTest.java failed assertion

[erik.osterlund at oracle.com]

Hi Robbin,

Awesome. Looks good.

Thanks,
/Erik

On 9/27/19 11:25 AM, Robbin Ehn wrote:
> Hi Erik,
>
> Full v2:
> http://cr.openjdk.java.net/~rehn/8231321/v2/webrev/
>
> Passes 100 UnexpectedDeoptimizationAllTest.
>
> Thanks, Robbin
>
> On 9/25/19 3:10 PM, erik.osterlund at oracle.com wrote:
>> Hi Robbin,
>>
>> I think moving in the OSR nmethod invalidation under the lock is 
>> enough; no need to clutter the transitioning code as well.I presume 
>> this webrev was a delta to the one that removed the false positive 
>> assert?
>>
>> Thanks,
>> /Erik
>>
>> On 9/25/19 2:44 PM, Robbin Ehn wrote:
>>> Hi,
>>>
>>> On 9/25/19 10:58 AM, Tobias Hartmann wrote:
>>>> Hi Robbin,
>>>>
>>>> please note that we also hit the following assert (see comments in 
>>>> bug report):
>>>> Crash: Internal Error ...nmethod.cpp...assert(!found) failed: osr 
>>>> nmethod should have been invalidated
>>>
>>> Fixed both:
>>> http://cr.openjdk.java.net/~rehn/8231321/v1/webrev/
>>>
>>> The other problem was an osr method that is not_installed and gets 
>>> marked for
>>> deopt. We call make_not_entrant_or_zombie where we first remove the 
>>> osr method
>>> before changing state to not_entrant. Before we changed state but 
>>> after we tried
>>> to remove the osr method we make this osr method in use and install 
>>> the code.
>>> After we made it not entrant we check that there is no osr method, 
>>> but due to
>>> above it is and we assert.
>>>
>>> I did a double fix by both moving the removal of osr into the same 
>>> lock scope as
>>> state change, since now we use same lock there is no issue with that.
>>> Also we should not install a method marked for deopt.
>>>
>>> It passes many iterations of UnexpectedDeoptimizationAllTest.java, 
>>> locally and
>>> in mach5. Started a pass with t3-5.
>>>
>>> Thanks, Robbin
>>>
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>> On 25.09.19 10:12, Robbin Ehn wrote:
>>>>> Hi all, please review.
>>>>>
>>>>> We no longer allow state to ever go backwards.
>>>>> So trying to make a zombie not-entrant is fine.
>>>>> What happens is something like:
>>>>> - make not-entrant
>>>>> - safepoint
>>>>> - deopt all => start iteration over nmethods
>>>>> - picks up this not-entrant method, since it is alive
>>>>> - sweeper turns it into a zombie
>>>>> - we try making the nmethod not-entrant once more => assert
>>>>>
>>>>> If we remove the assert the method would just returns false 
>>>>> instead, which is expected.
>>>>>
>>>>> Thanks, Robbin
>>>>>
>>>>> Issue:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8231321
>>>>>
>>>>> Changeset:
>>>>> diff -r 448fe2bfd505 src/hotspot/share/code/nmethod.cpp
>>>>> --- a/src/hotspot/share/code/nmethod.cpp    Tue Sep 24 08:54:39 
>>>>> 2019 +0200
>>>>> +++ b/src/hotspot/share/code/nmethod.cpp    Wed Sep 25 10:02:45 
>>>>> 2019 +0200
>>>>> @@ -1288,17 +1288,16 @@
>>>>>     }
>>>>>   }
>>>>>
>>>>>   /**
>>>>>    * Common functionality for both make_not_entrant and make_zombie
>>>>>    */
>>>>>   bool nmethod::make_not_entrant_or_zombie(int state) {
>>>>>     assert(state == zombie || state == not_entrant, "must be 
>>>>> zombie or not_entrant");
>>>>> -  assert(!is_zombie(), "should not already be a zombie");
>>>>>
>>>>>     if (Atomic::load(&_state) >= state) {
>>>>>       // Avoid taking the lock if already in required state.
>>>>>       // This is safe from races because the state is an end-state,
>>>>>       // which the nmethod cannot back out of once entered.
>>>>>       // No need for fencing either.
>>>>>       return false;
>>>>>     }