On Thu, 18 Aug 2022 09:32:54 GMT, Axel Boldt-Christmas <abold...@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