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