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

[John R Rose]

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

I've read the code and it looks good.  I find myself wishing for a few more comments to guide me, especially in knowing which methods to pay attention to, and which to ignore as "pure plumbing".

The array of vptr-ptrs is the key element.  It seems to work nicely.

There are lots of regularizations here, which I enjoy.  But the new code has (to me) distracting irregularities.  Why define one Vptr as a struct and others as classes?  Did we really regularize the names of all the print functions (they were irregular before)?

I was glad to see lots of magic code deleted from SA.  Although, having to look at SA at all is annoying!

I noticed a lot of churn in "innocent bystander" client code that looks like this:


                   p2i(_frame.pc()), decode_offset);
-      nm()->print_on(&ss);
+      nm()->print_on_v(&ss);
       nm()->method()->print_codes_on(&ss);


What is the client maintainer (or any casual reader) supposed to get from the "_v" suffix?  I know we have made the "v/nv" distinction before, but it is rather obscure, not documeted here.  Is it described elsewhere in our code base?  Our use of it here should be docuemented in codeBlob.hpp.

Normally, we try to keep client APIs invariant while doing refactorings like this, so as to avoid touching all the client code.

In this case, we have to use a new naming convention to distinguish all versions of (say) print_on:

M. The implementation in each CB class K, which can be private if K::Vptr is a friend.
P. The public API point, used outside of the CB classes, as well as inside.
V. The name of the virtual function defined by each K::Vptr.

I would expect I to have the "nice name" like print_on, not print_on_v, while while the private method M would be print_on_impl or print_on_nv, and never called except from Vptr or other methods of the same name.  But any convention will work, as long as it is documented and held to consistently.

I'm sympathetic to both Andrew's call for maacro-enforced regularity, and Vladimir's objection that macros make things hard to follow.  If macros won't work for us here, let's define a documented pattern and stick to it closely, documenting our decisions as we go.

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

PR Comment: https://git.openjdk.org/jdk/pull/23533#issuecomment-2655760868