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 17:19:16 UTC 2019


Thanks Jiangli & Coleen for your reviews!

Looks like we are close to the end. I will do more testing and push soon.

Thanks
- Ioi

On 11/13/19 7:54 AM, coleen.phillimore at oracle.com wrote:
>
> 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