[aarch64-port-dev ] RFR(M): 8243392: Remodel CDS/Metaspace storage reservation

[Thomas Stüfe]

On Tue, May 12, 2020 at 12:01 AM Ioi Lam <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> 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 :)

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