RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Ioi Lam
ioi.lam at oracle.com
Mon Nov 4 06:27:08 UTC 2019
Hi Jiangli,
Thank you so much for spending time reviewing this RFE!
On 11/3/19 6:34 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
> Sorry for the delay again. Will try to put this on the top of my list
> next week and reduce the turn-around time. The updates look good in
> general.
>
> We might want to have a better strategy when choosing metadata
> relocation address (when relocation is needed). Some
> applications/benchmarks may be more sensitive to cache locality and
> memory/data layout. There was a bug,
> https://bugs.openjdk.java.net/browse/JDK-8213713 that caused 1G gap
> between Java heap data and metadata before JDK 12. The gap seemed to
> cause a small but noticeable runtime effect in one case that I came
> across.
I guess you're saying we should try to relocate the archive into
somewhere under 32GB?
Could you elaborate more about the performance issue, especially about
cache locality? I looked at JDK-8213713 but it didn't mention about
performance.
Also, by default, we have non-zero narrow_klass_base and
narrow_klass_shift = 3, and archive relocation doesn't change that:
$ java -Xlog:cds=debug -version
... narrow_klass_base = 0x0000000800000000, narrow_klass_shift = 3
$ java -Xlog:cds=debug -XX:SharedBaseAddress=0 -version
... narrow_klass_base = 0x00007f1e8b499000, narrow_klass_shift = 3
We always use narrow_klass_shift due to this:
// CDS uses LogKlassAlignmentInBytes for narrow_klass_shift. See
// MetaspaceShared::initialize_dumptime_shared_and_meta_spaces() for
// how dump time narrow_klass_shift is set. Although, CDS can work
// with zero-shift mode also, to be consistent with AOT it uses
// LogKlassAlignmentInBytes for klass shift so archived java heap objects
// can be used at same time as AOT code.
if (!UseSharedSpaces
&& (uint64_t)(higher_address - lower_base) <=
UnscaledClassSpaceMax) {
CompressedKlassPointers::set_shift(0);
} else {
CompressedKlassPointers::set_shift(LogKlassAlignmentInBytes);
}
> Here are some additional comments (minor).
>
> Could you please fix the long lines in the following?
>
> 1237 void java_lang_Class::update_archived_primitive_mirror_native_pointers(oop
> archived_mirror) {
> 1238 if (MetaspaceShared::relocation_delta() != 0) {
> 1239 assert(archived_mirror->metadata_field(_klass_offset) ==
> NULL, "must be for primitive class");
> 1240
> 1241 Klass* ak =
> ((Klass*)archived_mirror->metadata_field(_array_klass_offset));
> 1242 if (ak != NULL) {
> 1243 archived_mirror->metadata_field_put(_array_klass_offset,
> (Klass*)(address(ak) + MetaspaceShared::relocation_delta()));
> 1244 }
> 1245 }
> 1246 }
>
> src/hotspot/share/memory/dynamicArchive.cpp
>
> 889 Thread* THREAD = Thread::current();
> 890 Method::sort_methods(ik->methods(), /*set_idnums=*/true,
> dynamic_dump_method_comparator);
> 891 if (ik->default_methods() != NULL) {
> 892 Method::sort_methods(ik->default_methods(),
> /*set_idnums=*/false, dynamic_dump_method_comparator);
> 893 }
>
OK will do.
> Please see inlined comments below.
>
> On Mon, Oct 28, 2019 at 9:05 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>> 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.
>
> It sounds like an issue. _buffer_to_target_delta was made as a
> non-static mostly because we might support more than one dynamic
> archives in the future. However, today's usages bake in an assumption
> that _buffer_to_target_delta is a singleton value. It is cleaner to
> either make _buffer_to_target_delta as a static variable for now, or
> adding an access API in DynamicArchiveBuilder to allow other code to
> properly and correctly use the value.
OK, I'll move it to a static variable.
>
>>> 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.
> It seems cleaner to get rid of these debugging only variables, by
> using 'relocatable_base' and
> '(address)Arguments::default_SharedBaseAddress()' in the logging code.
SharedDataRelocator is used under 3 different situations. These six
variables (patch_base, patch_end, valid_old_base, valid_old_end,
valid_new_base, valid_new_end) describes what is being patched, and what
the expectations are, for each situation. The code will be hard to
understand without them.
Please note there's also logging code in the SharedDataRelocator
constructor that prints out these values.
I think I'll just remove the 'debug only' comment to avoid confusion.
>
>>> ---
>>>
>>> 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.
> Ok. A new RFE sounds like the right thing to re-evaluable the usage
> issue here. Thanks.
I created https://bugs.openjdk.java.net/browse/JDK-8233446
>>> ---
>>>
>>> 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);
>>
> Ok, after staring at the code for a few seconds I saw that's intended.
> If the current region is 'bm', then the 'target_base' is NULL
> regardless if it's static or dynamic archive. Otherwise, the
> 'target_base' is handled differently for the static and dynamic case.
> The following would be cleaner and has better reliability.
>
> char* target_base = NULL;
>
> // The target_base is NULL for 'bm' region.
> if (!region == MetaspaceShared::bm) {
> if (DynamicDumpSharedSpaces) {
> assert(!HeapShared::is_heap_region(region), "dynamic archive
> doesn't support heap regions");
> target_base = DynamicArchive::buffer_to_target(base);
> } else {
> target_base = base;
> }
> }
How about this?
char* target_base;
if (region == MetaspaceShared::bm) {
target_base = NULL; // always NULL for bm region.
} else {
if (DynamicDumpSharedSpaces) {
assert(!HeapShared::is_heap_region(region), "dynamic archive
doesn't support heap regions");
target_base = DynamicArchive::buffer_to_target(base);
} else {
target_base = base;
}
}
>
>>> ---
>>>
>>> 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.
> It's cleaner to define debugging only variable for debugging only
> builds. You can wrapper it and related usage with DEBUG_ONLY.
OK, will do.
>
>>> ---
>>>
>>> 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;
>> }
>>
> 'bitmap_base' is used immediately after map_memory(). So the check
> needs to be done immediately after map_memory(), but not in the caller
> of map_relocation_bitmap().
>
> 1490 char* bitmap_base = os::map_memory(_fd, _full_path, si->file_offset(),
> 1491 requested_addr, bitmap_size,
> read_only, allow_exec);
> 1492
> 1493 if (VerifySharedSpaces && bitmap_base != NULL &&
> !region_crc_check(bitmap_base, bitmap_size, si->crc())) {
OK, I'll fix that.
>
>
>>> ---
>>>
>>> 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() &&
>>
> It's too restrictive to exclude those objects from the archived object
> graph because metadata relocation, since metadata relocation is rare.
> The trade-off doesn't seem to buy us much.
>
> Do you plan to add the needed relocation code?
I looked more into this. Actually we cannot handle these 5 classes at
all, even without archive relocation:
[1] #define MODULE_INJECTED_FIELDS(macro) \
macro(java_lang_Module, module_entry, intptr_signature, false)
-> module_entry is malloc'ed
[2] #define RESOLVEDMETHOD_INJECTED_FIELDS(macro) \
macro(java_lang_invoke_ResolvedMethodName, vmholder,
object_signature, false) \
macro(java_lang_invoke_ResolvedMethodName, vmtarget,
intptr_signature, false)
-> these fields are related to method handles and lambda forms, etc.
They can't be easily be archived without implementing lambda form
archiving. (I did a prototype; it's very complex and fragile).
[3] #define CALLSITECONTEXT_INJECTED_FIELDS(macro) \
macro(java_lang_invoke_MethodHandleNatives_CallSiteContext,
vmdependencies, intptr_signature, false) \
macro(java_lang_invoke_MethodHandleNatives_CallSiteContext,
last_cleanup, long_signature, false)
-> vmdependencies is malloc'ed.
[4] #define
MEMBERNAME_INJECTED_FIELDS(macro) \
macro(java_lang_invoke_MemberName, vmindex, intptr_signature, false)
-> this one is probably OK. Despite being declared as
'intptr_signature', it seems to be used just as an integer. However,
MemberNames are typically used with [2] and [3]. So let's just forbid it
to be safe.
[2] [3] [4] are not used directly by regular Java code and are unlikely
to be referenced (directly or indirectly) by static fields (except for
the static fields in the classes in java.lang.invoke, which we probably
won't support for heap archiving due to the problem I described for
[2]). Objects of these types are typically referenced via constant pool
entries.
[5] #define CLASSLOADER_INJECTED_FIELDS(macro) \
macro(java_lang_ClassLoader, loader_data, intptr_signature, false)
-> loader_data is malloc'ed.
So, I will change the DEBUG_ONLY into a product-mode check, and quit
dumping if these objects are found in the object subgraph.
Maybe we should backport the check to older versions as well?
>
>>> - 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.
> Could you please add some comments to _disable_method_binary_search
> with the above explanation? Thanks.
OK
>
>>> - 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.
> The initial value is based on the default CDS archive. When dealing
> with a really large archive, it would have to re-grow many times.
> Also, using a hard-coded value is less desirable.
OK, I changed it to the following
// Use this as initial guesstimate. We should need less space in the
// archive, but if we're wrong the bitmap will be expanded automatically.
size_t estimated_archive_size = MetaspaceGC::capacity_until_GC();
// But set it smaller in debug builds so we always test the expansion
code.
// (Default archive is about 12MB).
DEBUG_ONLY(estimated_archive_size = 6 * M);
// We need one bit per pointer in the archive.
_ptrmap->initialize(estimated_archive_size / sizeof(intptr_t));
Thanks!
- Ioi
>
>>>
>>>
>>> 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