On Wed, 12 Feb 2025 16:28:32 GMT, Vladimir Kozlov <k...@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
