RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Ioi Lam
ioi.lam at oracle.com
Wed Nov 13 05:10:21 UTC 2019
On 11/10/19 5:14 PM, Jiangli Zhou wrote:
>
>
> On Sun, Nov 10, 2019, 3:13 PM Ioi Lam <ioi.lam at oracle.com
> <mailto:ioi.lam at oracle.com>> wrote:
>
>
>
> On 11/9/19 8:25 PM, Jiangli Zhou wrote:
> > Hi Ioi,
> >
> > On Fri, Nov 8, 2019 at 1:35 PM Ioi Lam <ioi.lam at oracle.com
> <mailto:ioi.lam at oracle.com>> wrote:
> >> Hi Jiangli,
> >>
> >> Thanks for your comments. Please see my replies in-line:
> >>
> >> On 11/7/19 6:34 PM, Jiangli Zhou wrote:
> >>> On Thu, Nov 7, 2019 at 6:11 PM Jiangli Zhou
> <jianglizhou at google.com <mailto:jianglizhou at google.com>> wrote:
> >>>> I looked both 05.full and 06.delta webrevs. They look good.
> >>>>
> >>>> I still feel a bit uneasy about the potential runtime impact
> when data
> >>>> does get relocated. Long running apps/services may be shy
> away from
> >>>> enabling archive at runtime, if there is a detectable
> overhead even
> >>>> though it may only occur rarely. As relocation is enabled by
> default
> >>>> and users cannot turn it off, disabling with -Xshare:off entirely
> >>>> would become the only choice. Could you please create a new RFE
> >>>> (possibly with higher priority) to investigate the potential
> effect,
> >>>> or provide an option for users to opt-in relocation with the
> >>>> command-line switch?
> >> I created https://bugs.openjdk.java.net/browse/JDK-8233862
> >> Investigate performance benefit of relocating CDS archive to
> under 32G
> >>
> >> As I noted in the bug report, I ran benchmarks with CDS relocation
> >> on/off, and there's no sign of regression when the CDS archive is
> >> relocated. Please see the bug report for how to configure the
> VM to do
> >> the comparison.
> >>
> >> As you said before: "When enabling CDS we [google] noticed a small
> >> runtime overhead in JDK 11 recently with a benchmark. After I
> backported
> >> JDK-8213713 to 11, it seemed to reduce the runtime overhead
> that the
> >> benchmark was experiencing":
> >>
> >> Can you confirm whether this is stock JDK 11 or a special
> google build?
> >> Which test case did you use? Is it possible for you to run the
> tests
> >> again (using the exact before/after bits that you had when
> backporting
> >> JDK-8213713)? Can you check if narrow_klass_base and
> narrow_klass_shift
> >> are the same in your before/after builds?
> > Thanks for creating the RFE.
> >
> > JDK-8213713 closes the 1G gap between the shared space and class
> space
> > and everything else is unaffected. The compressed class base and
> shift
> > were the same for before and after applying JDK-8213713. The effect
> > was statistically observed for the benchmark since the
> difference was
> > very small and could be within noise level for single run
> comparison.
> > A small difference could still be important for some use cases so it
> > needs to be taken into consideration when designing and implementing
> > new changes.
>
> Hi Jiangli,
>
> Thanks for taking the time for doing the performance measurements.
>
> I also ran benchmarks in all 3 modes (no CDS, CDS without relocation,
> CDS with relocation), and did not see any significant performance with
> Octane-DeltaBlue, Octane-NavierStokes, SPECjbb2005-Tuned,
> JFR-SPECjbb2005-Tuned, SPECjvm2008-Serial-G1 and Tools-Javac-Hello.
>
>
> >
> > A new command-line for archived metadata relocation may still be
> > valuable. It would also be helpful for debugging and diagnosis.
> >
>
> How about a diagnostic flag ArchiveRelocationMode:
>
> 0: (default) first map at preferred address, and if unsuccessful,
> map to
> alternative address;
> 1: always map to alternative address;
> 2: always map at preferred address, and if unsuccessful, do not
> map the
> archive;
>
> 1 is for testing relocation, as well as for easy performance
> measurement
> (replaces the use of -XX:SharedBaseAddress=0 in my current patch.).
> 2 is for avoiding potential regression that may be introduced by
> relocation (revert to JDK 13 behavior).
>
> What do you think? If you like this I'll open a CSR.
>
>
>
> That sounds good to me!
Hi Jiangli,
It turns out that CSR is not needed for adding a diagnostic flag.
I implemented the flag as described above. See:
http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v07-delta/
Thanks
- Ioi
>
> Regards,
> Jiangli
>
>
> Thanks
> - Ioi
>
>
>
> >>> Forgot to say that when Java heap can fit into low 32G space,
> it takes
> >>> the class space size into account and leaves need space right
> above
> >>> (also in low 32G space) when reserving heap, for
> !UseSharedSpace. In
> >>> that case, it's more likely the class data and heap data can be
> >>> colocated successfully.
> >> The reason is not for "colocation". It's so that
> narrow_klass_base can
> >> be zero, and the klass pointer can be uncompressed with a shift
> (without
> >> also doing an addition).
> >>
> >> But with CDS enabled, we always hard code to use non-zero
> >> narrow_klass_base and 3 bit shift (for AOT). So by just
> relocating the
> >> CDS archive to under 32GB, without modifying how CDS handles
> >> narrow_klass_base/shift, I don't think we can expect any benefit.
> > I experimented with mapping the shared space in low 32G and placed
> > right above the Java heap. The class space was also allocated in the
> > low 32G space and after the mapped shared space in the
> experiment. The
> > compress class encoding was using 0 base and 3 shift, which was the
> > same as the encoding when CDS was disabled. I didn't observe runtime
> > performance difference when comparing that specific
> configuration with
> > the normal CDS mapping scheme (the shared space start at 32G and the
> > encoding is non-zero base and 3 shift).
> >
> > Thanks,
> > Jiangli
> >> For modern architectures, I am not aware of any inherent speed
> benefit
> >> simply by putting data (in our case much larger than a page)
> "close to
> >> each other" in the virtual address space. If you have any
> reference of
> >> that, please let me know.
> >>
> >> Thanks
> >> - Ioi
> >>
> >>> Thanks,
> >>> Jiangli
> >>>
> >>>> Regards,
> >>>> Jiangli
> >>>>
> >>>> On Thu, Nov 7, 2019 at 4:22 PM Ioi Lam <ioi.lam at oracle.com
> <mailto:ioi.lam at oracle.com>> wrote:
> >>>>> Hi Coleen,
> >>>>>
> >>>>> Thanks for the review. Here's an webrev that has
> incorporated your
> >>>>> suggestions:
> >>>>>
> >>>>>
> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v06-delta/
> >>>>>
> >>>>> Please see comments in-line
> >>>>>
> >>>>> On 11/7/19 2:46 PM, coleen.phillimore at oracle.com
> <mailto:coleen.phillimore at oracle.com> wrote:
> >>>>>> Hi, I've done a more high level code review of this and it
> looks good!
> >>>>>>
> >>>>>>
> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05/src/hotspot/share/memory/archiveUtils.hpp.html
> >>>>>>
> >>>>>>
> >>>>>> I think these classes require comments on what they do and
> why. The
> >>>>>> comments you sent me offline look good.
> >>>>> I added more comments for ArchivePtrMarker::_compacted per
> your offline
> >>>>> request.
> >>>>>
> >>>>>> Also .hpp files shouldn't include .inline.hpp files, like
> >>>>>> bitMap.inline.hpp. Hopefully it's just a case of moving
> do_bit() into
> >>>>>> the cpp file.
> >>>>> I moved the do_bit() function into archiveUtils.inline.hpp,
> since is
> >>>>> used by 3 .cpp files, and performance is important.
> >>>>>
> >>>>>> I wonder if the exception list of classes to exclude should
> be a
> >>>>>> function in javaClasses.hpp/cpp where the explanation would
> make more
> >>>>>> sense? ie bool
> >>>>>> JavaClasses::has_injected_native_pointers(InstanceKlass* k);
> >>>>> I moved the checking code to javaClasses.cpp. Since we do
> (partially)
> >>>>> support java.lang.Class, which has injected native pointers,
> I named the
> >>>>> function as JavaClasses::is_supported_for_archiving instead.
> I also
> >>>>> massaged the comments a little for clarification.
> >>>>>
> >>>>>> Is there already an RFE to move the DumpSharedSpaces output
> from
> >>>>>> tty->print() to log_info() ?
> >>>>> I created https://bugs.openjdk.java.net/browse/JDK-8233826
> (Change CDS
> >>>>> dumping tty->print_cr() to unified logging).
> >>>>>
> >>>>> Thanks
> >>>>> - Ioi
> >>>>>
> >>>>>> Thanks,
> >>>>>> Coleen
> >>>>>>
> >>>>>> On 11/6/19 4:17 PM, Ioi Lam wrote:
> >>>>>>> Hi Jiangli,
> >>>>>>>
> >>>>>>> I've uploaded the webrev after integrating your comments:
> >>>>>>>
> >>>>>>>
> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05/
> >>>>>>>
> >>>>>>>
> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v05-delta/
> >>>>>>>
> >>>>>>>
> >>>>>>> Please see more replies below:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 11/4/19 5:52 PM, Jiangli Zhou wrote:
> >>>>>>>> On Sun, Nov 3, 2019 at 10:27 PM Ioi Lam
> <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>
> >>>>>>>> <mailto:ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>>>
> wrote:
> >>>>>>>>
> >>>>>>>> Hi Jiangli,
> >>>>>>>>
> >>>>>>>> Thank you so much for spending time reviewing this RFE!
> >>>>>>>>
> >>>>>>>> On 11/3/19 6:34 PM, Jiangli Zhou wrote:
> >>>>>>>> > Hi Ioi,
> >>>>>>>> >
> >>>>>>>> > Sorry for the delay again. Will try to put this
> on the top of my
> >>>>>>>> list
> >>>>>>>> > next week and reduce the turn-around time. The
> updates look
> >>>>>>>> good in
> >>>>>>>> > general.
> >>>>>>>> >
> >>>>>>>> > We might want to have a better strategy when
> choosing metadata
> >>>>>>>> > relocation address (when relocation is needed). Some
> >>>>>>>> > applications/benchmarks may be more sensitive to
> cache
> >>>>>>>> locality and
> >>>>>>>> > memory/data layout. There was a bug,
> >>>>>>>> > https://bugs.openjdk.java.net/browse/JDK-8213713
> that caused
> >>>>>>>> 1G gap
> >>>>>>>> > between Java heap data and metadata before JDK
> 12. The gap
> >>>>>>>> seemed to
> >>>>>>>> > cause a small but noticeable runtime effect in
> one case that I
> >>>>>>>> came
> >>>>>>>> > across.
> >>>>>>>>
> >>>>>>>> I guess you're saying we should try to relocate the
> archive into
> >>>>>>>> somewhere under 32GB?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> I don't yet have sufficient data that determins if
> mapping at low
> >>>>>>>> 32G produces better runtime performance. I experimented
> with that,
> >>>>>>>> but didn't see noticeable difference when comparing to
> mapping at
> >>>>>>>> the current default address. It doesn't hurt, I think. So
> it may be
> >>>>>>>> a better choice than relocating to a random address in
> high 32G
> >>>>>>>> space (when Java heap is in low 32G address space).
> >>>>>>> Maybe we should reconsider this when we have more concrete
> data for
> >>>>>>> the benefits of moving the compressed class space to under
> 32G.
> >>>>>>>
> >>>>>>> Please note that in metaspace.cpp, when CDS is disabled
> and the VM
> >>>>>>> fails to allocate the class space at the requested address
> >>>>>>> (0x7c000000 for 16GB heap), it also just allocates from a
> random
> >>>>>>> address (without trying to to search under 32GB):
> >>>>>>>
> >>>>>>>
> http://hg.openjdk.java.net/jdk/jdk/annotate/e767fa6a1d45/src/hotspot/share/memory/metaspace.cpp#l1128
> >>>>>>>
> >>>>>>>
> >>>>>>> This code has been there since 2013 and we have not seen
> any issues.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> Could you elaborate more about the performance
> issue, especially
> >>>>>>>> about
> >>>>>>>> cache locality? I looked at JDK-8213713 but it
> didn't mention about
> >>>>>>>> performance.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> When enabling CDS we noticed a small runtime overhead in
> JDK 11
> >>>>>>>> recently with a benchmark. After I backported JDK-8213713
> to 11, it
> >>>>>>>> seemed to reduce the runtime overhead that the benchmark was
> >>>>>>>> experiencing.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Also, by default, we have non-zero
> narrow_klass_base and
> >>>>>>>> narrow_klass_shift = 3, and archive relocation
> doesn't change that:
> >>>>>>>>
> >>>>>>>> $ java -Xlog:cds=debug -version
> >>>>>>>> ... narrow_klass_base = 0x0000000800000000,
> narrow_klass_shift = 3
> >>>>>>>> $ java -Xlog:cds=debug -XX:SharedBaseAddress=0 -version
> >>>>>>>> ... narrow_klass_base = 0x00007f1e8b499000,
> narrow_klass_shift = 3
> >>>>>>>>
> >>>>>>>> We always use narrow_klass_shift due to this:
> >>>>>>>>
> >>>>>>>> // CDS uses LogKlassAlignmentInBytes for
> narrow_klass_shift. See
> >>>>>>>> //
> >>>>>>>>
> MetaspaceShared::initialize_dumptime_shared_and_meta_spaces() for
> >>>>>>>> // how dump time narrow_klass_shift is set.
> Although, CDS can
> >>>>>>>> work
> >>>>>>>> // with zero-shift mode also, to be consistent
> with AOT it uses
> >>>>>>>> // LogKlassAlignmentInBytes for klass shift so
> archived java
> >>>>>>>> heap objects
> >>>>>>>> // can be used at same time as AOT code.
> >>>>>>>> if (!UseSharedSpaces
> >>>>>>>> && (uint64_t)(higher_address - lower_base) <=
> >>>>>>>> UnscaledClassSpaceMax) {
> >>>>>>>> CompressedKlassPointers::set_shift(0);
> >>>>>>>> } else {
> >>>>>>>> CompressedKlassPointers::set_shift(LogKlassAlignmentInBytes);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Right. If we relocate to low 32G space, it needs to make
> sure that
> >>>>>>>> the range containing the mapped class data and class
> space must be
> >>>>>>>> encodable.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> > Here are some additional comments (minor).
> >>>>>>>> >
> >>>>>>>> > Could you please fix the long lines in the following?
> >>>>>>>> >
> >>>>>>>> > 1237 void
> >>>>>>>>
> java_lang_Class::update_archived_primitive_mirror_native_pointers(oop
> >>>>>>>> > archived_mirror) {
> >>>>>>>> > 1238 if (MetaspaceShared::relocation_delta() !=
> 0) {
> >>>>>>>> > 1239
> assert(archived_mirror->metadata_field(_klass_offset) ==
> >>>>>>>> > NULL, "must be for primitive class");
> >>>>>>>> > 1240
> >>>>>>>> > 1241 Klass* ak =
> >>>>>>>> >
> ((Klass*)archived_mirror->metadata_field(_array_klass_offset));
> >>>>>>>> > 1242 if (ak != NULL) {
> >>>>>>>> > 1243
> archived_mirror->metadata_field_put(_array_klass_offset,
> >>>>>>>> > (Klass*)(address(ak) +
> MetaspaceShared::relocation_delta()));
> >>>>>>>> > 1244 }
> >>>>>>>> > 1245 }
> >>>>>>>> > 1246 }
> >>>>>>>> >
> >>>>>>>> > src/hotspot/share/memory/dynamicArchive.cpp
> >>>>>>>> >
> >>>>>>>> > 889 Thread* THREAD = Thread::current();
> >>>>>>>> > 890 Method::sort_methods(ik->methods(),
> /*set_idnums=*/true,
> >>>>>>>> > dynamic_dump_method_comparator);
> >>>>>>>> > 891 if (ik->default_methods() != NULL) {
> >>>>>>>> > 892 Method::sort_methods(ik->default_methods(),
> >>>>>>>> > /*set_idnums=*/false,
> dynamic_dump_method_comparator);
> >>>>>>>> > 893 }
> >>>>>>>> >
> >>>>>>>>
> >>>>>>>> OK will do.
> >>>>>>>>
> >>>>>>>> > Please see inlined comments below.
> >>>>>>>> >
> >>>>>>>> > On Mon, Oct 28, 2019 at 9:05 PM Ioi Lam
> <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>
> >>>>>>>> <mailto:ioi.lam at oracle.com
> <mailto:ioi.lam at oracle.com>>> wrote:
> >>>>>>>> >> Hi Jiangli,
> >>>>>>>> >>
> >>>>>>>> >> Thanks for the review. I've updated the patch
> according to your
> >>>>>>>> comments:
> >>>>>>>> >>
> >>>>>>>> >>
> >>>>>>>>
> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v04/
> >>>>>>>>
> >>>>>>>> >>
> >>>>>>>>
> http://cr.openjdk.java.net/~iklam/jdk14/8231610-relocate-cds-archive.v04.delta/
> >>>>>>>>
> >>>>>>>> >>
> >>>>>>>> >> (the delta is on top of
> 8231610-relocate-cds-archive.v03.delta
> >>>>>>>> in my
> >>>>>>>> >> reply to Calvin's comments).
> >>>>>>>> >>
> >>>>>>>> >> On 10/27/19 9:13 PM, Jiangli Zhou wrote:
> >>>>>>>> >>> Hi Ioi,
> >>>>>>>> >>>
> >>>>>>>> >>> Sorry for the delay. Here are my remaining
> comments.
> >>>>>>>> >>>
> >>>>>>>> >>> - src/hotspot/share/memory/dynamicArchive.cpp
> >>>>>>>> >>>
> >>>>>>>> >>> 128 static intx _method_comparator_name_delta;
> >>>>>>>> >>>
> >>>>>>>> >>> The name of the above variable is confusing.
> It's the value of
> >>>>>>>> >>> _buffer_to_target_delta. It's better to
> _buffer_to_target_delta
> >>>>>>>> >>> directly.
> >>>>>>>> >> _buffer_to_target_delta is a non-static field, but
> >>>>>>>> >> dynamic_dump_method_comparator() must be a
> static function so
> >>>>>>>> it can't
> >>>>>>>> >> use the non-static field easily.
> >>>>>>>> >
> >>>>>>>> > It sounds like an issue. _buffer_to_target_delta
> was made as a
> >>>>>>>> > non-static mostly because we might support more
> than one dynamic
> >>>>>>>> > archives in the future. However, today's usages
> bake in an
> >>>>>>>> assumption
> >>>>>>>> > that _buffer_to_target_delta is a singleton
> value. It is
> >>>>>>>> cleaner to
> >>>>>>>> > either make _buffer_to_target_delta as a static
> variable for
> >>>>>>>> now, or
> >>>>>>>> > adding an access API in DynamicArchiveBuilder to
> allow other
> >>>>>>>> code to
> >>>>>>>> > properly and correctly use the value.
> >>>>>>>>
> >>>>>>>> OK, I'll move it to a static variable.
> >>>>>>>>
> >>>>>>>> >
> >>>>>>>> >>> Also, we can do a quick pointer comparison of
> 'a_name' and
> >>>>>>>> >>> 'b_name' first before adjusting the pointers.
> >>>>>>>> >> I added this:
> >>>>>>>> >>
> >>>>>>>> >> if (a_name == b_name) {
> >>>>>>>> >> return 0;
> >>>>>>>> >> }
> >>>>>>>> >>
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 934 void
> DynamicArchiveBuilder::relocate_buffer_to_target() {
> >>>>>>>> >>> ...
> >>>>>>>> >>> 944
> >>>>>>>> >>> 945 ArchivePtrMarker::compact(relocatable_base,
> >>>>>>>> relocatable_end);
> >>>>>>>> >>> ...
> >>>>>>>> >>>
> >>>>>>>> >>> 974 SharedDataRelocator
> patcher((address*)patch_base,
> >>>>>>>> >>> (address*)patch_end, valid_old_base, valid_old_end,
> >>>>>>>> >>> 975 valid_new_base, valid_new_end, addr_delta);
> >>>>>>>> >>> 976 ArchivePtrMarker::ptrmap()->iterate(&patcher);
> >>>>>>>> >>>
> >>>>>>>> >>> Could we reduce the number of data
> re-iterations to help
> >>>>>>>> archive
> >>>>>>>> >>> dumping performance. The
> ArchivePtrMarker::compact operation
> >>>>>>>> can be
> >>>>>>>> >>> combined with the patching iteration.
> >>>>>>>> ArchivePtrMarker::compact API
> >>>>>>>> >>> can be removed.
> >>>>>>>> >> That's a good idea. I implemented it using a
> template parameter
> >>>>>>>> so that
> >>>>>>>> >> we can have max performance when relocating the
> archive at run
> >>>>>>>> time.
> >>>>>>>> >>
> >>>>>>>> >> I added comments to explain why the relocation
> is done here. The
> >>>>>>>> >> relocation is pretty rare (only when the base
> archive was not
> >>>>>>>> mapped at
> >>>>>>>> >> the default location).
> >>>>>>>> >>
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 967 address valid_new_base =
> >>>>>>>> >>> (address)Arguments::default_SharedBaseAddress();
> >>>>>>>> >>> 968 address valid_new_end = valid_new_base +
> >>>>>>>> base_plus_top_size;
> >>>>>>>> >>>
> >>>>>>>> >>> The debugging only code can be included under
> #ifdef ASSERT.
> >>>>>>>> >> These values are actually also used in debug
> logging so they
> >>>>>>>> can't be
> >>>>>>>> >> ifdef'ed out.
> >>>>>>>> >>
> >>>>>>>> >> Also, the c++ compiler is pretty good with
> eliding code
> >>>>>>>> that's no
> >>>>>>>> >> actually used. If I comment out all the logging
> code in
> >>>>>>>> >>
> DynamicArchiveBuilder::relocate_buffer_to_target() and
> >>>>>>>> >> SharedDataRelocator, gcc elides all the unused
> fields and their
> >>>>>>>> >> assignments. So no code is generated for this, etc.
> >>>>>>>> >>
> >>>>>>>> >> address valid_new_base =
> >>>>>>>> >> (address)Arguments::default_SharedBaseAddress();
> >>>>>>>> >>
> >>>>>>>> >> Since #ifdef ASSERT makes the code harder to
> read, I think we
> >>>>>>>> should use
> >>>>>>>> >> it only when really necessary.
> >>>>>>>> > It seems cleaner to get rid of these debugging
> only variables, by
> >>>>>>>> > using 'relocatable_base' and
> >>>>>>>> > '(address)Arguments::default_SharedBaseAddress()'
> in the logging
> >>>>>>>> code.
> >>>>>>>>
> >>>>>>>> SharedDataRelocator is used under 3 different
> situations. These six
> >>>>>>>> variables (patch_base, patch_end, valid_old_base,
> valid_old_end,
> >>>>>>>> valid_new_base, valid_new_end) describes what is
> being patched,
> >>>>>>>> and what
> >>>>>>>> the expectations are, for each situation. The code
> will be hard to
> >>>>>>>> understand without them.
> >>>>>>>>
> >>>>>>>> Please note there's also logging code in the
> SharedDataRelocator
> >>>>>>>> constructor that prints out these values.
> >>>>>>>>
> >>>>>>>> I think I'll just remove the 'debug only' comment
> to avoid
> >>>>>>>> confusion.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Ok.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> >
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 993
> >>>>>>>>
> dynamic_info->write_bitmap_region(ArchivePtrMarker::ptrmap());
> >>>>>>>> >>>
> >>>>>>>> >>> We could combine the archived heap data bitmap
> into the new
> >>>>>>>> region as
> >>>>>>>> >>> well? It can be handled as a separate RFE.
> >>>>>>>> >> I've filed
> https://bugs.openjdk.java.net/browse/JDK-8233093
> >>>>>>>> >>
> >>>>>>>> >>> - src/hotspot/share/memory/filemap.cpp
> >>>>>>>> >>>
> >>>>>>>> >>> 1038 if (is_static()) {
> >>>>>>>> >>> 1039 if (errno == ENOENT) {
> >>>>>>>> >>> 1040 // Not locating the shared archive
> is ok.
> >>>>>>>> >>> 1041 fail_continue("Specified shared
> archive not found
> >>>>>>>> (%s).",
> >>>>>>>> >>> _full_path);
> >>>>>>>> >>> 1042 } else {
> >>>>>>>> >>> 1043 fail_continue("Failed to open
> shared archive file
> >>>>>>>> (%s).",
> >>>>>>>> >>> 1044 os::strerror(errno));
> >>>>>>>> >>> 1045 }
> >>>>>>>> >>> 1046 } else {
> >>>>>>>> >>> 1047 log_warning(cds, dynamic)("specified
> dynamic archive
> >>>>>>>> >>> doesn't exist: %s", _full_path);
> >>>>>>>> >>> 1048 }
> >>>>>>>> >>>
> >>>>>>>> >>> If the top layer is explicitly specified by the
> user, a
> >>>>>>>> warning does
> >>>>>>>> >>> not seem to be a proper behavior if the VM
> fails to open the
> >>>>>>>> archive
> >>>>>>>> >>> file.
> >>>>>>>> >>>
> >>>>>>>> >>> If might be better to handle the relocation
> unrelated code in
> >>>>>>>> separate
> >>>>>>>> >>> changeset and track with a separate RFE.
> >>>>>>>> >> This code was moved from
> >>>>>>>> >>
> >>>>>>>> >>
> >>>>>>>>
> http://hg.openjdk.java.net/jdk/jdk/file/d3382812b788/src/hotspot/share/memory/dynamicArchive.cpp#l1070
> >>>>>>>>
> >>>>>>>> >>
> >>>>>>>> >> so I am not changing the behavior. If you want,
> we can file an
> >>>>>>>> REF to
> >>>>>>>> >> change the behavior.
> >>>>>>>> > Ok. A new RFE sounds like the right thing to
> re-evaluable the
> >>>>>>>> usage
> >>>>>>>> > issue here. Thanks.
> >>>>>>>>
> >>>>>>>> I created
> https://bugs.openjdk.java.net/browse/JDK-8233446
> >>>>>>>>
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 1148 void FileMapInfo::write_region(int region,
> char* base,
> >>>>>>>> size_t size,
> >>>>>>>> >>> 1149 bool
> read_only, bool
> >>>>>>>> allow_exec) {
> >>>>>>>> >>> ...
> >>>>>>>> >>> 1154
> >>>>>>>> >>> 1155 if (region == MetaspaceShared::bm) {
> >>>>>>>> >>> 1156 target_base = NULL;
> >>>>>>>> >>> 1157 } else if (DynamicDumpSharedSpaces) {
> >>>>>>>> >>>
> >>>>>>>> >>> It's not too clear to me how the bitmap (bm)
> region is handled
> >>>>>>>> for the
> >>>>>>>> >>> base layer and top layer. Could you please explain?
> >>>>>>>> >> The bm region for both layers are mapped at an
> address picked
> >>>>>>>> by the OS:
> >>>>>>>> >>
> >>>>>>>> >> char* FileMapInfo::map_relocation_bitmap(size_t&
> bitmap_size) {
> >>>>>>>> >> FileMapRegion* si = space_at(MetaspaceShared::bm);
> >>>>>>>> >> bitmap_size = si->used_aligned();
> >>>>>>>> >> bool read_only = true, allow_exec = false;
> >>>>>>>> >> char* requested_addr = NULL; // allow OS to
> pick any
> >>>>>>>> location
> >>>>>>>> >> char* bitmap_base = os::map_memory(_fd,
> _full_path,
> >>>>>>>> si->file_offset(),
> >>>>>>>> >> requested_addr, bitmap_size,
> >>>>>>>> >> read_only, allow_exec);
> >>>>>>>> >>
> >>>>>>>> > Ok, after staring at the code for a few seconds I
> saw that's
> >>>>>>>> intended.
> >>>>>>>> > If the current region is 'bm', then the
> 'target_base' is NULL
> >>>>>>>> > regardless if it's static or dynamic archive.
> Otherwise, the
> >>>>>>>> > 'target_base' is handled differently for the
> static and dynamic
> >>>>>>>> case.
> >>>>>>>> > The following would be cleaner and has better
> reliability.
> >>>>>>>> >
> >>>>>>>> > char* target_base = NULL;
> >>>>>>>> >
> >>>>>>>> > // The target_base is NULL for 'bm' region.
> >>>>>>>> > if (!region == MetaspaceShared::bm) {
> >>>>>>>> > if (DynamicDumpSharedSpaces) {
> >>>>>>>> > assert(!HeapShared::is_heap_region(region), "dynamic
> >>>>>>>> archive
> >>>>>>>> > doesn't support heap regions");
> >>>>>>>> > target_base =
> DynamicArchive::buffer_to_target(base);
> >>>>>>>> > } else {
> >>>>>>>> > target_base = base;
> >>>>>>>> > }
> >>>>>>>> > }
> >>>>>>>>
> >>>>>>>> How about this?
> >>>>>>>>
> >>>>>>>> char* target_base;
> >>>>>>>> if (region == MetaspaceShared::bm) {
> >>>>>>>> target_base = NULL; // always NULL for bm region.
> >>>>>>>> } else {
> >>>>>>>> if (DynamicDumpSharedSpaces) {
> >>>>>>>> assert(!HeapShared::is_heap_region(region), "dynamic
> >>>>>>>> archive
> >>>>>>>> doesn't support heap regions");
> >>>>>>>> target_base = DynamicArchive::buffer_to_target(base);
> >>>>>>>> } else {
> >>>>>>>> target_base = base;
> >>>>>>>> }
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> No objection If you prefer the extra 'else' block.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> >
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 1362
> >>>>>>>>
> DEBUG_ONLY(header()->set_mapped_base_address((char*)(uintptr_t)0xdeadbeef);)
> >>>>>>>>
> >>>>>>>> >>>
> >>>>>>>> >>> Could you please explain the above?
> >>>>>>>> >> I added the comments
> >>>>>>>> >>
> >>>>>>>> >> // Make sure we don't attempt to use
> >>>>>>>> header()->mapped_base_address()
> >>>>>>>> >> unless
> >>>>>>>> >> // it's been successfully mapped.
> >>>>>>>> >>
> >>>>>>>>
> DEBUG_ONLY(header()->set_mapped_base_address((char*)(uintptr_t)0xdeadbeef);)
> >>>>>>>>
> >>>>>>>> >>
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 1359 FileMapRegion* last_region = NULL;
> >>>>>>>> >>>
> >>>>>>>> >>> 1371 if (last_region != NULL) {
> >>>>>>>> >>> 1372 // Ensure that the OS won't be able
> to allocate new
> >>>>>>>> memory
> >>>>>>>> >>> spaces between any mapped
> >>>>>>>> >>> 1373 // regions, or else it would mess up
> the simple
> >>>>>>>> comparision
> >>>>>>>> >>> in MetaspaceObj::is_shared().
> >>>>>>>> >>> 1374 assert(si->mapped_base() ==
> >>>>>>>> last_region->mapped_end(),
> >>>>>>>> >>> "must have no gaps");
> >>>>>>>> >>>
> >>>>>>>> >>> 1379 last_region = si;
> >>>>>>>> >>>
> >>>>>>>> >>> Can you please place 'last_region' related code
> under #ifdef
> >>>>>>>> ASSERT?
> >>>>>>>> >> I think that will make the code more cluttered.
> The compiler
> >>>>>>>> will
> >>>>>>>> >> optimize out that away.
> >>>>>>>> > It's cleaner to define debugging only variable
> for debugging only
> >>>>>>>> > builds. You can wrapper it and related usage with
> DEBUG_ONLY.
> >>>>>>>>
> >>>>>>>> OK, will do.
> >>>>>>>>
> >>>>>>>> >
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 1478 char*
> FileMapInfo::map_relocation_bitmap(size_t&
> >>>>>>>> bitmap_size) {
> >>>>>>>> >>> 1479 FileMapRegion* si =
> space_at(MetaspaceShared::bm);
> >>>>>>>> >>> 1480 bitmap_size = si->used_aligned();
> >>>>>>>> >>> 1481 bool read_only = true, allow_exec = false;
> >>>>>>>> >>> 1482 char* requested_addr = NULL; // allow OS
> to pick any
> >>>>>>>> location
> >>>>>>>> >>> 1483 char* bitmap_base = os::map_memory(_fd,
> _full_path,
> >>>>>>>> si->file_offset(),
> >>>>>>>> >>> 1484 requested_addr, bitmap_size,
> >>>>>>>> >>> read_only, allow_exec);
> >>>>>>>> >>>
> >>>>>>>> >>> We need to handle mapping failure here.
> >>>>>>>> >> It's handled here:
> >>>>>>>> >>
> >>>>>>>> >> bool FileMapInfo::relocate_pointers(intx
> addr_delta) {
> >>>>>>>> >> log_debug(cds, reloc)("runtime archive
> relocation start");
> >>>>>>>> >> size_t bitmap_size;
> >>>>>>>> >> char* bitmap_base =
> map_relocation_bitmap(bitmap_size);
> >>>>>>>> >> if (bitmap_base != NULL) {
> >>>>>>>> >> ...
> >>>>>>>> >> } else {
> >>>>>>>> >> log_error(cds)("failed to map relocation bitmap");
> >>>>>>>> >> return false;
> >>>>>>>> >> }
> >>>>>>>> >>
> >>>>>>>> > 'bitmap_base' is used immediately after
> map_memory(). So the
> >>>>>>>> check
> >>>>>>>> > needs to be done immediately after map_memory(),
> but not in the
> >>>>>>>> caller
> >>>>>>>> > of map_relocation_bitmap().
> >>>>>>>> >
> >>>>>>>> > 1490 char* bitmap_base = os::map_memory(_fd,
> _full_path,
> >>>>>>>> si->file_offset(),
> >>>>>>>> > 1491 requested_addr, bitmap_size,
> >>>>>>>> > read_only, allow_exec);
> >>>>>>>> > 1492
> >>>>>>>> > 1493 if (VerifySharedSpaces && bitmap_base !=
> NULL &&
> >>>>>>>> > !region_crc_check(bitmap_base, bitmap_size,
> si->crc())) {
> >>>>>>>>
> >>>>>>>> OK, I'll fix that.
> >>>>>>>>
> >>>>>>>> >
> >>>>>>>> >
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 1513 // debug only -- the current value of
> the pointers
> >>>>>>>> to be
> >>>>>>>> >>> patched must be within this
> >>>>>>>> >>> 1514 // range (i.e., must be between the
> requesed base
> >>>>>>>> address,
> >>>>>>>> >>> and the of the current archive).
> >>>>>>>> >>> 1515 // Note: top archive may point to
> objects in the base
> >>>>>>>> >>> archive, but not the other way around.
> >>>>>>>> >>> 1516 address valid_old_base =
> >>>>>>>> (address)header()->requested_base_address();
> >>>>>>>> >>> 1517 address valid_old_end = valid_old_base +
> >>>>>>>> mapping_end_offset();
> >>>>>>>> >>>
> >>>>>>>> >>> Please place all FileMapInfo::relocate_pointers
> debugging only
> >>>>>>>> code
> >>>>>>>> >>> under #ifdef ASSERT.
> >>>>>>>> >> Ditto about ifdef ASSERT
> >>>>>>>> >>
> >>>>>>>> >>> - src/hotspot/share/memory/heapShared.cpp
> >>>>>>>> >>>
> >>>>>>>> >>> 441 void
> >>>>>>>> HeapShared::initialize_from_archived_subgraph(Klass* k) {
> >>>>>>>> >>> 442 if (!open_archive_heap_region_mapped() ||
> >>>>>>>> !MetaspaceObj::is_shared(k)) {
> >>>>>>>> >>> 443 return; // nothing to do
> >>>>>>>> >>> 444 }
> >>>>>>>> >>>
> >>>>>>>> >>> When do we call
> HeapShared::initialize_from_archived_subgraph
> >>>>>>>> for a
> >>>>>>>> >>> klass that's not shared?
> >>>>>>>> >> I've removed the !MetaspaceObj::is_shared(k). I
> probably added
> >>>>>>>> that for
> >>>>>>>> >> debugging purposes only.
> >>>>>>>> >>
> >>>>>>>> >>> 616 DEBUG_ONLY({
> >>>>>>>> >>> 617 Klass* klass = orig_obj->klass();
> >>>>>>>> >>> 618 assert(klass !=
> >>>>>>>> SystemDictionary::Module_klass() &&
> >>>>>>>> >>> 619 klass !=
> >>>>>>>> SystemDictionary::ResolvedMethodName_klass() &&
> >>>>>>>> >>> 620 klass !=
> >>>>>>>> SystemDictionary::MemberName_klass() &&
> >>>>>>>> >>> 621 klass !=
> >>>>>>>> SystemDictionary::Context_klass() &&
> >>>>>>>> >>> 622 klass !=
> >>>>>>>> SystemDictionary::ClassLoader_klass(), "we
> >>>>>>>> >>> can only relocate metaspace object pointers inside
> >>>>>>>> java_lang_Class
> >>>>>>>> >>> instances");
> >>>>>>>> >>> 623 });
> >>>>>>>> >>>
> >>>>>>>> >>> Let's leave the above for a separate RFE. I
> think assert is not
> >>>>>>>> >>> sufficient for the check. Also, why
> ResolvedMethodName,
> >>>>>>>> Module and
> >>>>>>>> >>> MemberName cannot be part of the graph?
> >>>>>>>> >>>
> >>>>>>>> >>>
> >>>>>>>> >> I added the following comment:
> >>>>>>>> >>
> >>>>>>>> >> DEBUG_ONLY({
> >>>>>>>> >> // The following are classes in
> >>>>>>>> share/classfile/javaClasses.cpp
> >>>>>>>> >> that have injected native pointers
> >>>>>>>> >> // to metaspace objects. To support
> these classes, we
> >>>>>>>> need to add
> >>>>>>>> >> relocation code similar to
> >>>>>>>> >> //
> >>>>>>>> java_lang_Class::update_archived_mirror_native_pointers.
> >>>>>>>> >> Klass* klass = orig_obj->klass();
> >>>>>>>> >> assert(klass != SystemDictionary::Module_klass() &&
> >>>>>>>> >> klass !=
> >>>>>>>> SystemDictionary::ResolvedMethodName_klass() &&
> >>>>>>>> >>
> >>>>>>>> > It's too restrictive to exclude those objects
> from the archived
> >>>>>>>> object
> >>>>>>>> > graph because metadata relocation, since metadata
> relocation is
> >>>>>>>> rare.
> >>>>>>>> > The trade-off doesn't seem to buy us much.
> >>>>>>>> >
> >>>>>>>> > Do you plan to add the needed relocation code?
> >>>>>>>>
> >>>>>>>> I looked more into this. Actually we cannot handle
> these 5
> >>>>>>>> classes at
> >>>>>>>> all, even without archive relocation:
> >>>>>>>>
> >>>>>>>> [1] #define MODULE_INJECTED_FIELDS(macro) \
> >>>>>>>> macro(java_lang_Module, module_entry, intptr_signature,
> false)
> >>>>>>>>
> >>>>>>>> -> module_entry is malloc'ed
> >>>>>>>>
> >>>>>>>> [2] #define RESOLVEDMETHOD_INJECTED_FIELDS(macro) \
> >>>>>>>> macro(java_lang_invoke_ResolvedMethodName, vmholder,
> >>>>>>>> object_signature, false) \
> >>>>>>>> macro(java_lang_invoke_ResolvedMethodName, vmtarget,
> >>>>>>>> intptr_signature, false)
> >>>>>>>>
> >>>>>>>> -> these fields are related to method handles and
> lambda forms,
> >>>>>>>> etc.
> >>>>>>>> They can't be easily be archived without
> implementing lambda form
> >>>>>>>> archiving. (I did a prototype; it's very complex
> and fragile).
> >>>>>>>>
> >>>>>>>> [3] #define CALLSITECONTEXT_INJECTED_FIELDS(macro) \
> >>>>>>>> macro(java_lang_invoke_MethodHandleNatives_CallSiteContext,
> >>>>>>>> vmdependencies, intptr_signature, false) \
> >>>>>>>> macro(java_lang_invoke_MethodHandleNatives_CallSiteContext,
> >>>>>>>> last_cleanup, long_signature, false)
> >>>>>>>>
> >>>>>>>> -> vmdependencies is malloc'ed.
> >>>>>>>>
> >>>>>>>> [4] #define
> >>>>>>>> MEMBERNAME_INJECTED_FIELDS(macro) \
> >>>>>>>> macro(java_lang_invoke_MemberName, vmindex, intptr_signature,
> >>>>>>>> false)
> >>>>>>>>
> >>>>>>>> -> this one is probably OK. Despite being declared as
> >>>>>>>> 'intptr_signature', it seems to be used just as an
> integer.
> >>>>>>>> However,
> >>>>>>>> MemberNames are typically used with [2] and [3]. So
> let's just
> >>>>>>>> forbid it
> >>>>>>>> to be safe.
> >>>>>>>>
> >>>>>>>> [2] [3] [4] are not used directly by regular Java
> code and are
> >>>>>>>> unlikely
> >>>>>>>> to be referenced (directly or indirectly) by static
> fields (except
> >>>>>>>> for
> >>>>>>>> the static fields in the classes in
> java.lang.invoke, which we
> >>>>>>>> probably
> >>>>>>>> won't support for heap archiving due to the problem
> I described for
> >>>>>>>> [2]). Objects of these types are typically
> referenced via constant
> >>>>>>>> pool
> >>>>>>>> entries.
> >>>>>>>>
> >>>>>>>> [5] #define CLASSLOADER_INJECTED_FIELDS(macro) \
> >>>>>>>> macro(java_lang_ClassLoader, loader_data, intptr_signature,
> >>>>>>>> false)
> >>>>>>>>
> >>>>>>>> -> loader_data is malloc'ed.
> >>>>>>>>
> >>>>>>>> So, I will change the DEBUG_ONLY into a
> product-mode check, and
> >>>>>>>> quit
> >>>>>>>> dumping if these objects are found in the object
> subgraph.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Sounds good. Can you please also add a comment with
> explanation.
> >>>>>>>>
> >>>>>>>> For ClassLoader and Module, it worth considering caching the
> >>>>>>>> additional native data some time in the future. Lois had
> suggested
> >>>>>>>> the Module part a while ago.
> >>>>>>> I think we can do that if/when we archive Modules directly
> into the
> >>>>>>> shared heap.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Maybe we should backport the check to older
> versions as well?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> We should discuss with Andrew Haley for backports to JDK
> 11 update
> >>>>>>>> releases. Since the current OpenJDK 11 only applies Java heap
> >>>>>>>> archiving to a restricted set of JDK library code, I
> think it is
> >>>>>>>> safe without the new check.
> >>>>>>>>
> >>>>>>>> For non-LTS releases, it might not be worthwhile as they
> may not be
> >>>>>>>> widely used?
> >>>>>>> I agree. FYI, we (Oracle) have no plan for backporting
> more types of
> >>>>>>> heap object archiving, so the decision would be up to
> whoever that
> >>>>>>> decides to do so.
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>> - Ioi
> >>>>>>>
> >>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> Jiangli
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> >
> >>>>>>>> >>> - src/hotspot/share/memory/metaspace.cpp
> >>>>>>>> >>>
> >>>>>>>> >>> 1036 metaspace_rs =
> >>>>>>>> ReservedSpace(compressed_class_space_size(),
> >>>>>>>> >>> 1037 _reserve_alignment,
> >>>>>>>> >>> 1038 large_pages,
> >>>>>>>> >>> 1039 requested_addr);
> >>>>>>>> >>>
> >>>>>>>> >>> Please fix indentation.
> >>>>>>>> >> Fixed.
> >>>>>>>> >>
> >>>>>>>> >>> - src/hotspot/share/memory/metaspaceClosure.hpp
> >>>>>>>> >>>
> >>>>>>>> >>> 78 enum SpecialRef {
> >>>>>>>> >>> 79 _method_entry_ref
> >>>>>>>> >>> 80 };
> >>>>>>>> >>>
> >>>>>>>> >>> Are there other pointers that are not references to
> >>>>>>>> MetaspaceObj? If
> >>>>>>>> >>> _method_entry_ref is the only type, it's
> probably not worth
> >>>>>>>> defining
> >>>>>>>> >>> SpecialRef?
> >>>>>>>> >> There may be more types in the future, so I want
> to have a
> >>>>>>>> stable API
> >>>>>>>> >> that can be easily expanded without touching all
> the code that
> >>>>>>>> uses it.
> >>>>>>>> >>
> >>>>>>>> >>
> >>>>>>>> >>> - src/hotspot/share/memory/metaspaceShared.hpp
> >>>>>>>> >>>
> >>>>>>>> >>> 42 enum MapArchiveResult {
> >>>>>>>> >>> 43 MAP_ARCHIVE_SUCCESS,
> >>>>>>>> >>> 44 MAP_ARCHIVE_MMAP_FAILURE,
> >>>>>>>> >>> 45 MAP_ARCHIVE_OTHER_FAILURE
> >>>>>>>> >>> 46 };
> >>>>>>>> >>>
> >>>>>>>> >>> If we want to define different failure types,
> it's probably
> >>>>>>>> worth
> >>>>>>>> >>> using separate types for relocation failure and
> validation
> >>>>>>>> failure.
> >>>>>>>> >> For now, I just need to distinguish between
> MMAP_FAILURE (where
> >>>>>>>> I should
> >>>>>>>> >> attempt to remap at an alternative address) and
> OTHER_FAILURE
> >>>>>>>> (where the
> >>>>>>>> >> CDS archive loading will fail -- due to
> validation error,
> >>>>>>>> insufficient
> >>>>>>>> >> memory, etc -- without attempting to remap.)
> >>>>>>>> >>
> >>>>>>>> >>> ---
> >>>>>>>> >>>
> >>>>>>>> >>> 193 static intx _mapping_delta; // FIXME rename
> >>>>>>>> >>>
> >>>>>>>> >>> How about _relocation_delta?
> >>>>>>>> >> Changed as suggested.
> >>>>>>>> >>
> >>>>>>>> >>> - src/hotspot/share/oops/instanceKlass
> >>>>>>>> >>>
> >>>>>>>> >>> 1573 bool
> InstanceKlass::_disable_method_binary_search = false;
> >>>>>>>> >>>
> >>>>>>>> >>> The use of _disable_method_binary_search is not
> necessary. You
> >>>>>>>> can use
> >>>>>>>> >>> DynamicDumpSharedSpaces for the purpose. That
> would make things
> >>>>>>>> >>> cleaner.
> >>>>>>>> >> If we always disable the binary search when
> >>>>>>>> DynamicDumpSharedSpaces is
> >>>>>>>> >> true, it will slow down normal execution of the
> Java program
> >>>>>>>> when
> >>>>>>>> >> -XX:ArchiveClassesAtExit has been specified, but
> the program
> >>>>>>>> hasn't exited.
> >>>>>>>> > Could you please add some comments to
> >>>>>>>> _disable_method_binary_search
> >>>>>>>> > with the above explanation? Thanks.
> >>>>>>>>
> >>>>>>>> OK
> >>>>>>>> >
> >>>>>>>> >>> -
> test/hotspot/jtreg/runtime/cds/SpaceUtilizationCheck.java
> >>>>>>>> >>>
> >>>>>>>> >>> 76 if (name.equals("s0") ||
> >>>>>>>> name.equals("s1")) {
> >>>>>>>> >>> 77 // String regions are
> listed at
> >>>>>>>> the end and
> >>>>>>>> >>> they may not be fully occupied.
> >>>>>>>> >>> 78 break;
> >>>>>>>> >>> 79 } else if
> (name.equals("bm")) {
> >>>>>>>> >>> 80 // Bitmap space does
> not have a
> >>>>>>>> requested address.
> >>>>>>>> >>> 81 break;
> >>>>>>>> >>>
> >>>>>>>> >>> It's not part of your change, but could you
> please fix line 76
> >>>>>>>> - 78
> >>>>>>>> >>> since it is trivial. It seems the lines can be
> removed.
> >>>>>>>> >> Removed.
> >>>>>>>> >>
> >>>>>>>> >>> - /src/hotspot/share/memory/archiveUtils.hpp
> >>>>>>>> >>> The file name does not match with the macro
> '#ifndef
> >>>>>>>> >>> SHARE_MEMORY_SHAREDDATARELOCATOR_HPP'. Could
> you please rename
> >>>>>>>> >>> archiveUtils.* ? archiveRelocator.hpp and
> >>>>>>>> archiveRelocator.cpp are
> >>>>>>>> >>> more descriptive.
> >>>>>>>> >> I named the file archiveUtils.hpp so we can move
> other misc
> >>>>>>>> stuff used
> >>>>>>>> >> by dumping into this file (e.g., DumpRegion,
> WriteClosure from
> >>>>>>>> >> metaspaceShared.hpp), since theses are not used
> by the majority
> >>>>>>>> of the
> >>>>>>>> >> files that use metaspaceShared.hpp.
> >>>>>>>> >>
> >>>>>>>> >> I fixed the ifdef.
> >>>>>>>> >>
> >>>>>>>> >>> - src/hotspot/share/memory/archiveUtils.cpp
> >>>>>>>> >>>
> >>>>>>>> >>> 36 void
> ArchivePtrMarker::initialize(CHeapBitMap* ptrmap,
> >>>>>>>> address*
> >>>>>>>> >>> ptr_base, address* ptr_end) {
> >>>>>>>> >>> 37 assert(_ptrmap == NULL, "initialize only
> once");
> >>>>>>>> >>> 38 _ptr_base = ptr_base;
> >>>>>>>> >>> 39 _ptr_end = ptr_end;
> >>>>>>>> >>> 40 _compacted = false;
> >>>>>>>> >>> 41 _ptrmap = ptrmap;
> >>>>>>>> >>> 42 _ptrmap->initialize(12 * M /
> sizeof(intptr_t)); //
> >>>>>>>> default
> >>>>>>>> >>> archive is about 12MB.
> >>>>>>>> >>> 43 }
> >>>>>>>> >>>
> >>>>>>>> >>> Could we do a better estimate here? We could
> guesstimate the
> >>>>>>>> size
> >>>>>>>> >>> based on the current used class space and
> metaspace size. It's
> >>>>>>>> okay if
> >>>>>>>> >>> a larger bitmap used, since it can be reduced
> after all
> >>>>>>>> marking are
> >>>>>>>> >>> done.
> >>>>>>>> >> The bitmap is automatically expanded when
> necessary in
> >>>>>>>> >> ArchivePtrMarker::mark_pointer(). It's only
> about 1/32 or 1/64
> >>>>>>>> of the
> >>>>>>>> >> total archive size, so even if we do expand, the
> cost will be
> >>>>>>>> trivial.
> >>>>>>>> > The initial value is based on the default CDS
> archive. When
> >>>>>>>> dealing
> >>>>>>>> > with a really large archive, it would have to
> re-grow many times.
> >>>>>>>> > Also, using a hard-coded value is less desirable.
> >>>>>>>>
> >>>>>>>> OK, I changed it to the following
> >>>>>>>>
> >>>>>>>> // Use this as initial guesstimate. We should
> need less space
> >>>>>>>> in the
> >>>>>>>> // archive, but if we're wrong the bitmap will
> be expanded
> >>>>>>>> automatically.
> >>>>>>>> size_t estimated_archive_size =
> >>>>>>>> MetaspaceGC::capacity_until_GC();
> >>>>>>>> // But set it smaller in debug builds so we
> always test the
> >>>>>>>> expansion
> >>>>>>>> code.
> >>>>>>>> // (Default archive is about 12MB).
> >>>>>>>> DEBUG_ONLY(estimated_archive_size = 6 * M);
> >>>>>>>>
> >>>>>>>> // We need one bit per pointer in the archive.
> >>>>>>>> _ptrmap->initialize(estimated_archive_size /
> sizeof(intptr_t));
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Thanks!
> >>>>>>>> - Ioi
> >>>>>>>>
> >>>>>>>> >
> >>>>>>>> >>>
> >>>>>>>> >>>
> >>>>>>>> >>> On Wed, Oct 16, 2019 at 4:58 PM Jiangli Zhou
> >>>>>>>> <jianglizhou at google.com
> <mailto:jianglizhou at google.com> <mailto:jianglizhou at google.com
> <mailto:jianglizhou at google.com>>> wrote:
> >>>>>>>> >>>> Hi Ioi,
> >>>>>>>> >>>>
> >>>>>>>> >>>> This is another great step for CDS usability
> improvement.
> >>>>>>>> Thank you!
> >>>>>>>> >>>>
> >>>>>>>> >>>> I have a high level question (or request):
> could we consider
> >>>>>>>> >>>> separating the relocation work for 'direct'
> class metadata
> >>>>>>>> from other
> >>>>>>>> >>>> types of metadata (such as the shared system
> dictionary,
> >>>>>>>> symbol table,
> >>>>>>>> >>>> etc)? Initially we only relocate the tables
> and other
> >>>>>>>> archived global
> >>>>>>>> >>>> data. When each archived class is being
> loaded, we can
> >>>>>>>> relocate all
> >>>>>>>> >>>> the pointers within the current class. We
> could find the
> >>>>>>>> segment (for
> >>>>>>>> >>>> the current class) in the bitmap and update
> the pointers
> >>>>>>>> within the
> >>>>>>>> >>>> segment. That way we can reduce initial
> startup costs and
> >>>>>>>> also avoid
> >>>>>>>> >>>> relocating class data that's not used at
> runtime. In some
> >>>>>>>> real world
> >>>>>>>> >>>> large systems, an archive may contain
> extremely large
> >>>>>>>> number of
> >>>>>>>> >>>> classes.
> >>>>>>>> >>>>
> >>>>>>>> >>>> Following are partial review comments so we
> can move things
> >>>>>>>> forward.
> >>>>>>>> >>>> Still going through the rest of the changes.
> >>>>>>>> >>>>
> >>>>>>>> >>>> - src/hotspot/share/classfile/javaClasses.cpp
> >>>>>>>> >>>>
> >>>>>>>> >>>> 1218 void
> >>>>>>>> java_lang_Class::update_archived_mirror_native_pointers(oop
> >>>>>>>> >>>> archived_mirror) {
> >>>>>>>> >>>> 1219 Klass* k =
> >>>>>>>> ((Klass*)archived_mirror->metadata_field(_klass_offset));
> >>>>>>>> >>>> 1220 if (k != NULL) { // k is NULL for the
> primitive
> >>>>>>>> classes such as
> >>>>>>>> >>>> java.lang.Byte::TYPE <<<<<<<<<<<
> >>>>>>>> >>>> 1221
> archived_mirror->metadata_field_put(_klass_offset,
> >>>>>>>> >>>> (Klass*)(address(k) +
> MetaspaceShared::mapping_delta()));
> >>>>>>>> >>>> 1222 }
> >>>>>>>> >>>> 1223 ...
> >>>>>>>> >>>>
> >>>>>>>> >>>> Primitive type mirrors are handled separately.
> Could you
> >>>>>>>> please verify
> >>>>>>>> >>>> if this call path happens for primitive type
> mirror?
> >>>>>>>> >>>>
> >>>>>>>> >>>> To answer my question above, looks like you
> added the
> >>>>>>>> following, which
> >>>>>>>> >>>> is to be used for primitive type mirrors. That
> seems to be
> >>>>>>>> the reason
> >>>>>>>> >>>> why update_archived_mirror_native_pointers is
> trying to also
> >>>>>>>> cover
> >>>>>>>> >>>> primitive type. It better to have a separate
> API for
> >>>>>>>> primitive type
> >>>>>>>> >>>> mirror, which is cleaner. And, we also can
> replace the above
> >>>>>>>> check at
> >>>>>>>> >>>> line 1220 to be an assert for regular mirrors.
> >>>>>>>> >>>>
> >>>>>>>> >>>> +void ReadClosure::do_mirror_oop(oop *p) {
> >>>>>>>> >>>> + do_oop(p);
> >>>>>>>> >>>> + oop mirror = *p;
> >>>>>>>> >>>> + if (mirror != NULL) {
> >>>>>>>> >>>> +
> >>>>>>>>
> java_lang_Class::update_archived_mirror_native_pointers(mirror);
> >>>>>>>> >>>> + }
> >>>>>>>> >>>> +}
> >>>>>>>> >>>> +
> >>>>>>>> >>>>
> >>>>>>>> >>>> How about renaming
> update_archived_mirror_native_pointers to
> >>>>>>>> >>>> update_archived_mirror_klass_pointers.
> >>>>>>>> >>>>
> >>>>>>>> >>>> It would be good to pass the current klass as
> an argument.
> >>>>>>>> We can
> >>>>>>>> >>>> verify the relocated pointer matches with the
> current klass
> >>>>>>>> pointer.
> >>>>>>>> >>>>
> >>>>>>>> >>>> We should also check if relocation is
> necessary before
> >>>>>>>> spending cycles
> >>>>>>>> >>>> to obtain the
>
More information about the hotspot-runtime-dev
mailing list