RFR: 8291237: Encapsulate nmethod Deoptimization logic [v6]

[Axel Boldt-Christmas]

On Thu, 18 Aug 2022 08:57:19 GMT, Dean Long <dlong at openjdk.org> wrote:

>> Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Cleanup comment
>
> src/hotspot/share/code/compiledMethod.cpp line 128:
> 
>> 126:   if (old_status < new_status) {
>> 127:     if (old_status == not_enqueued) {
>> 128:       assert(extract_enqueued_deoptimization_method(_enqueued_deoptimization_link) == nullptr, "Compiled Method should not already be linked");
> 
> This doesn't work for the tail of the list, does it?  That's why I suggested making the tail loop back to itself
> 
> I would also like to see similar asserts added to make sure we don't recycle an nmethod that is still on the list, perhaps in nmethod::flush().

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.

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

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