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

[Robbin Ehn]

Thanks Tobias and Erik.

/Robbin

On 9/27/19 2:43 PM, Tobias Hartmann wrote:
> +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;
>>>>>>>      }