RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Ioi Lam
ioi.lam at oracle.com
Tue Oct 29 04:04:56 UTC 2019
Hi Jiangli,
Thanks for the review. I've updated the patch according to your comments:
http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v04/
http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v04.delta/
(the delta is on top of 8231610-relocate-cds-archive.v03.delta in my
reply to Calvin's comments).
On 10/27/19 9:13 PM, Jiangli Zhou wrote:
> 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.
_buffer_to_target_delta is a non-static field, but
dynamic_dump_method_comparator() must be a static function so it can't
use the non-static field easily.
> Also, we can do a quick pointer comparison of 'a_name' and
> 'b_name' first before adjusting the pointers.
I added this:
if (a_name == b_name) {
return 0;
}
> ---
>
> 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.
That's a good idea. I implemented it using a template parameter so that
we can have max performance when relocating the archive at run time.
I added comments to explain why the relocation is done here. The
relocation is pretty rare (only when the base archive was not mapped at
the default location).
> ---
>
> 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.
These values are actually also used in debug logging so they can't be
ifdef'ed out.
Also, the c++ compiler is pretty good with eliding code that's no
actually used. If I comment out all the logging code in
DynamicArchiveBuilder::relocate_buffer_to_target() and
SharedDataRelocator, gcc elides all the unused fields and their
assignments. So no code is generated for this, etc.
address valid_new_base =
(address)Arguments::default_SharedBaseAddress();
Since #ifdef ASSERT makes the code harder to read, I think we should use
it only when really necessary.
> ---
>
> 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.
I've filed https://bugs.openjdk.java.net/browse/JDK-8233093
>
> - 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.
This code was moved from
http://hg.openjdk.java.net/jdk/jdk/file/d3382812b788/src/hotspot/share/memory/dynamicArchive.cpp#l1070
so I am not changing the behavior. If you want, we can file an REF to
change the behavior.
> ---
>
> 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?
The bm region for both layers are mapped at an address picked by the OS:
char* FileMapInfo::map_relocation_bitmap(size_t& bitmap_size) {
FileMapRegion* si = space_at(MetaspaceShared::bm);
bitmap_size = si->used_aligned();
bool read_only = true, allow_exec = false;
char* requested_addr = NULL; // allow OS to pick any location
char* bitmap_base = os::map_memory(_fd, _full_path, si->file_offset(),
requested_addr, bitmap_size,
read_only, allow_exec);
>
> ---
>
> 1362 DEBUG_ONLY(header()->set_mapped_base_address((char*)(uintptr_t)0xdeadbeef);)
>
> Could you please explain the above?
I added the comments
// Make sure we don't attempt to use header()->mapped_base_address()
unless
// it's been successfully mapped.
DEBUG_ONLY(header()->set_mapped_base_address((char*)(uintptr_t)0xdeadbeef);)
>
> ---
>
> 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?
I think that will make the code more cluttered. The compiler will
optimize out that away.
> ---
>
> 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.
It's handled here:
bool FileMapInfo::relocate_pointers(intx addr_delta) {
log_debug(cds, reloc)("runtime archive relocation start");
size_t bitmap_size;
char* bitmap_base = map_relocation_bitmap(bitmap_size);
if (bitmap_base != NULL) {
...
} else {
log_error(cds)("failed to map relocation bitmap");
return false;
}
> ---
>
> 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.
Ditto about 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?
I've removed the !MetaspaceObj::is_shared(k). I probably added that for
debugging purposes only.
>
> 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?
>
>
I added the following comment:
DEBUG_ONLY({
// The following are classes in share/classfile/javaClasses.cpp
that have injected native pointers
// to metaspace objects. To support these classes, we need to add
relocation code similar to
// java_lang_Class::update_archived_mirror_native_pointers.
Klass* klass = orig_obj->klass();
assert(klass != SystemDictionary::Module_klass() &&
klass != SystemDictionary::ResolvedMethodName_klass() &&
>
> - 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.
Fixed.
>
> - 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?
There may be more types in the future, so I want to have a stable API
that can be easily expanded without touching all the code that uses it.
>
> - 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.
For now, I just need to distinguish between MMAP_FAILURE (where I should
attempt to remap at an alternative address) and OTHER_FAILURE (where the
CDS archive loading will fail -- due to validation error, insufficient
memory, etc -- without attempting to remap.)
>
> ---
>
> 193 static intx _mapping_delta; // FIXME rename
>
> How about _relocation_delta?
Changed as suggested.
>
> - 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.
If we always disable the binary search when DynamicDumpSharedSpaces is
true, it will slow down normal execution of the Java program when
-XX:ArchiveClassesAtExit has been specified, but the program hasn't exited.
>
> - 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.
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.
I named the file archiveUtils.hpp so we can move other misc stuff used
by dumping into this file (e.g., DumpRegion, WriteClosure from
metaspaceShared.hpp), since theses are not used by the majority of the
files that use metaspaceShared.hpp.
I fixed the ifdef.
>
> - 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.
The bitmap is automatically expanded when necessary in
ArchivePtrMarker::mark_pointer(). It's only about 1/32 or 1/64 of the
total archive size, so even if we do expand, the cost will be trivial.
Thanks
- Ioi
> 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