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

[Tobias Hartmann]

+1

Best regards,
Tobias

On 27.09.19 11:30, erik.osterlund at oracle.com wrote:
> 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;
>>>>>>     }