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

[Axel Boldt-Christmas]

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

Similar to what @rose00 noted I think the `_v` and `_nv` suffixes are unfortunate in the public API.

Maybe it we could add a protected `x_impl` containing the implementation, then dispatch to the correct one based on _kind, using the Vptr abstraction. And have the normal print_on method use this. We could let our leaf types to directly call the specific implementation, not that I think that our print functions require compile time devirtualisation. 

There are many solutions here with their pros and cons.

src/hotspot/share/code/codeBlob.hpp line 140:

> 138:       instance->print_value_on_nv(st);
> 139:     }
> 140:   };

I wonder why the base class is not abstract. AFAICT `print_value_on` is unreachable and `print_on` is only used by `DeoptimizationBlob::Vptr` which also seems like a behavioural change, as before this patch calling `print_on` a `DeoptimizationBlob` object would dispatch to `SingletonBlob::print_on` not `CodeBlob::print_on`. 

Suggestion:

 struct Vptr {
    virtual void print_on(const CodeBlob* instance, outputStream* st) const = 0;
    virtual void print_value_on(const CodeBlob* instance, outputStream* st) const = 0;
  };

src/hotspot/share/code/codeBlob.hpp line 339:

> 337:   void print_value_on(outputStream* st) const;
> 338: 
> 339:   class Vptr : public CodeBlob::Vptr {

I wonder if these should share the same type hierarchy as tier container class. This would also solve the issueI noted in my other comment about not calling the correct `print_on`.
Suggestion:

  class Vptr : public RuntimeBlob::Vptr {

src/hotspot/share/code/codeBlob.hpp line 427:

> 425:   void print_value_on(outputStream* st) const;
> 426: 
> 427:   class Vptr : public CodeBlob::Vptr {

Suggestion:

  class Vptr : public RuntimeBlob::Vptr {

src/hotspot/share/code/codeBlob.hpp line 467:

> 465:   void print_value_on(outputStream* st) const;
> 466: 
> 467:   class Vptr : public CodeBlob::Vptr {

Suggestion:

  class Vptr : public RuntimeBlob::Vptr {

src/hotspot/share/code/codeBlob.hpp line 553:

> 551:   void print_value_on(outputStream* st) const;
> 552: 
> 553:   class Vptr : public CodeBlob::Vptr {

This one specifically
Suggestion:

  class Vptr : public SingletonBlob::Vptr {

src/hotspot/share/code/codeBlob.hpp line 679:

> 677:   void print_value_on(outputStream* st) const;
> 678: 
> 679:   class Vptr : public CodeBlob::Vptr {

Suggestion:

  class Vptr : public RuntimeBlob::Vptr {

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

PR Review: https://git.openjdk.org/jdk/pull/23533#pullrequestreview-2614177723
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1954019308
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1954024528
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1954028620
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1954028940
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1954027733
PR Review Comment: https://git.openjdk.org/jdk/pull/23533#discussion_r1954029504