On Thu, 18 Aug 2022 08:57:19 GMT, Dean Long <[email protected]> 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
