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

[Robbin Ehn]

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