On Thu, 18 Aug 2022 09:32:54 GMT, Axel Boldt-Christmas <[email protected]>
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
