RFR(S) 8208658: Make CDS archived heap regions usable even if compressed oop encoding has changed
Ioi Lam
ioi.lam at oracle.com
Tue Aug 7 22:38:52 UTC 2018
Hi Jiangli,
Thanks for the review.
On 8/7/18 9:25 AM, Jiangli Zhou wrote:
> Hi Ioi,
>
> This is a welcomed change! Thanks for doing this. I'd like to suggest
> building a relocation table at dump time. The table can contain
> entries with pair of 'offset_to_region_start:narrow_oop_value'. Then
> at runtime, we can avoid iterating the objects. That would help to
> reduce the overhead that you are observing when relocation is needed
> at runtime. This could be done as a further optimization in a
> follow-up RFE for 12.
>
That's a good suggestion. I'll try to do that as part of this fix, by
generating a bitmap of all the pointers that need to be relocated. I
will need one bit per 8 bytes of archived heap data, so the (read-only)
memory cost is pretty minimal.
> - src/hotspot/share/memory/heapShared.inline.hpp
>
> - src/hotspot/share/memory/heapShared.cpp
>
> 33 inline oop HeapShared::decode_not_null(narrowOop v) {
> 34 assert(!CompressedOops::is_null(v), "narrow oop value can never
> be zero");
> 35 oop result = (oop)(void*)((uintptr_t)_narrow_oop_base +
> ((uintptr_t)v << _narrow_oop_shift));
> 36 assert(check_obj_alignment(result), "address not aligned: "
> INTPTR_FORMAT, p2i((void*) result));
> 37 return result;
> 38 }
>
> 510 void HeapShared::init_narrow_oop_decoding(address base, int shift) {
> 511 _narrow_oop_base = base;
> 512 _narrow_oop_shift = shift;
> 513 }
>
> - src/hotspot/share/memory/filemap.cpp
>
>
> 913 // dumptime heap end ------------v
> 914 // [ |archived heap regions| ] runtime heap
> end ------v
> 915 // [ |archived heap
> regions| ]
> 916 // |<-----delta-------------------->|
> 917 //
> 918 // At dump time, the archived heap region were near the top
> of the heap.
> 919 // At run time, that region may not be inside the heap, so we
> move it so
> 920 // that it's now near the top of teh runtime time. This can
> be done by
> 921 // the simple math of adding the delta as shown above.
> 922 address dumptime_heap_end =
> (address)_header->_g1_reserved.end();
> 923 address runtime_heap_end =
> (address)G1CollectedHeap::heap()->g1_reserved().end();
> 924 delta = runtime_heap_end - dumptime_heap_end;
> 925 }
> 926
> 927 HeapShared::init_narrow_oop_decoding(narrow_oop_base() + delta,
> narrow_oop_shift());
>
> 1051 class RelocateInternalPointers: public BasicOopIterateClosure {
> ...
> 1056 virtual void do_oop(narrowOop *p) {
> 1057 narrowOop v = *p;
> 1058 if (!CompressedOops::is_null(v)) {
> 1059 oop o = HeapShared::decode_not_null(v);
> 1060 RawAccess<IS_NOT_NULL>::oop_store(p, o);
> 1061 }
> 1062 }
>
> Your relocation change boils down to above sections of code. It works
> as both UnscaledNarrowOop encoding and ZeroBasedNarrowOop encoding use
> 0 as the base. I believe we don't handle heap based encoding with CDS
> enabled. Can you please double check that.
>
It actually does work. See
http://cr.openjdk.java.net/~iklam/jdk12/8208658-relocate-archived-heap-regions.v00/log.txt
Command line: [/home/iklam/jdk/bld/abe-fastdebug/images/jdk/bin/java ...
-Xmx31000m -XX:+UseStringDeduplication Hello ]
[STDOUT]
[0.092s][info][cds] Archived narrow_klass_base = 0x0000000800000000,
narrow_klass_shift = 3
[0.092s][info][cds] Archived narrow_oop_mode = 0, narrow_oop_base =
0x0000000000000000, narrow_oop_shift = 0
[0.092s][info][cds] CDS heap data need to be relocated because
[0.092s][info][cds] the archive was created with an incompatible heap
size: 128M.
[0.092s][info][cds] Current narrow_oop_mode = 2, narrow_oop_base =
0x0000001000000000, narrow_oop_shift = 3
Hello World
> The new HeapShared::decode_not_null() decodes the narrowOop value
> using the dump time encoding mode with added base delta. Can you
> please rename it to be more descriptive, for example
> HeapShared::decode_with_archived_encoding_mode()? Please also add some
> comments to include these detail for this function.
>
OK, I will fix that.
> - src/hotspot/share/memory/filemap.cpp
>
> 886 if (narrow_klass_base() != Universe::narrow_klass_base() ||
> 887 narrow_klass_shift() != Universe::narrow_klass_shift()) {
> 888 log_info(cds)("Cached heap data from the CDS archive need to
> be relocated because");
> 889 log_info(cds)("the CDS archive was created with an
> incompatible heap size: " UINTX_FORMAT "M.", max_heap_size()/M);
> 890 log_info(cds)("Current narrow_klass_base = " PTR_FORMAT ",
> narrow_klass_shift = %d",
> 891 p2i(Universe::narrow_klass_base()),
> Universe::narrow_klass_shift());
> 892 return;
> 893 }
>
> Please fix the log message at 888 as relocation is not done in above
> case.
>
Oops, it should have been "Cached heap data from the CDS archive cannot
be used because", but I messed up with copy-pasting.
> Please move the code at line 895 - 898 to be before the above code, so
> we still have the logging information about the dump time encoding
> modes when runtime narrow_klass_encoding is different. The dump time
> encoding modes are useful debugging information.
>
OK. Will do.
>
> 1039 void FileMapInfo::relocate_archived_heap_embedded_pointers()
>
> 1051 class RelocateInternalPointers: public BasicOopIterateClosure
>
> 1068 void FileMapInfo::relocate_archived_heap_embedded_pointers_impl
>
> Could you please place above new code blocks in heapShared.* files? I
> filed JDK-8206009 to move all heap object archiving code to
> heapShared.*. It's helpful to make sure the related new code goes to
> the right files before we completely resolve existing code for
> JDK-8206009.
>
OK. Will do.
> - src/hotspot/share/gc/g1/heapRegion.cpp
>
> Can you please check the percentage of unused region space if we force
> 8M gc region at dump time? The unused spaces in the archived heap
> regions are filled with dummy objects at runtime and cannot be utilized.
At runtime, the region size is chosen according to -Xmx. The amount of
waste (filled by dummy objects) is related only to the runtime region
size. It's not related to the region size chosen at dump time.
So in this regard, this patch does not increase the amount of runtime
memory waste.
> It would also introduce gaps between the archived heap regions at
> runtime when the GC region size is < 8M with smaller heap size.
Yes, this is an issue for small heaps. Previously, without my patch, if
you run with
java -Xshare:dump -Xmx128m
java -Xshare:on -Xmx128m
the open and closed archive regions will in two consecutive 1MB regions.
With my patch, there will be a gap of 7MB between the 2 regions (each as
still 1MB in size). This means if you run with very small heaps with
CDS, you may not be able to allocate very large arrays.
> We should check with GC team on this and see if there are other
> alternatives to resolve the overlapping issue. One possible solution I
> think is to relocate the 'open' archive heap region further down at
> runtime if the 'open' and 'closed' archive regions are in the same G1
> region at runtime.
>
I'll try implement what you described above (in a separate RFE). This
will make runtime relocation a little slower, but hopefully that will
just be noise, and I won't need to make any change in G1.
For this RFE, I will revert the changes in heapRegion.cpp. This means in
some cases, we won't be able to map the archived regions:
http://cr.openjdk.java.net/~iklam/jdk12/8208658-relocate-archived-heap-regions.v00/log2.txt
Note that 2 out of 26 cases fail to map the heap regions with "Unable to
allocate region, java heap range is already in use",
but it's still works for the majority of the cases, so it still much
better than what we have today
> - test/hotspot/jtreg/runtime/appcds/cacheObject/DifferentHeapSizes.java
>
> Looks good! How about adding some scenarios with heap size > 32G as
> sanity checks.
>
Will do, but with heap size > 32G CDS will be disabled because
compressed oop is disabled.
Thanks
- Ioi
> Thanks,
>
> Jiangli
>
>
> On 8/1/18 11:04 PM, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8208658
>> http://cr.openjdk.java.net/~iklam/jdk12/8208658-relocate-archived-heap-regions.v00/
>>
>>
>> PROBLEM:
>>
>> At runtime, if you use a maximum heap size (-Xmx) setting that's
>> different than the one used at CDS archive creation time, the
>> archived
>> heap regions may fail to map. The reason is the archived heap
>> regions
>> are hard-coded to a specific compressed oop encoding scheme.
>>
>> This causes degradation in start-up time, because the archived heap
>> regions enable many aggressive optimizations, such as JDK-8202035,
>> archiving the system module info.
>>
>> This degradation is undesirable for JDK-8204247 (include default
>> CDS archive in JDK binary) -- what we want is to ship a CDS archive
>> that's optimal regardless of the max heap settings.
>>
>> FIX:
>>
>> If the compressed oop encoding scheme is different:
>> - relocate the archived heap regions so it
>> fits within the runtime heap bounds.
>> - patch all reference fields in the archived objects to
>> use the runtime compressed oop encoding scheme.
>>
>> RESULTS:
>>
>> # dump archive
>> java -Xshare:dump -Xmx2000m
>>
>> # same compressed oop encoding
>> java -Xshare:on -Xmx2000m -Xlog:cds -version
>>
>> # different compressed oop encoding
>> java -Xshare:on -Xmx2090m -Xlog:cds -version
>>
>> Note: the archive is dumped with UnscaledNarrowOop encoding.
>> At run time, if -Xmx2090m is specified, ZeroBasedNarrowOop is
>> used instead.
>>
>> ========================================================================
>> OLD same encoding 34.45 ms +- 0.51%
>> OLD diff encoding 47.76 ms +- 0.94% <<<<< 13.3 ms degradation!
>>
>> NEW same encoding 34.58 ms +- 0.61%
>> NEW diff encoding 35.09 ms +- 0.24% <<<<< 0.5 ms degradation
>> ========================================================================
>>
>>
>> The cost of relocating/patching the archived heap regions is only
>> 0.5 ms. This is much better than the 13.3 ms degradation caused
>> by the
>> failure to map the archived heap regions.
>>
>> TESTING:
>>
>> All 150+ CDS tests passed locally. I am starting tiers 1/2/3 runs.
>>
>> Thanks
>> - Ioi
>
More information about the hotspot-gc-dev
mailing list