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

[erik.osterlund at oracle.com]

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;
>>>     }