RFR: 8291237: Encapsulate nmethod Deoptimization logic [v6]

[Axel Boldt-Christmas]

On Fri, 19 Aug 2022 01:17:14 GMT, Dean Long <dlong at openjdk.org> wrote:

>> I think you have to describe the scenario that does not work, because I am not sure I see it.
>> 
>> For ease of writing, let me call the currently embedded status `status` and the currently embedded link `next_link` 
>> (`=>` means implies here)
>> 
>> The assert checks this invariant: `status == not_enqueued => next_link == nullptr`
>> 
>> After adding the first method (which will be the tail) to the list (`root == nullptr`) we will have:
>>  * `status > not_enqueued`
>>  * `next_link == nullptr`
>>  
>> After adding any method after that ( `root != nullptr`) we will have:
>>  * `status > not_enqueued`
>>  * `next_link == previous_root`
>>  
>>  And there is also a fresh method
>>  * `status == not_enqueued`
>>  * `next_link == nullptr`
>> 
>> If we try to enqueue an already enqueued method (calling `enqueue_deoptimization` twice) the method will have  `status != not_enqueued` and will set `next_link == next_link` if `new_status > status`. (So just update the status to a stronger variant, but not change the link)
>> 
>> All of these possibilities upholds the invariant. 
>> 
>> Maybe you are looking for some more invariant checks, like
>> `status > not_enqueued => InList(method)` which can be asserted by setting `next_link == this` for the tail (when  `root == nullptr`) and assert that `next_link != nullptr` if `status > not_enqueued`. But it just seems like we are adding another flag for something we already check, essentially `next_link != nullptr` iff `status > not_enqueued`.
>> 
>> There is currently an assert when we iterate over the list; that the number of methods we find in the list is equal to number of methods we enqueued.
>
> Yes, something like (status == not_enqueued) == !InList(method), which is strong than the current =>, and would be a sanity check on the status.
> 
>> There is currently an assert when we iterate over the list; that the number of methods we find in the list is equal to number of methods we enqueued.
> 
> That wouldn't necessarily catch accessing stale nmethod memory that has been released by CodeCache::free(), would it?

Yeah the point of that assert was simply to check the sanity of the iteration implementation. 

The validity of the actual pointer is suppose to be guaranteed by the deoptimization context. I think adding asserts that we are inside deoptimization context would be good. 

You have a point about nmethods becoming stale. In the world with the sweeper my understanding is that if we add a `is_alive` method to the list it will not be flushed and freed until after the next safepoint, which the context ensures does not happen until the list is processed. And in the world after the sweeper the nmethods are just stable if no safepoint occurs. Because they would be unlinked in an earlier phase so we do not see them while iterating the code cache, and when walking dependency contexts it only returns not unloading nmethods. 

Maybe this deoptimization interface should just explicitly not add not alive, or unloading methods. Or just assert it and leave it up to the users of the interface. I think that invariant is upheld today. But I will investigate. Maybe some of the whitebox APIs.

Erik will be back next week so I will discuss this with him.

-------------

PR: https://git.openjdk.org/jdk/pull/9655