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

[Erik Osterlund]

Hi Coleen,

Thanks for the review.

/Erik

> On 17 Jul 2019, at 22:19, coleen.phillimore at oracle.com wrote:
> 
> 
> 
>> 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
>>>> 
>> 
>