-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8349088: De-virtualize Codeblob and nmethod #23533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 Welcome back kvn! A progress list of the required criteria for merging this PR into |
|
@vnkozlov This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 52 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
|
@dougxc and @tkrodriguez, please look if it affects Graal. |
|
/cc graal |
|
/cc hotspot-compiler |
|
@vnkozlov |
|
@vnkozlov |
|
/cc hotspot-runtime |
|
/label remove hotspot |
|
@vnkozlov |
|
@vnkozlov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
|
||
| public class CodeCache { | ||
| private static GrowableArray<CodeHeap> heapArray; | ||
| private static VirtualConstructor virtualConstructor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for switching from the virtualConstructor/hashMap approach to using getClassFor()? The hashmap is the model for JavaThread, MetaData, and CollectedHeap subtypes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found the answer. Since there is no longer a vtable, TypeDataBase.addressTypeIsEqualToType() will no longer work for Codeblobs. I was wondering if the lack of a vtable might have some negative impact. Glad to see you found a solution. I hope the lack of a vtable does not creep in elsewhere. I know it will have some negative impact on things like the "findpc" functionality, which will no longer be able to tell the user that an address points to a Codeblob instance. There's no test for this, but users might run across it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for switching from the virtualConstructor/hashMap approach to using getClassFor()? The hashmap is the model for JavaThread, MetaData, and CollectedHeap subtypes.
I don't need any more mapping from CodeBlob class to corresponding virtual table name which does not exist anymore. CodeBlob::_kind field's value is used to determine which class should be used.
I think hashMap is overkill here. I can construct array Class<?> cbClasses[] and use cbClasses[CodeBlob::_kind] instead of if/else in getClassFor. But I would still need to check for unknown value of CodeBlob::_kind somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impact on things like the "findpc" functionality
Do you mean findpc() function in VM which is used in debugger? Nothing should be changed for it.
It calls os::print_location() which calls CodeBlob::dump_for_addr(addr, st, verbose);:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/os.cpp#L1278
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was referring to the clhsdb findpc command, which uses PointerFinder, but actually that should be ok because it special cases the codecache and knows how to find CodeBlobs in it. It's the clhsdb "inspect" command that will no longer be able to identify the type for an address that points to the start of a CodeBlob. This is true of any address that points to the start of a hotspot C++ object that does not have a vtable, or is not declared in vmstructs. So it's not a new issue, but is just adding more types to the list that "inspect" won't figure out.
I'm pretty sure JVMCI does not care about the virtual-ness of these C++ classes. Running tier9 in mach5 is a good way to be sure. |
|
|
||
| #include <type_traits> | ||
|
|
||
| // Virtual methods are not allowed in code blobs to simplify caching compiled code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth considering generating this code plus also some of the existing code in the header using an iterator template macro? e.g.
#define CODEBLOBS_DO(do_codeblob_abstract, do_codeblob_nonleaf, \
do_codeblob_leaf) \
do_codeblob_abstract(CodeBlob) \
do_codeblob_leaf(nmethod, Nmethod, nmethod) \
do_codeblob_abstract(RuntimeBlob) \
do_codeblob_nonleaf(BufferBlob, Buffer, buffer) \
do_codeblob_leaf(AdapterBlob, Adapter, adapter) \
. . . \
do_codeblob_leaf(RuntimeStub, Runtime_Stub, runtime_stub) \
. . .
The macro arguments to the templates would themselves be macros:
do_codeblob_abstract(classname) // abstract, non-instantiable class
do_codeblob_nonleaf(classname, kindname, accessorname) // instantiable, subclassable
do_codeblob_leaf(classname, kindname, accessorname) // instantiable, non-subclassable
Using a template macro like this to generate the code below -- plus also some of the code currently declared piecemeal in the header -- would guarantee all cases are covered now and will remain so later so when the macro is updated. I think it would probably also allow case handling code in AOT cache code to be generated.
So, we would generate the code here as follows
#define EMPTY1(classname)
#define EMPTY3(classname, kindname, accessorname)
#define assert_nonvirtual_leaf(classname, kindname, accessorname) \
static_assert(!std::is_polymorphic<classname>::value, \
"no virtual methods are allowed in " # classname );
CODEBLOBS_DO(empty1, empty3, assert_nonvirtual_leaf)
#undef assert_nonvirtual_leaf
Likewise in codeBlob.hpp we could generate enum CodeBlobKind to cover all the non-abstract classes and likewise generate the accessor methods is_nmethod(), is_buffer_blob() in class CodeBlob which allow the kind to be tested.
#define codekind_enum_tag(classname, kindname, accessorname) \
kindname,
enum CodeBlobKind : u1 {
None,
CODEBLOBS_DO(empty1, codekind_enum_tag, codekind_enum_tag)
Number_Of_Kinds
};
#define is_codeblob_define(classname, kindname, accessorname) \
void is_ # accessor_name () { return _kind == kindname; }
class CodeBlob {
. . .
CODEBLOBS_DO(empty1, is_codeblob_define, is_codeblob_define);
. . .
There may be other opportunities to use the iterator (e.g. in vmStructs.cpp?) but this looks like a good start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @adinn for suggestion but no, I don't like macros - hard to debug and they add more complexity in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, understood!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a similar situation with oopDesc that are not allowed to have a vtable. The solution there is to use the Klass as the proxy vtable and then have a bunch of Klass::oop_ functions that act like virtual dispatch functions for associated oopDesc functions.
I wonder if a similar approach can be use here? Such an approach would (to me at lest) have the benefit that we don't have to spread switch statements in various functions in the top-most class.
If you are interested in seeing a prototype of this, take a look at this branch:
master...stefank:jdk:code_blob_vptr
Just a suggestion if you want to consider alternatives to these switch statements.
|
Thank you, @stefank. This is very interesting suggestion which I may take. I will check it. |
|
Before I forgot to answer you, @plummercj I think some wrapper subclasses for CodeBlob were kept because of An other purpose could be a place holder for additional information in a future which never come. Other wrapper provides information available in So yes, feel free to clean this up. I will help with review. |
| INTPTR_FORMAT " not found or invalid at %d", | ||
| p2i(_frame.pc()), decode_offset); | ||
| nm()->print_on(&ss); | ||
| nm()->print_on_v(&ss); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest removing _v suffix to reduce changes and match existing naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Testing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HotSpot C++ changes look good. I skipped SA changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I addressed most @xmas92 and @dean-long comments and working on avoid _v suffix
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/CodeCache.java
Outdated
Show resolved
Hide resolved
src/hotspot/share/code/codeBlob.hpp
Outdated
| struct Vptr { | ||
| virtual void print_on(const CodeBlob* instance, outputStream* st) const { | ||
| instance->print_on_nv(st); | ||
| } | ||
| virtual void print_value_on(const CodeBlob* instance, outputStream* st) const { | ||
| instance->print_value_on_nv(st); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/hotspot/share/code/codeBlob.hpp
Outdated
|
|
||
| void print_on(outputStream* st) const override; | ||
| void print_value_on(outputStream* st) const override; | ||
| class Vptr : public CodeBlob::Vptr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/hotspot/share/code/codeBlob.hpp
Outdated
| void print_on(outputStream* st) const; | ||
| void print_value_on(outputStream* st) const; | ||
|
|
||
| class Vptr : public CodeBlob::Vptr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/hotspot/share/code/codeBlob.hpp
Outdated
|
|
||
| void print_on(outputStream* st) const override; | ||
| void print_value_on(outputStream* st) const override; | ||
| class Vptr : public CodeBlob::Vptr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/hotspot/share/code/codeBlob.hpp
Outdated
|
|
||
| void print_value_on(outputStream* st) const; | ||
|
|
||
| class Vptr : public CodeBlob::Vptr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/hotspot/share/code/codeBlob.hpp
Outdated
| void print_on(outputStream* st) const; | ||
| void print_value_on(outputStream* st) const; | ||
|
|
||
| class Vptr : public CodeBlob::Vptr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
Thank you, Dean, for review. |
|
I removed I renamed I made I added empty I also did some arguments renaming in SA in Tier1-5 testing passed. Ready for new round of reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not looked at the SA changes.
lgtm.
|
|
||
| class Vptr : public CodeBlob::Vptr { | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this needed for some compiler? Or is it to be more explicit about the type hierarchy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @xmas92, for review and suggestions.
It is second (explicit type hierarchy). I think it should be explicitly declared (even empty) because it is referenced in subclasses to avoid confusion. And it could be useful in a future if we need other virtual methods.
Local build with gcc on Linux passed without it but I did not try to build on other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SA changes look good. Thanks for taking care of this.
|
Thank you, @plummercj , for review. |
|
Thank you all for reviews and suggestions. |
|
/integrate |
|
Going to push as commit 93b443ae36876bc182e06e142742945c65fe5349.
Your commit was automatically rebased without conflicts. |
|
@vnkozlov An unexpected error occurred during integration. No push attempt will be made. The error has been logged and will be investigated. It is possible that this error is caused by a transient issue; feel free to retry the operation. |
|
/integrate |
|
Going to push as commit 46d4a60.
Your commit was automatically rebased without conflicts. |
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::_kindfield value to determine the type of blob.Tested tier1-5, hs-tier6-rt (for JFR testing), stress, xcomp
Progress
Issue
Reviewers
Contributors
<stefank@openjdk.org><cjplummer@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23533/head:pull/23533$ git checkout pull/23533Update a local copy of the PR:
$ git checkout pull/23533$ git pull https://git.openjdk.org/jdk.git pull/23533/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23533View PR using the GUI difftool:
$ git pr show -t 23533Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23533.diff
Using Webrev
Link to Webrev Comment