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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Wed Nov 13 15:54:24 UTC 2019


I agree, the new diagnostic option looks good.   Better than 
SharedBaseAddress=0.

Thanks,
Coleen

On 11/13/19 10:37 AM, Jiangli Zhou wrote:
> Look good!
>
> Best,
> Jiangli
>
> On Tue, Nov 12, 2019 at 9:12 PM Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>>
>> 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> 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!
>>
>>
>> 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> 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