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

Ioi Lam ioi.lam at oracle.com
Wed Nov 13 05:10:21 UTC 2019



On 11/10/19 5:14 PM, Jiangli Zhou wrote:
>
>
> On Sun, Nov 10, 2019, 3:13 PM Ioi Lam <ioi.lam at oracle.com 
> <mailto: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
>     <mailto: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 <mailto: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!

Hi Jiangli,

It turns out that CSR is not needed for adding a diagnostic flag.

I implemented the flag as described above. See:

http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v07-delta/


Thanks
- Ioi

>
> 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
>     <mailto: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
>     <mailto: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>
>     >>>>>>>> <mailto: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>
>     >>>>>>>>       <mailto: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> <mailto: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