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

[Chris Plummer]

On Wed, 12 Feb 2025 16:28:32 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> Remove virtual methods from CodeBlob and nmethod to simplify saving/restoring in Leyden AOT cache. It avoids the need to patch hidden VPTR pointer to class's virtual table.
>> 
>> Added C++ static asserts to make sure no virtual methods are added in a future.
>> 
>> Fixed/cleaned SA code which process CodeBlob and its subclasses. Use `CodeBlob::_kind` field value to determine the type of blob.
>> 
>> Tested tier1-5, hs-tier6-rt (for JFR testing), stress, xcomp
>
> 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?

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeBlob.java line 146:

> 144:         }
> 145:       }
> 146:       return null;

Should this be 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?

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);`

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953665953
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953666268
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953667349
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1953682557