RFR(L) 8231610 Relocate the CDS archive if it cannot be mapped to the requested address
Jiangli Zhou
jianglizhou at google.com
Wed Nov 13 15:37:17 UTC 2019
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