RFR[13]: 8224674: NMethod state machine is not monotonic

[coleen.phillimore at oracle.com]

On 7/17/19 3:24 PM, dean.long at oracle.com wrote:
> On 7/17/19 9:59 AM, Erik Österlund wrote:
>> Hi Dean,
>>
>> You are correct that the winner of the race will already have made 
>> sure the operations you listed have run. So by not returning you 
>> would essentially just continue performing a bunch of unnecessary 
>> operations that won't do anything.
>>
>> I would prefer though to stay as close to my original fix as possible 
>> as that one has gone through extensive testing, and I would like to 
>> push this before the cut-off for P3 bugs to 13 tomorrow.
>>
>> Here is my latest attempt:
>> http://cr.openjdk.java.net/~eosterlund/8224674/webrev.03/
>>
>> Here is an incremental webrev to the original proposition:
>> http://cr.openjdk.java.net/~eosterlund/8224674/webrev.00_03/
>>
>> As you can see I have only changed the guarantee to assert as 
>> requested by Coleen, and added a bunch of commentary to explain why 
>> e.g. a failing transition does not need to worry about the 3 side 
>> effects before the failure. Hope my comments make sense.
>>
>> I hope you think this is okay. If there is more clarification or 
>> cleanups you would like to see, I am more than happy to file such 
>> RFEs for 14.
>>
>
> I'm OK with this, but yes, please file an RFE for cleanup in 14. It's 
> not obvious to me that make_not_entrant and make_unloaded are doing 
> the same operations.  Some refactoring here should help a lot.

I agree.  I had to reread the code to see it (and ask Erik!) and it 
still looks different.  The change looks good for 13 though, and the 
comments are helpful.

Thanks,
Coleen

>
> dl
>
>> Thanks,
>> /Erik
>>
>> On 2019-07-17 09:17, dean.long at oracle.com wrote:
>>> On 7/16/19 10:51 AM, dean.long at oracle.com wrote:
>>>> Back to the make_not_entrant / make_unloaded race.  If 
>>>> make_not_entrant bails out half-way through because make_unloaded 
>>>> won the race, doesn't that mean that make_unloaded needs to have 
>>>> already done all the work that make_not_entrant is not doing? 
>>>> unlink_from_method, invalidate_nmethod_mirror, remove_osr_nmethod, 
>>>> unregister_nmethod, etc. 
>>>
>>> What I'm thinking is, what happens if instead of this:
>>>
>>> 1365     // Change state
>>> 1366 if (!try_transition(state)) {
>>> 1367 return false;
>>> 1368 }
>>> we do this: 1365     // Maybe change state
>>> 1366 if (!try_transition(state)) {
>>> 1367 // fall through 1368 }
>>>
>>> dl
>>>
>