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

[Vladimir Kozlov]

On Thu, 13 Feb 2025 02:06:57 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix Zero VM build
>
> 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.

> 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.

> 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.

> 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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953732919
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953733212
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953738572
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953745389