RFR: 8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite()

[Erik Österlund]

On Fri, 21 Oct 2022 07:04:03 GMT, Dean Long <dlong at openjdk.org> wrote:

>> src/hotspot/share/runtime/sharedRuntime.cpp line 2113:
>> 
>>> 2111:   NoSafepointVerifier nsv;
>>> 2112: 
>>> 2113:   CompiledMethod* callee = moop->code();
>> 
>> There is a moop->code() null check just a few lines below, so now it looks like we are reading the code pointer twice checking if it is null. Is ot enough to do that one time?
>
> It's actually the same number of null checks as before, if you look at what from_compiled_entry_no_trampoline() used to do.  But I did consider removing the 2nd check, because no matter how late we check, we can always lose the race where it becomes null right after our last check.  It's harmless however, so I decided to keep it.

Okay. That seems fine.

>> src/hotspot/share/runtime/sharedRuntime.cpp line 2119:
>> 
>>> 2117: 
>>> 2118:   CodeBlob* cb = CodeCache::find_blob(caller_pc);
>>> 2119:   if (cb == NULL || !cb->is_compiled() || callee->is_unloading()) {
>> 
>> Why not move the is_unloading check on callee to the if statement just above that checks the callee (as opposed to the callsite)?
>
> I guess I was thinking is_unloading() can be a bit expensive the first time it is called, so it might be better to fail for other reasons first.  But I believe is_unloading will eventually be called for every nmethod each unloading cycle, so avoiding the cost here just means moving it to somewhere else.  I can move it to where you suggest if you like.

Okay I see. I'll leave it to you to decide if you prefer to move it or not. I'm okay with it either way.

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

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