[aarch64-port-dev ] RFR(M): 8243392: Remodel CDS/Metaspace storage reservation
Ioi Lam
ioi.lam at oracle.com
Tue May 12 15:42:33 UTC 2020
On 5/12/20 5:41 AM, Thomas Stüfe wrote:
>
>
> On Tue, May 12, 2020 at 12:01 AM Ioi Lam <ioi.lam at oracle.com
> <mailto:ioi.lam at oracle.com>> wrote:
>
>
>
> On 5/8/20 12:33 AM, Thomas Stüfe wrote:
>> Hi Ioi,
>>
>> thanks for taking the time to look at this!
>>
>> On Fri, May 8, 2020 at 7:37 AM Ioi Lam <ioi.lam at oracle.com
>> <mailto:ioi.lam at oracle.com>> wrote:
>>
>> Hi Thomas,
>>
>> I am running your patch in out CI pipeline now. Some comments:
>>
>> [1] Do we still need to use _class_space_list? It looks like
>> we just have a single range. Is
>> _class_space_list->current_virtual_space() always the same VS?
>>
>> if (_class_space_list != NULL) {
>> address base =
>> (address)_class_space_list->current_virtual_space()->bottom();
>> address top = base + compressed_class_space_size();
>> st->print("Compressed class space mapped at: " PTR_FORMAT
>> "-" PTR_FORMAT ", size: " SIZE_FORMAT,
>> p2i(base), p2i(top), top - base);
>>
>>
>> Yes it is. Has been bugging me for years too. With ccs, the
>> VirtualSpaceList is degenerated and only contains one node. It
>> still makes some sense code wise. But maybe VirtualSpace*List* is
>> a misnomer.
>>
>> It also ties in into the question whether Metaspace reservations
>> should be able to grow on demand. Oracle folks I talk to tend to
>> say yes. I personally think the "Metaspace is infinite don't
>> worry" meme was broken since ccs was a thing, since
>> CompressedClassSpaceSize is an absolute limit already.
>>
>> I may tweak and simplify the code a bit with Elastic Metaspace.
>> Maybe, as you say, with ccs we can get rid of VirtualSpaceList
>> and deal with the range directly. Have to check.
>>
>> [2] Does JDK-8243535 exist with the current jdk/jdk repo?
>>
>> // This does currently not work because rs may be the
>> result of a split operation
>> // and NMT seems not to be able to handle splits.
>> // See JDK-8243535.
>> //
>> MemTracker::record_virtual_memory_type((address)rs.base(),
>> mtClass);
>>
>>
>> Yes. It shows up more rarely. With this patch, it will show up
>> always. I had a conversation with Zhengyu about this. I think
>> there may be workarounds, like removing the original mapping in
>> NMT and replacing it with the two new ones for archive and ccs.
>> But it seems iffy, I'd rather have this fixed in NMT.
>>
>> I also think it is not such a big deal, but if you think
>> otherwise, I can integrate a workaround into this patch.
>
> My question is -- will NMT (or control-backspace) now always
> report incorrect memory size for the metaspace when CDS is
> enabled? If so, I think a work-around is necessary.
>
>
> What is control backspace? I'm staring at '^H' output in my terminal
> and feel like I miss something :)
If you have a running foreground JVM in your Linux terminal, you can
type "Ctrl-\" and see this. E.g.,
$ java -cp . SleepForever
<...>
Heap
garbage-first heap total 1048576K, used 824K [0x0000000412000000,
0x0000000800000000)
region size 8192K, 1 young (8192K), 0 survivors (0K)
Metaspace used 143K, capacity 4486K, committed 4864K, reserved
1056768K
class space used 6K, capacity 386K, committed 512K, reserved 1048576K
Actually I see that the current output is not correct -- class space
doesn't include the CDS sizes :-(
I am not sure how many people use this anyway, so it may not be important.
Thanks
- Ioi
>
> Today NMT reports wrong memory size at dumptime (I think no-one cares
> probably) and at runtime if the archive got relocated. With the patch
> NMT will also report wrong numbers at runtime when mapping to default
> address.
>
> But okay, I will add a workaround.
>
>>
>> [3] I think this can be put in header file.
>>
>> bool Metaspace::class_space_is_initialized() {
>> return _class_space_list != NULL;
>> }
>>
>>
>> Will do.
>>
>> [4] Why does the CCS need to follow UseLargePagesInMetaspace?
>> This is the reason for the
>> gap in the following chart:
>>
>> // We do this by reserving space for the ccs behind the
>> archives. Note however that
>> // ccs follows a different alignment
>> (Metaspace::reserve_alignment), so there may
>> // be a gap between ccs and cds.
>> // We use a similar layout at runtime, see
>> reserve_address_space_for_archives().
>> //
>> // +-- SharedBaseAddress
>> (default = 0x800000000)
>> // v
>> // +-..---------+---------+ ...
>> +----+----+----+--------+--------------------+
>> // | Heap | Archive | | MC | RW | RO | [gap]
>> | class space |
>> // +-..---------+---------+ ...
>> +----+----+----+--------+--------------------+
>> // |<-- MaxHeapSize -->| |<-- UnscaledClassSpaceMax =
>> 4GB -->|
>>
>>
>> No, nothing to do with large pages. CCS does not have large pages.
>>
>> The gap can result from different alignment rules between
>> metaspace and cds. With Elastic Metaspace, metaspace reservations
>> will be aligned to 4M. So, ccs will have to start at a 4M aligned
>> border.
>>
>>
>> _reserve_alignment is determined here:
>>
>> void Metaspace::ergo_initialize() {
>> if (DumpSharedSpaces) {
>> // Using large pages when dumping the shared archive is
>> currently not implemented.
>> FLAG_SET_ERGO(UseLargePagesInMetaspace, false);
>> }
>>
>> size_t page_size = os::vm_page_size();
>> if (UseLargePages && UseLargePagesInMetaspace) {
>> page_size = os::large_page_size();
>> }
>>
>> _commit_alignment = page_size;
>> _reserve_alignment = MAX2(page_size,
>> (size_t)os::vm_allocation_granularity());
>>
>> But when CDS is enabled, the RS is reserved without large
>> pages, so it should be possible to incrementally commit the
>> CCS using just os::vm_allocation_granularity().
>>
>>
>> Absolutely. CCS does not use large pages for that reason. As I
>> wrote, this is about the new reservation granularity of 4M.
>
> OK, if I understand correctly:
>
> + With the current patch, the gap exists only when
> UseLargePagesInMetaspace is used at run time. The gap actually is
> unnecessary, since ccs doesn't use large pages.
>
> + With the upcoming Elastic Metaspace, _reserve_alignment will be
> 4MB, so we will almost always have a gap.
>
>
> Yes.
>
> Note that we decided to remove the large page feature from Metaspace
> with Elastic Metaspace for now, though we may reintroduce it later in
> a different form. See CSR:
> https://bugs.openjdk.java.net/browse/JDK-8243161 .
>
>
>> [5] I see the aarch64 code in
>> Metaspace::reserve_address_space_for_compressed_classes has
>> been changed. Is this necessary for this RFE, or is it a
>> separate improvement that can be done in a different RFE?
>>
>>
>> Well, it had to be changed somewhat, since the prototype differs.
>> I swapped Metaspace::reserve_preferred_space() for
>> Metaspace::reserve_address_space_for_compressed_classes(), which
>> follows a somewhat different protocol.
>>
>> That aside, the functionality for AARCH64 should be as
>> unchanged as possible. I removed the AIX specific coding, since
>> it was subtly wrong, but that's fine since we are AIX/ppc
>> maintainers.
>>
>>
>> [6] For AARCH64, should we skip the part that reserves it
>> right above the heap? Or does AARCH64 always allocate the
>> heap such that it's a preferable address?
>>
>>
>> Yes and yes :)
>>
>> We do skip that for aarch64 (at least if the address is wrong):
>>
>> 1157 // Try right above the Java heap... 1158 address base =
>> align_up(CompressedOops::end(), Metaspace::reserve_alignment());
>> 1159 assert(base != NULL, "Sanity"); 1160 1161 const size_t size
>> = align_up(CompressedClassSpaceSize,
>> Metaspace::reserve_alignment()); 1162 if
>> (CompressedKlassPointers::is_valid_base(base)) { 1163 rs =
>> ReservedSpace(size, Metaspace::reserve_alignment(), false /*
>> large */, (char*)base); 1164 }
>> At 1162, we check if the attach address would be valid for this
>> platform. If aarch64 says no, we won't reserve there but go
>> straight to the "attach wherever you think is good" part. Which
>> is implemented
>> by Metaspace::reserve_address_space_for_compressed_classes(),
>> which on aarch64 does search a suitable address.
>>
>> // case (b)
>> ReservedSpace rs;
>>
>> // Try right above the Java heap...
>> address base = align_up(CompressedOops::end(),
>> Metaspace::reserve_alignment());
>> assert(base != NULL, "Sanity");
>>
>> const size_t size = align_up(CompressedClassSpaceSize,
>> Metaspace::reserve_alignment());
>> if (CompressedKlassPointers::is_valid_base(base)) {
>> rs = ReservedSpace(size,
>> Metaspace::reserve_alignment(), false /* large */, (char*)base);
>> }
>>
>> // ...failing that, reserve anywhere, but let platform do
>> optimized placement:
>> if (!rs.is_reserved()) {
>> rs =
>> Metaspace::reserve_address_space_for_compressed_classes(size);
>> }
>>
>> [7] argument.cpp checks this:
>>
>> if (UseSharedSpaces || DumpSharedSpaces) {
>> if
>> (!CompressedKlassPointers::is_valid_base((address)SharedBaseAddress))
>> {
>> log_warning(cds)("SharedBaseAddress=" PTR_FORMAT " is
>> invalid for this platform, option will be ignored.",
>> p2i((address)SharedBaseAddress));
>> FLAG_SET_DEFAULT(SharedBaseAddress,
>> default_SharedBaseAddress());
>> }
>> }
>>
>> but initialize_dumptime_shared_and_meta_spaces aligns up the
>> SharedBaseAddress before doing the assert.
>>
>> void
>> MetaspaceShared::initialize_dumptime_shared_and_meta_spaces() {
>> assert(DumpSharedSpaces, "should be called for dump time
>> only");
>>
>> const size_t reserve_alignment =
>> MetaspaceShared::reserved_space_alignment();
>> char* shared_base =
>> (char*)align_up((char*)SharedBaseAddress, reserve_alignment);
>>
>> #ifdef _LP64
>> assert(CompressedKlassPointers::is_valid_base((address)shared_base),
>> "Sanity");
>>
>> So theoretically shared_base may no longer be is_valid_base
>> after the align_up. It probably won't happen, but I think the
>> code will be much clearer tofirst to align_up, then check for
>> is_valid_base and reset to default, and finally assert.
>>
>>
>> Okay, I'll change that.
>>
>> [8] In
>> MetaspaceShared::initialize_dumptime_shared_and_meta_spaces,
>> the following block can be simplified:
>>
>> if (!_shared_rs.is_reserved()) {
>> // Get a reserved space anywhere if attaching at the
>> SharedBaseAddress fails:
>> if (UseCompressedClassPointers) {
>> // If we need a compressed class space too, let the
>> platform handle the reservation.
>> _shared_rs =
>> Metaspace::reserve_address_space_for_compressed_classes(cds_total);
>> } else {
>> // anywhere is fine.
>> _shared_rs = ReservedSpace(cds_total,
>> reserve_alignment, false /* large */, (char*)NULL);
>> }
>> }
>>
>> ... if you change the declaration to:
>>
>> static ReservedSpace
>> reserve_address_space_for_compressed_classes(size_t size)
>> NOT_LP64({ return ReservedSpace();});
>>
>> [9] I think the #ifdef _LP64 is not necessary:
>>
>> #ifdef _LP64
>> // Some sanity checks after reserving address spaces for
>> archives and class space.
>> assert(archive_space_rs.is_reserved(), "Sanity");
>> if (Metaspace::using_class_space()) {
>> // Class space must closely follow the archive space.
>> Both spaces must be aligned correctly.
>> assert(class_space_rs.is_reserved(), "A class space
>> should have been reserved");
>> assert(class_space_rs.base() >= archive_space_rs.end(),
>> "class space should follow the cds archive space");
>> assert(is_aligned(archive_space_rs.base(),
>> MetaspaceShared::reserved_space_alignment()), "Archive space
>> misaligned");
>> assert(is_aligned(class_space_rs.base(),
>> Metaspace::reserve_alignment()), "class space misaligned");
>> }
>> #endif
>>
>>
>> I'll take a look.
>>
>> The rest of the code looks OK to me, but I may take a look at
>> it again after getting more sleep :-)
>>
>>
>> Sure. Thanks man! :)
>
> Good news. I ran our tiers 1-4 and there were no failures related
> to your changes!
>
>
> Neat. This mirrors our results.
>
> I'll prepare a new webrev.
>
> ..Thomas
More information about the aarch64-port-dev
mailing list