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

Jiangli Zhou jianglizhou at google.com
Mon Nov 11 01:14:40 UTC 2019


On Sun, Nov 10, 2019, 3:13 PM Ioi Lam <ioi.lam at oracle.com> wrote:

>
>
> 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.
>


That sounds good to me!

Regards,
Jiangli


> 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


More information about the hotspot-runtime-dev mailing list