On Mon, 10 Feb 2025 03:11:22 GMT, Chris Plummer <[email protected]> wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix Zero and Minimal VM builds
>
> I almost wished I hadn't looked because there is a lot of SA CodeBlob support
> that could use some cleanup. Most notably I think most of the wrapper
> subclasses are not needed by SA, and could be served by one common class. See
> what I'm doing in #23456 for JavaThread subclasses. Wrapper classes don't
> need to be 1-to-1 with the class type they are wrapping. A single wrapper
> class type can handle any number of hotspot types. It usually just make more
> sense for them to be 1-to-1, but when they are trivial and the implementation
> is replicated across subtypes, just having one wrapper class implement them
> all can simplify things.
>
> The other thing I noticed is a lot of the subtypes seem to be doing some
> unnecessary things like registering Observers, and they all do something like
> the following:
>
> 44 Type type = db.lookupType("BufferBlob");
>
> Even when it never references "type".
>
> I'm not suggesting you clean up any of this now, but just pointed it out. I
> might file an issue and try to clean it up myself at some point.
>
> I still need to take a closer look at the SA changes.
Before I forgot to answer you, @plummercj
I completely agree with your comment about cleaning up wrapper subclasses which
do nothing.
I think some wrapper subclasses for CodeBlob were kept because of `is*()` which
were used only in `PStack` to print name. Why not use `getName()` for this
purpose without big `if/else` there?
An other purpose could be a place holder for additional information in a future
which never come.
Other wrapper provides information available in `CodeBlob`. Like `RuntimeStub.
callerMustGCArguments()`. `_caller_must_gc_arguments` field is part of VM's
`CodeBlob` class for some time now. Looks like I missed change in SA when did
change in VM.
So yes, feel free to clean this up. I will help with review.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/23533#issuecomment-2652321179
