RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address

Ioi Lam ioi.lam at oracle.com
Sun Nov 10 23:13:43 UTC 2019



On 11/9/19 8:25 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
> On Fri, Nov 8, 2019 at 1:35 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>> 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?
> Thanks for creating the RFE.
>
> JDK-8213713 closes the 1G gap between the shared space and class space
> and everything else is unaffected. The compressed class base and shift
> were the same for before and after applying JDK-8213713. The effect
> was statistically observed for the benchmark since the difference was
> very small and could be within noise level for single run comparison.
> A small difference could still be important for some use cases so it
> needs to be taken into consideration when designing and implementing
> new changes.

Hi Jiangli,

Thanks for taking the time for doing the performance measurements.

I also ran benchmarks in all 3 modes (no CDS, CDS without relocation, 
CDS with relocation), and did not see any significant performance with
Octane-DeltaBlue, Octane-NavierStokes, SPECjbb2005-Tuned, 
JFR-SPECjbb2005-Tuned, SPECjvm2008-Serial-G1 and Tools-Javac-Hello.


>
> A new command-line for archived metadata relocation may still be
> valuable. It would also be helpful for debugging and diagnosis.
>

How about a diagnostic flag ArchiveRelocationMode:

0: (default) first map at preferred address, and if unsuccessful, map to 
alternative address;
1: always map to alternative address;
2: always map at preferred address, and if unsuccessful, do not map the 
archive;

1 is for testing relocation, as well as for easy performance measurement 
(replaces the use of -XX:SharedBaseAddress=0 in my current patch.).
2 is for avoiding potential regression that may be introduced by 
relocation (revert to JDK 13 behavior).

What do you think? If you like this I'll open a CSR.

Thanks
- Ioi



>>> 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.
> I experimented with mapping the shared space in low 32G and placed
> right above the Java heap. The class space was also allocated in the
> low 32G space and after the mapped shared space in the experiment. The
> compress class encoding was using 0 base and 3 shift, which was the
> same as the encoding when CDS was disabled. I didn't observe runtime
> performance difference when comparing that specific configuration with
> the normal CDS mapping scheme (the shared space start at 32G and the
> encoding is non-zero base and 3 shift).
>
> Thanks,
> Jiangli
>> 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