RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Ioi Lam
ioi.lam at oracle.com
Fri Nov 8 21:35:36 UTC 2019
Hi Jiangli,
Thanks for your comments. Please see my replies in-line:
On 11/7/19 6:34 PM, Jiangli Zhou wrote:
> On Thu, Nov 7, 2019 at 6:11 PM Jiangli Zhou <jianglizhou at google.com> wrote:
>> I looked both 05.full and 06.delta webrevs. They look good.
>>
>> I still feel a bit uneasy about the potential runtime impact when data
>> does get relocated. Long running apps/services may be shy away from
>> enabling archive at runtime, if there is a detectable overhead even
>> though it may only occur rarely. As relocation is enabled by default
>> and users cannot turn it off, disabling with -Xshare:off entirely
>> would become the only choice. Could you please create a new RFE
>> (possibly with higher priority) to investigate the potential effect,
>> or provide an option for users to opt-in relocation with the
>> command-line switch?
I created https://bugs.openjdk.java.net/browse/JDK-8233862
Investigate performance benefit of relocating CDS archive to under 32G
As I noted in the bug report, I ran benchmarks with CDS relocation
on/off, and there's no sign of regression when the CDS archive is
relocated. Please see the bug report for how to configure the VM to do
the comparison.
As you said before: "When enabling CDS we [google] noticed a small
runtime overhead in JDK 11 recently with a benchmark. After I backported
JDK-8213713 to 11, it seemed to reduce the runtime overhead that the
benchmark was experiencing":
Can you confirm whether this is stock JDK 11 or a special google build?
Which test case did you use? Is it possible for you to run the tests
again (using the exact before/after bits that you had when backporting
JDK-8213713)? Can you check if narrow_klass_base and narrow_klass_shift
are the same in your before/after builds?
> Forgot to say that when Java heap can fit into low 32G space, it takes
> the class space size into account and leaves need space right above
> (also in low 32G space) when reserving heap, for !UseSharedSpace. In
> that case, it's more likely the class data and heap data can be
> colocated successfully.
The reason is not for "colocation". It's so that narrow_klass_base can
be zero, and the klass pointer can be uncompressed with a shift (without
also doing an addition).
But with CDS enabled, we always hard code to use non-zero
narrow_klass_base and 3 bit shift (for AOT). So by just relocating the
CDS archive to under 32GB, without modifying how CDS handles
narrow_klass_base/shift, I don't think we can expect any benefit.
For modern architectures, I am not aware of any inherent speed benefit
simply by putting data (in our case much larger than a page) "close to
each other" in the virtual address space. If you have any reference of
that, please let me know.
Thanks
- Ioi
>
> Thanks,
> Jiangli
>
>> Regards,
>> Jiangli
>>
>> On Thu, Nov 7, 2019 at 4:22 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>> Hi Coleen,
>>>
>>> Thanks for the review. Here's an webrev that has incorporated your
>>> suggestions:
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v06-delta/
>>>
>>> Please see comments in-line
>>>
>>> On 11/7/19 2:46 PM, coleen.phillimore at oracle.com wrote:
>>>> Hi, I've done a more high level code review of this and it looks good!
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05/src/hotspot/share/memory/archiveUtils.hpp.html
>>>>
>>>>
>>>> I think these classes require comments on what they do and why. The
>>>> comments you sent me offline look good.
>>> I added more comments for ArchivePtrMarker::_compacted per your offline
>>> request.
>>>
>>>> Also .hpp files shouldn't include .inline.hpp files, like
>>>> bitMap.inline.hpp. Hopefully it's just a case of moving do_bit() into
>>>> the cpp file.
>>> I moved the do_bit() function into archiveUtils.inline.hpp, since is
>>> used by 3 .cpp files, and performance is important.
>>>
>>>> I wonder if the exception list of classes to exclude should be a
>>>> function in javaClasses.hpp/cpp where the explanation would make more
>>>> sense? ie bool
>>>> JavaClasses::has_injected_native_pointers(InstanceKlass* k);
>>> I moved the checking code to javaClasses.cpp. Since we do (partially)
>>> support java.lang.Class, which has injected native pointers, I named the
>>> function as JavaClasses::is_supported_for_archiving instead. I also
>>> massaged the comments a little for clarification.
>>>
>>>> Is there already an RFE to move the DumpSharedSpaces output from
>>>> tty->print() to log_info() ?
>>> I created https://bugs.openjdk.java.net/browse/JDK-8233826 (Change CDS
>>> dumping tty->print_cr() to unified logging).
>>>
>>> Thanks
>>> - Ioi
>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>> On 11/6/19 4:17 PM, Ioi Lam wrote:
>>>>> Hi Jiangli,
>>>>>
>>>>> I've uploaded the webrev after integrating your comments:
>>>>>
>>>>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05/
>>>>>
>>>>> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05-delta/
>>>>>
>>>>>
>>>>> Please see more replies below:
>>>>>
>>>>>
>>>>> On 11/4/19 5:52 PM, Jiangli Zhou wrote:
>>>>>> On Sun, Nov 3, 2019 at 10:27 PM Ioi Lam <ioi.lam at oracle.com
>>>>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>>>>
>>>>>> 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?
>>>>>>
>>>>>>
>>>>>> I don't yet have sufficient data that determins if mapping at low
>>>>>> 32G produces better runtime performance. I experimented with that,
>>>>>> but didn't see noticeable difference when comparing to mapping at
>>>>>> the current default address. It doesn't hurt, I think. So it may be
>>>>>> a better choice than relocating to a random address in high 32G
>>>>>> space (when Java heap is in low 32G address space).
>>>>> Maybe we should reconsider this when we have more concrete data for
>>>>> the benefits of moving the compressed class space to under 32G.
>>>>>
>>>>> Please note that in metaspace.cpp, when CDS is disabled and the VM
>>>>> fails to allocate the class space at the requested address
>>>>> (0x7c000000 for 16GB heap), it also just allocates from a random
>>>>> address (without trying to to search under 32GB):
>>>>>
>>>>> http://hg.openjdk.java.net/jdk/jdk/annotate/e767fa6a1d45/src/hotspot/share/memory/metaspace.cpp#l1128
>>>>>
>>>>>
>>>>> This code has been there since 2013 and we have not seen any issues.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> Could you elaborate more about the performance issue, especially
>>>>>> about
>>>>>> cache locality? I looked at JDK-8213713 but it didn't mention about
>>>>>> performance.
>>>>>>
>>>>>>
>>>>>> When enabling CDS we noticed a small runtime overhead in JDK 11
>>>>>> recently with a benchmark. After I backported JDK-8213713 to 11, it
>>>>>> seemed to reduce the runtime overhead that the benchmark was
>>>>>> experiencing.
>>>>>>
>>>>>>
>>>>>> 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);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> Right. If we relocate to low 32G space, it needs to make sure that
>>>>>> the range containing the mapped class data and class space must be
>>>>>> encodable.
>>>>>>
>>>>>>
>>>>>> > 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
>>>>>> <mailto: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.
>>>>>>
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>
>>>>>> >
>>>>>> >>> ---
>>>>>> >>>
>>>>>> >>> 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;
>>>>>> }
>>>>>> }
>>>>>>
>>>>>>
>>>>>> No objection If you prefer the extra 'else' block.
>>>>>>
>>>>>>
>>>>>> >
>>>>>> >>> ---
>>>>>> >>>
>>>>>> >>> 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.
>>>>>>
>>>>>>
>>>>>> Sounds good. Can you please also add a comment with explanation.
>>>>>>
>>>>>> For ClassLoader and Module, it worth considering caching the
>>>>>> additional native data some time in the future. Lois had suggested
>>>>>> the Module part a while ago.
>>>>> I think we can do that if/when we archive Modules directly into the
>>>>> shared heap.
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Maybe we should backport the check to older versions as well?
>>>>>>
>>>>>>
>>>>>> We should discuss with Andrew Haley for backports to JDK 11 update
>>>>>> releases. Since the current OpenJDK 11 only applies Java heap
>>>>>> archiving to a restricted set of JDK library code, I think it is
>>>>>> safe without the new check.
>>>>>>
>>>>>> For non-LTS releases, it might not be worthwhile as they may not be
>>>>>> widely used?
>>>>> I agree. FYI, we (Oracle) have no plan for backporting more types of
>>>>> heap object archiving, so the decision would be up to whoever that
>>>>> decides to do so.
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>>
>>>>>>
>>>>>> >
>>>>>> >>> - 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 <mailto: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
>>>>>> <mailto: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