RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Jiangli Zhou
jianglizhou at google.com
Mon Oct 28 04:13:50 UTC 2019
Hi Ioi,
Sorry for the delay. Here are my remaining comments.
- src/hotspot/share/memory/dynamicArchive.cpp
128 static intx _method_comparator_name_delta;
The name of the above variable is confusing. It's the value of
_buffer_to_target_delta. It's better to _buffer_to_target_delta
directly. Also, we can do a quick pointer comparison of 'a_name' and
'b_name' first before adjusting the pointers.
---
934 void DynamicArchiveBuilder::relocate_buffer_to_target() {
...
944
945 ArchivePtrMarker::compact(relocatable_base, relocatable_end);
...
974 SharedDataRelocator patcher((address*)patch_base,
(address*)patch_end, valid_old_base, valid_old_end,
975 valid_new_base, valid_new_end, addr_delta);
976 ArchivePtrMarker::ptrmap()->iterate(&patcher);
Could we reduce the number of data re-iterations to help archive
dumping performance. The ArchivePtrMarker::compact operation can be
combined with the patching iteration. ArchivePtrMarker::compact API
can be removed.
---
967 address valid_new_base =
(address)Arguments::default_SharedBaseAddress();
968 address valid_new_end = valid_new_base + base_plus_top_size;
The debugging only code can be included under #ifdef ASSERT.
---
993 dynamic_info->write_bitmap_region(ArchivePtrMarker::ptrmap());
We could combine the archived heap data bitmap into the new region as
well? It can be handled as a separate RFE.
- src/hotspot/share/memory/filemap.cpp
1038 if (is_static()) {
1039 if (errno == ENOENT) {
1040 // Not locating the shared archive is ok.
1041 fail_continue("Specified shared archive not found (%s).",
_full_path);
1042 } else {
1043 fail_continue("Failed to open shared archive file (%s).",
1044 os::strerror(errno));
1045 }
1046 } else {
1047 log_warning(cds, dynamic)("specified dynamic archive
doesn't exist: %s", _full_path);
1048 }
If the top layer is explicitly specified by the user, a warning does
not seem to be a proper behavior if the VM fails to open the archive
file.
If might be better to handle the relocation unrelated code in separate
changeset and track with a separate RFE.
---
1148 void FileMapInfo::write_region(int region, char* base, size_t size,
1149 bool read_only, bool allow_exec) {
...
1154
1155 if (region == MetaspaceShared::bm) {
1156 target_base = NULL;
1157 } else if (DynamicDumpSharedSpaces) {
It's not too clear to me how the bitmap (bm) region is handled for the
base layer and top layer. Could you please explain?
---
1362 DEBUG_ONLY(header()->set_mapped_base_address((char*)(uintptr_t)0xdeadbeef);)
Could you please explain the above?
---
1359 FileMapRegion* last_region = NULL;
1371 if (last_region != NULL) {
1372 // Ensure that the OS won't be able to allocate new memory
spaces between any mapped
1373 // regions, or else it would mess up the simple comparision
in MetaspaceObj::is_shared().
1374 assert(si->mapped_base() == last_region->mapped_end(),
"must have no gaps");
1379 last_region = si;
Can you please place 'last_region' related code under #ifdef ASSERT?
---
1478 char* FileMapInfo::map_relocation_bitmap(size_t& bitmap_size) {
1479 FileMapRegion* si = space_at(MetaspaceShared::bm);
1480 bitmap_size = si->used_aligned();
1481 bool read_only = true, allow_exec = false;
1482 char* requested_addr = NULL; // allow OS to pick any location
1483 char* bitmap_base = os::map_memory(_fd, _full_path, si->file_offset(),
1484 requested_addr, bitmap_size,
read_only, allow_exec);
We need to handle mapping failure here.
---
1513 // debug only -- the current value of the pointers to be
patched must be within this
1514 // range (i.e., must be between the requesed base address,
and the of the current archive).
1515 // Note: top archive may point to objects in the base
archive, but not the other way around.
1516 address valid_old_base = (address)header()->requested_base_address();
1517 address valid_old_end = valid_old_base + mapping_end_offset();
Please place all FileMapInfo::relocate_pointers debugging only code
under #ifdef ASSERT.
- src/hotspot/share/memory/heapShared.cpp
441 void HeapShared::initialize_from_archived_subgraph(Klass* k) {
442 if (!open_archive_heap_region_mapped() || !MetaspaceObj::is_shared(k)) {
443 return; // nothing to do
444 }
When do we call HeapShared::initialize_from_archived_subgraph for a
klass that's not shared?
616 DEBUG_ONLY({
617 Klass* klass = orig_obj->klass();
618 assert(klass != SystemDictionary::Module_klass() &&
619 klass != SystemDictionary::ResolvedMethodName_klass() &&
620 klass != SystemDictionary::MemberName_klass() &&
621 klass != SystemDictionary::Context_klass() &&
622 klass != SystemDictionary::ClassLoader_klass(), "we
can only relocate metaspace object pointers inside java_lang_Class
instances");
623 });
Let's leave the above for a separate RFE. I think assert is not
sufficient for the check. Also, why ResolvedMethodName, Module and
MemberName cannot be part of the graph?
- src/hotspot/share/memory/metaspace.cpp
1036 metaspace_rs = ReservedSpace(compressed_class_space_size(),
1037 _reserve_alignment,
1038 large_pages,
1039 requested_addr);
Please fix indentation.
- src/hotspot/share/memory/metaspaceClosure.hpp
78 enum SpecialRef {
79 _method_entry_ref
80 };
Are there other pointers that are not references to MetaspaceObj? If
_method_entry_ref is the only type, it's probably not worth defining
SpecialRef?
- src/hotspot/share/memory/metaspaceShared.hpp
42 enum MapArchiveResult {
43 MAP_ARCHIVE_SUCCESS,
44 MAP_ARCHIVE_MMAP_FAILURE,
45 MAP_ARCHIVE_OTHER_FAILURE
46 };
If we want to define different failure types, it's probably worth
using separate types for relocation failure and validation failure.
---
193 static intx _mapping_delta; // FIXME rename
How about _relocation_delta?
- src/hotspot/share/oops/instanceKlass
1573 bool InstanceKlass::_disable_method_binary_search = false;
The use of _disable_method_binary_search is not necessary. You can use
DynamicDumpSharedSpaces for the purpose. That would make things
cleaner.
- test/hotspot/jtreg/runtime/cds/SpaceUtilizationCheck.java
76 if (name.equals("s0") || name.equals("s1")) {
77 // String regions are listed at the end and
they may not be fully occupied.
78 break;
79 } else if (name.equals("bm")) {
80 // Bitmap space does not have a requested address.
81 break;
It's not part of your change, but could you please fix line 76 - 78
since it is trivial. It seems the lines can be removed.
- /src/hotspot/share/memory/archiveUtils.hpp
The file name does not match with the macro '#ifndef
SHARE_MEMORY_SHAREDDATARELOCATOR_HPP'. Could you please rename
archiveUtils.* ? archiveRelocator.hpp and archiveRelocator.cpp are
more descriptive.
- src/hotspot/share/memory/archiveUtils.cpp
36 void ArchivePtrMarker::initialize(CHeapBitMap* ptrmap, address*
ptr_base, address* ptr_end) {
37 assert(_ptrmap == NULL, "initialize only once");
38 _ptr_base = ptr_base;
39 _ptr_end = ptr_end;
40 _compacted = false;
41 _ptrmap = ptrmap;
42 _ptrmap->initialize(12 * M / sizeof(intptr_t)); // default
archive is about 12MB.
43 }
Could we do a better estimate here? We could guesstimate the size
based on the current used class space and metaspace size. It's okay if
a larger bitmap used, since it can be reduced after all marking are
done.
Thanks,
Jiangli
On Wed, Oct 16, 2019 at 4:58 PM Jiangli Zhou <jianglizhou at google.com> wrote:
>
> Hi Ioi,
>
> This is another great step for CDS usability improvement. Thank you!
>
> I have a high level question (or request): could we consider
> separating the relocation work for 'direct' class metadata from other
> types of metadata (such as the shared system dictionary, symbol table,
> etc)? Initially we only relocate the tables and other archived global
> data. When each archived class is being loaded, we can relocate all
> the pointers within the current class. We could find the segment (for
> the current class) in the bitmap and update the pointers within the
> segment. That way we can reduce initial startup costs and also avoid
> relocating class data that's not used at runtime. In some real world
> large systems, an archive may contain extremely large number of
> classes.
>
> Following are partial review comments so we can move things forward.
> Still going through the rest of the changes.
>
> - src/hotspot/share/classfile/javaClasses.cpp
>
> 1218 void java_lang_Class::update_archived_mirror_native_pointers(oop
> archived_mirror) {
> 1219 Klass* k = ((Klass*)archived_mirror->metadata_field(_klass_offset));
> 1220 if (k != NULL) { // k is NULL for the primitive classes such as
> java.lang.Byte::TYPE <<<<<<<<<<<
> 1221 archived_mirror->metadata_field_put(_klass_offset,
> (Klass*)(address(k) + MetaspaceShared::mapping_delta()));
> 1222 }
> 1223 ...
>
> Primitive type mirrors are handled separately. Could you please verify
> if this call path happens for primitive type mirror?
>
> To answer my question above, looks like you added the following, which
> is to be used for primitive type mirrors. That seems to be the reason
> why update_archived_mirror_native_pointers is trying to also cover
> primitive type. It better to have a separate API for primitive type
> mirror, which is cleaner. And, we also can replace the above check at
> line 1220 to be an assert for regular mirrors.
>
> +void ReadClosure::do_mirror_oop(oop *p) {
> + do_oop(p);
> + oop mirror = *p;
> + if (mirror != NULL) {
> + java_lang_Class::update_archived_mirror_native_pointers(mirror);
> + }
> +}
> +
>
> How about renaming update_archived_mirror_native_pointers to
> update_archived_mirror_klass_pointers.
>
> It would be good to pass the current klass as an argument. We can
> verify the relocated pointer matches with the current klass pointer.
>
> We should also check if relocation is necessary before spending cycles
> to obtain the klass pointer from the mirror.
>
> 1252 update_archived_mirror_native_pointers(m);
> 1253
> 1254 // mirror is archived, restore
> 1255 assert(HeapShared::is_archived_object(m), "must be archived
> mirror object");
> 1256 Handle mirror(THREAD, m);
>
> Could we move the line at 1252 after the assert at line 1255?
>
> - src/hotspot/share/include/cds.h
>
> 47 int _mapped_from_file; // Is this region mapped from a file?
> 48 // If false, this region was
> initialized using os::read().
>
> Is the new field truly needed? It seems we could use _mapped_base to
> determine if a region is mapped or not?
>
> - src/hotspot/share/memory/dynamicArchive.cpp
>
> Could you please remove the debugging print code in
> dynamic_dump_method_comparator? Or convert those to logging output if
> they are helpful.
>
> Will send out the rest of the review comments later.
>
> Best,
>
> Jiangli
>
>
>
>
> On Thu, Oct 10, 2019 at 6:00 PM Ioi Lam <ioi.lam at oracle.com> wrote:
> >
> > Bug:
> > https://bugs.openjdk.java.net/browse/JDK-8231610
> >
> > Webrev:
> > http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v01/
> >
> > Design:
> > http://cr.openjdk.java.net/~iklam/jdk14/design/8231610-relocate-cds-archive.txt
> >
> >
> > Overview:
> >
> > The CDS archive is mmaped to a fixed address range (starting at
> > SharedBaseAddress, usually 0x800000000). Previously, if this
> > requested address range is not available (usually due to Address
> > Space Layout Randomization (ASLR) [2]), the JVM will give up and
> > will load classes dynamically using class files.
> >
> > [a] This causes slow down in JVM start-up.
> > [b] Handling of mapping failures causes unnecessary complication in
> > the CDS tests.
> >
> > Here are some preliminary benchmarking results (using default CDS archive,
> > running helloworld):
> >
> > (a) 47.1ms (CDS enabled, mapped at requested addr)
> > (b) 53.8ms (CDS enabled, mapped at alternate addr)
> > (c) 86.2ms (CDS disabled)
> >
> > The small degradation in (b) is caused by the relocation of
> > absolute pointers embedded in the CDS archive. However, it is
> > still a big improvement over case (c)
> >
> > Please see the design doc (link above) for details.
> >
> > Thanks
> > - Ioi
> >
More information about the hotspot-runtime-dev
mailing list