Skip to content
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

8348426: Generate binary file for -XX:AOTMode=record -XX:AOTConfiguration=file #23484

Closed
wants to merge 15 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/hotspot/share/cds/archiveUtils.inline.hpp
Original file line number Diff line number Diff line change
@@ -66,6 +66,10 @@ Array<T>* ArchiveUtils::archive_non_ptr_array(GrowableArray<T>* tmp_array) {
}

// Returns the address of an Array<T> that's allocated in the ArchiveBuilder "buffer" space.
// All pointers in tmp_array must point to:
// - a buffered object; or
// - a source object that has been archived; or
// - (only when dumping dynamic archive) an object in the static archive.
template <typename T>
Array<T>* ArchiveUtils::archive_ptr_array(GrowableArray<T>* tmp_array) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am reading this code correctly it requires that the elements in tmp_array have already been archived. Can we add a comment and/or an assert to that effect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments about what the elements can be.

// All pointers in tmp_array must point to:
//    - a buffered object; or
//    - a source object that has been archived; or
//    - (only when dumping dynamic archive) an object in the static archive.

If it's not one of these types, it will be caught by the assert inside builder->get_buffered_addr(ptr)

ArchiveBuilder* builder = ArchiveBuilder::current();
2 changes: 1 addition & 1 deletion src/hotspot/share/cds/cdsConfig.cpp
Original file line number Diff line number Diff line change
@@ -547,7 +547,7 @@ bool CDSConfig::is_dumping_classic_static_archive() {
}

bool CDSConfig::is_dumping_preimage_static_archive() {
return _is_dumping_static_archive && _is_dumping_preimage_static_archive;
return _is_dumping_preimage_static_archive;
}

bool CDSConfig::is_dumping_final_static_archive() {
21 changes: 16 additions & 5 deletions src/hotspot/share/cds/cppVtables.cpp
Original file line number Diff line number Diff line change
@@ -189,13 +189,22 @@ enum ClonedVtableKind {
_num_cloned_vtable_kinds
};

// This is a map of all the original vtptrs. E.g., for
// _orig_cpp_vtptrs and _archived_cpp_vtptrs are used for type checking in
// CppVtables::get_archived_vtable().
//
// _orig_cpp_vtptrs is a map of all the original vtptrs. E.g., for
// ConstantPool *cp = new (...) ConstantPool(...) ; // a dynamically allocated constant pool
// the following holds true:
// _orig_cpp_vtptrs[ConstantPool_Kind] == ((intptr_t**)cp)[0]
static intptr_t* _orig_cpp_vtptrs[_num_cloned_vtable_kinds]; // vtptrs set by the C++ compiler
static intptr_t* _archived_cpp_vtptrs[_num_cloned_vtable_kinds]; // vtptrs used in the static archive
// _orig_cpp_vtptrs[ConstantPool_Kind] == ((intptr_t**)cp)[0]
//
// _archived_cpp_vtptrs is a map of all the vptprs used by classes in a preimage. E.g., for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding these comments. I think I now understand why we need _archived_cpp_vtptrs.
I am wondering if we really need to store this table in the preimage.
When the control enters CppVtables::dumptime_init, if we are dumping the final archive, then the _index[kind].cloned_vtable() would be pointing to the vtable in the preimage. So we can initialize the _archived_cpp_vtptrs at that time before _index[kind] is overwritten by the runtime vtable data.
Wouldn't that work?

Something like this:

 void CppVtables::dumptime_init(ArchiveBuilder* builder) {
   assert(CDSConfig::is_dumping_static_archive(), "cpp tables are only dumped into static archive");
 
-  CPP_VTABLE_TYPES_DO(ALLOCATE_AND_INITIALIZE_VTABLE);
-
-  if (!CDSConfig::is_dumping_final_static_archive()) {
+  // When dumping final archive, _index[kind] at this point is in the preimage.
+  // Store the vtable pointers present in the preimage  as _index[kind] will now be rewritten
+  // to point to the runtime vtable data.
+  if (CDSConfig::is_dumping_final_static_archive()) {
     for (int kind = 0; kind < _num_cloned_vtable_kinds; kind++) {
       _archived_cpp_vtptrs[kind] = _index[kind]->cloned_vtable();
     }
   }
+  CPP_VTABLE_TYPES_DO(ALLOCATE_AND_INITIALIZE_VTABLE);
 
   size_t cpp_tables_size = builder->rw_region()->top() - builder->rw_region()->base();
   builder->alloc_stats()->record_cpp_vtables((int)cpp_tables_size);
@@ -253,16 +255,6 @@ void CppVtables::serialize(SerializeClosure* soc) {
   if (soc->reading()) {
     CPP_VTABLE_TYPES_DO(INITIALIZE_VTABLE);
   }
-
-  if (soc->writing() && !CDSConfig::is_dumping_preimage_static_archive()) {
-    // This table is written only when creating the preimage. It will be used
-    // only when writing the final static archive.
-    memset(_archived_cpp_vtptrs, 0, sizeof(_archived_cpp_vtptrs));
-  }
-
-  for (int kind = 0; kind < _num_cloned_vtable_kinds; kind++) {
-    soc->do_ptr(&_archived_cpp_vtptrs[kind]);
-  }
 }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I've incorporated your patch and added an assert. I also fixed CppVtables::is_valid_shared_method() to use _archived_cpp_vtptrs

While fixing the code, I found some typos in near-by comments that are also unclear, so I rewrote the comment about _vtables_serialized_base

// InstanceKlass* k = a class loaded from the preimage;
// ConstantPool* cp = k->constants();
// the following holds true:
// _archived_cpp_vtptrs[ConstantPool_Kind] == ((intptr_t**)cp)[0]
static bool _orig_cpp_vtptrs_inited = false;
static intptr_t* _orig_cpp_vtptrs[_num_cloned_vtable_kinds];
static intptr_t* _archived_cpp_vtptrs[_num_cloned_vtable_kinds];

template <class T>
void CppVtableCloner<T>::init_orig_cpp_vtptr(int kind) {
@@ -245,7 +254,9 @@ void CppVtables::serialize(SerializeClosure* soc) {
CPP_VTABLE_TYPES_DO(INITIALIZE_VTABLE);
}

if (soc->writing() && CDSConfig::is_dumping_final_static_archive()) {
if (soc->writing() && !CDSConfig::is_dumping_preimage_static_archive()) {
// This table is written only when creating the preimage. It will be used
// only when writing the final static archive.
memset(_archived_cpp_vtptrs, 0, sizeof(_archived_cpp_vtptrs));
}