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

[Erik Osterlund]

Hi Dean,

Thanks for the review. I will file an enhancement to refactor the code. 
I think what we need to make this more intuitive is to have a new unlinking function. More on that in 14...

Thanks,
/Erik

> On 17 Jul 2019, at 21:24, 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.
> 
> 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
>>> 
>