RFR: 8291237: Encapsulate nmethod Deoptimization logic [v6]

[Dean Long]

On Thu, 18 Aug 2022 09:32:54 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> 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.

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?

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

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