RFR: 8349088: De-virtualize Codeblob and nmethod [v6]

[Chris Plummer]

On Thu, 13 Feb 2025 03:26:19 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java line 118:
>> 
>>> 116:   }
>>> 117: 
>>> 118:   public static Class<?> getClassFor(Address addr) {
>> 
>> Did you consider using a lookup table here that is indexed using the kind value?
>
> Example please.

static Class wrapperClasses = new Class[Number_Of_Kinds];
    wrapperClasses[NMethodKind] = NMethodBlob.class;
    wrapperClasses[BufferKind] = BufferBopb.class;
    ...;
    wrapperClasses[SafepointKind] = SafepointBlob.class;



    CodeBlob cb = new CodeBlob(addr);
    return wrapperClasses[cb.getKind()];

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java line 146:
>> 
>>> 144:         }
>>> 145:       }
>>> 146:       return null;
>> 
>> Should this be an assert?
>
> I don't think we need it - the caller `CodeCache.createCodeBlobWrapper()` will throw `RuntimeException` when `null` is returned.

I guess my real question is whether or not it can be considered normal behavior to return null. It seems it should never happen, which is why I was suggesting an assert.

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java line 213:
>> 
>>> 211: 
>>> 212:   public boolean isUncommonTrapBlob()   {
>>> 213:     if (!VM.getVM().isServerCompiler()) return false;
>> 
>> Why is the check needed? Why not just return the value `getKind() == UncommonTrapKind` result below?
>
> `UncommonTrapKind` and `ExceptionKind` are not initialized for Client VM because corresponding `CodeBlobKind` values are not defined. See `CodeBlob.initialize()`.
> Their not initialized value will be 0 which matches `CodeBlobKind::None` value. Returning true in such case will be incorrect.

Ok. Leaving UncommonTrapKind and ExceptionKind uninitialized seems a bit error prone. Perhaps they can be given some sort of INVALID value.

>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeCache.java line 95:
>> 
>>> 93:   }
>>> 94: 
>>> 95:   public CodeBlob createCodeBlobWrapper(Address cbAddr, Address start) {
>> 
>> I think the use of the name "start" here is a carryover from `findBlobUnsafe(Address start)`. I find it a very misleading name. cbAddr points to the "start" of the blob. "start" points somewhere in the middle of the blob. In fact callers of this API somethimes pass in findStart(addr) for cbAddr, which just adds to the confusion. Perhaps this is a good time to rename "start" to something else, although I can't come up with a good suggestion, but I think anything other than "start" would be an improvement. Maybe "pcAddr".  That aligns with the "for PC=" message below. Or maybe just "ptr" which aligns with `createCodeBlobWrapper(findStart(ptr), ptr);`
>
> `cbPc` with comment explaining that it could be inside code blob.

That sounds fine.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953818292
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953819796
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953821968
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953822595