[aarch64-port-dev ] RFR(M): 8243392: Remodel CDS/Metaspace storage reservation
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri May 15 22:34:21 UTC 2020
One more in-context comment.
On 5/8/20 3: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.
Yes, this could be an improvement for a later time. The compressed
class space isn't a list. I think that was the least amount of special
code place to hook it in at the time. It makes sense for it to be a
single VirtualSpaceNode.
CompressedClassSpace + Data (the rest of Metaspace) has a complicated
relationship for determining high water mark for starting GC, and for
reporting LowMemory thresholds. I'd have to look up what was decided there.
Data (the rest of Metaspace) does grow on demand unless someone uses
-XX:MaxMetaspaceSize and then class metaspace size is factored in (maybe
just committed space not the whole 1G). HTH.
Coleen
>
> [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.
>
>
> [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.
>
> [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! :)
>
> ..Thomas
>
> Thanks
> - Ioi
>
>
>
>
> On 5/7/20 7:21 AM, Thomas Stüfe wrote:
>> Hi all,
>>
>> please take a look at the third iteration of this change:
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.02/webrev/
>>
>> Changes in this version:
>>
>> - at the request of Coleen (sorry I did not see your post
>> earlier) I removed all platform dependent files and put the
>> platform dependent coding back to metaspace.cpp and
>> compressedOops.cpp to ease the review pain. I used plain platform
>> defines though (#ifdef AARCH64) instead of hiding them behind
>> another macro to make things clearer. Note that I still intent to
>> put this code away into the platform corners but will do so in a
>> follow up RFE.
>>
>> - I reinstated, in a fashion, the special handling of
>> reservations on Windows. On all platforms we reserve address
>> space to map the archive files in with a subsequent mapping
>> operation. However, on Windows, we cannot use MapViewOfFile()
>> into an existing mapping. So I remove the mapping again before
>> mapping the archives - see comment in code for details.
>>
>> All CI tests at SAP run through without problems, including on
>> Win64 and aarch64, but I would be happy if others were to run
>> test too.
>>
>> Thank you, Thomas
>>
>> On Tue, Apr 28, 2020 at 4:54 PM Thomas Stüfe
>> <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>> wrote:
>>
>> Hi all,
>>
>> Could I have reviews for the following proposal of reworking
>> cds/class space reservation?
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8243392
>>
>> Webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.00/webrev/
>>
>> (Many thanks to Ioi Lam for so patiently explaining CDS
>> internals to me, and to Andrew Haley and Nick Gasson for help
>> with aarch64!)
>>
>> Reservation of the compressed class space is needlessly
>> complicated and has some minor issues. It can be simplified
>> and made clearer.
>>
>> The complexity stems from the fact that this area lives at
>> the intersection of two to three sub systems, depending on
>> how one counts. Metaspace, CDS, and the platform which may or
>> may not its own view of how to reserve class space. And this
>> code has been growing organically over time.
>>
>> One small example:
>>
>> ReservedSpace Metaspace::reserve_preferred_space(size_t size,
>> size_t alignment,
>> bool large_pages, char *requested_addr,
>> bool use_requested_addr)
>>
>> which I spent hours decoding, resulting in a very confused
>> mail to hs-runtime and aarch64-port-dev [2].
>>
>> This patch attempts to simplify cds and metaspace setup a
>> bit; to comment implicit knowledge which is not immediately
>> clear; to cleanly abstract platform concerns like optimized
>> class space placement; and to disentangle cds from metaspace
>> to solve issues which may bite us later with Elastic
>> Metaspace [4].
>>
>> ---
>>
>> The main change is the reworked reservation mechanism. This
>> is based on Ioi's proposal [5].
>>
>> When reserving class space, three things must happen:
>>
>> 1) reservation of the space obviously. If cds is active that
>> space must be in the vicinity of cds archives to be covered
>> by compressed class pointer encoding.
>> 2) setting up the internal Metaspace structures atop of that
>> space
>> 3) setting up compressed class pointer encoding.
>>
>> In its current form, Metaspace may or may not do some or all
>> of that in one function
>> (Metaspace::allocate_metaspace_compressed_klass_ptrs(ReservedSpace
>> metaspace_rs, char* requested_addr, address cds_base);) - if
>> cds is active, it will reserve the space for Metaspace and
>> hand it in, otherwise it will create it itself.
>>
>> When discussing this in [2], Ioi proposed to move the
>> reservation of the class space completely out of Metaspace
>> and make it a responsibility of the caller always. This would
>> reduce some complexity, and this patch follows the proposal.
>>
>> I removed
>> Metaspace::allocate_metaspace_compressed_klass_ptrs(ReservedSpace
>> metaspace_rs, char* requested_addr, address cds_base); and
>> all its sub functions.
>>
>> (1) now has to always be done outside - a ReservedSpace for
>> class space has to be provided by the caller. However,
>> Metaspace now offers a utility function for reserving space
>> at a "nice" location, and explicitly doing nothing else:
>>
>> ReservedSpace
>> Metaspace::reserve_address_space_for_compressed_classes(size_t
>> size);
>>
>> this function can be redefined on a platform level for
>> platform optimized reservation, see below for details.
>>
>> (2) is taken care of by a new function,
>> Metaspace::initialize_class_space(ReservedSpace rs)
>>
>> (3) is taken care of a new function
>> CompressedKlassPointers::initialize(), see below for details.
>>
>>
>> So, class space now is set up three explicit steps:
>>
>> - First, reserve a suitable space by however means you want.
>> For convenience you may use
>> Metaspace::reserve_address_space_for_compressed_classes(), or
>> you may roll your own reservation.
>> - Next, tell Metaspace to use that range as backing storage
>> for class space:
>> Metaspace::initialize_class_space(ReservedSpace rs)
>> - Finally, set up encoding. Encoding is independent from the
>> concept of a ReservedSpace, it just gets an address range,
>> see below for details.
>>
>> Separating these steps and moving them out of the
>> responsibility of Metaspace makes this whole thing more
>> flexible; it also removes unnecessary knowledge (e.g.
>> Metaspace does not need to know anything about either ccp
>> encoding or cds).
>>
>> ---
>>
>> How it comes together:
>>
>> If CDS is off, we just reserve a space using
>> Metaspace::reserve_address_space_for_compressed_classes(),
>> initialize it with
>> Metaspace::initialize_class_space(ReservedSpace rs), then set
>> up compressed class pointer encoding covering the range of
>> this class space.
>>
>> If CDS is on (dump time), we reserve large 4G space, either
>> at SharedBaseAddress or using
>> Metaspace::reserve_address_space_for_compressed_classes(); we
>> then split that into 3G archive space and 1G class space; we
>> set up that space with Metaspace as class space; then we set
>> up compressed class pointer encoding covering both archive
>> space and cds.
>>
>> If CDS is on (run time), we reserve a large space, split it
>> into archive space (large enough to hold both archives) and
>> class space, then basically proceed as above.
>>
>> Note that this is almost exactly how things worked before
>> (modulo some minor fixes, e.g. alignment issues), only the
>> code is reformed and made more explicit.
>>
>> ---
>>
>> I moved compressed class pointer setup over to
>> CompressedKlassPointers and changed the interface:
>>
>> -void
>> Metaspace::set_narrow_klass_base_and_shift(ReservedSpace
>> metaspace_rs, address cds_base)
>> +void CompressedKlassPointers::initialize(address addr,
>> size_t len);
>>
>> Instead of feeding it a single ReservedSpace, which is
>> supposed to represent class space, and an optional alternate
>> base if cds is on, now we give it just an numeric address
>> range. That range marks the limits to where Klass structures
>> are to be expected, and is the implicit promise that outside
>> that range no Klass structures will exist, so encoding has to
>> cover only this range.
>>
>> This range may contain just the class space; or class
>> space+cds; or whatever allocation scheme we come up with in
>> the future. Encoding does not really care how the memory is
>> organized as long as the input range covers all possible
>> Klass locations. That way we remove knowledge about class
>> space/cds from compressed class pointer encoding.
>>
>> Moving it away from metaspace.cpp into the
>> CompressedKlassPointers class also mirrors
>> CompressedOops::initialize().
>>
>> ---
>>
>> I renamed _narrow_klass_range to just _range, because
>> strictly speaking this is the range un-narrow Klass pointers
>> can have.
>>
>> As for the implementation of
>> CompressedKlassPointers::initialize(address addr, size_t
>> len), I mimicked very closely what happened before, so there
>> should be almost no differences. Since "almost no
>> differences" sounds scary :) here are the differences:
>>
>> - When CDS is active (dump or run time) we now always,
>> unconditionally, set the encoding range to 4G. This fixes a
>> theoretical bug discussed on aarch64-port-dev [1].
>>
>> - When CDS is not active, we set the encoding range to the
>> minimum required length. Before, it was left at its default
>> value of 4G.
>>
>> Both differences only affect aarch64, since they are
>> currently the only one using the range field in
>> CompressedKlassPointers.
>>
>> I wanted to add an assert somewhere to test encoding of the
>> very last address of the CompressedKlassPointers range, again
>> to prevent errors like [3]. But I did not come up with a good
>> place for this assert which would cover also the encoding
>> done by C1/C2.
>>
>> For the same reason I thought about introducing a mode where
>> Klass structures would be allocated in reverse order,
>> starting at the end of the ccs, but again left it out as too
>> big a change.
>>
>> ---
>>
>> OS abstraction: platforms may have restrictions of what
>> constitutes a valid compressed class pointer encoding base.
>> Or if not, they may have at least preferences. There was
>> logic like this in metaspace.cpp, which I removed and cleanly
>> factored out into platform dependent files, giving each
>> platform the option to add special logic.
>>
>> These are two new methods:
>>
>> - bool CompressedKlassPointers::is_valid_base(address p)
>>
>> to let the platform tell you whether it considers p to be a
>> valid encoding base. The only platform having these
>> restrictions currently is aarch64.
>>
>> - ReservedSpace
>> Metaspace::reserve_address_space_for_compressed_classes(size_t
>> size);
>>
>> this hands over the process of allocating a range suitable
>> for compressed class pointer encoding to the platform. Most
>> platforms will allocate just anywhere, but some platforms may
>> have a better strategy (e.g. trying low memory first, trying
>> only correctly aligned addresses and so on).
>>
>> Beforehand, this coding existed in a similar form in
>> metaspace.cpp for aarch64 and AIX. For now, I left the AIX
>> part out - it seems only half done, and I want to check
>> further if we even need it, if yes why not on Linux ppc, and
>> C1 does not seem to support anything other than base+offset
>> with shift either, but I may be mistaken.
>>
>> These two methods should give the platform enough control to
>> implement their own scheme for optimized class space
>> placement without bothering any shared code about it.
>>
>> Note about the form, I introduced two new platform dependent
>> files, "metaspace_<cpu>.cpp" and "compressedOops_<cpu>.cpp".
>> I am not happy about this but this seems to be what we
>> generally do in hotspot, right?
>>
>> ---
>>
>> Metaspace reserve alignment vs cds alignment
>>
>> CDS was using Metaspace reserve alignment for CDS internal
>> purposes. I guess this was just a copy paste issue. It never
>> caused problems since Metaspace reserve alignment == page
>> size, but that is not true anymore in the upcoming Elastic
>> Metaspace where reserve alignment will be larger. This causes
>> a number of issues.
>>
>> I separated those two cleanly. CDS now uses
>> os::vm_allocation_granularity. Metaspace::reserve_alignment
>> is only used in those two places where it is needed, when CDS
>> creates the address space for class space on behalf of the
>> Metaspace.
>>
>> ---
>>
>> Windows special handling in CDS
>>
>> To simplify coding I removed the windows specific handling
>> which left out reservation of the archive. This was needed
>> because windows cannot mmap files into reserved regions. But
>> fallback code exists in filemap.cpp for this case which just
>> reads in the region instead of mapping it.
>>
>> Should that turn out to be a performance problem, I will
>> reinstate the feature. But a simpler way would be reserve the
>> archive and later just before mmapping the archive file to
>> release the archive space. That would not only be simpler but
>> give us the best guarantee that that address space is
>> actually available. But I'd be happy to leave that part out
>> completely if we do not see any performance problems on
>> windows x64.
>>
>> ---
>>
>> NMT cannot deal with spaces which are split. This problem
>> manifests in that bookkeeping for class space is done under
>> "Shared Classes", not "Classes" as it should. This problem
>> exists today too at dump time and randomly at run time. But
>> since I simplified the reservation, this problem now shows up
>> always, whether or not we map at the SharedBaseAddress.
>> While I could work around this problem, I'd prefer this
>> problem to be solved at the core, and NMT to have an option
>> to recognize reservation splits. So I'd rather not put a
>> workaround for this into the patch but leave it for fixing as
>> a separate issue. I opened this issue to track it [6].
>>
>> ---
>>
>> Jtreg tests:
>>
>> I expanded the CompressedOops/CompressedClassPointers.java. I
>> also extended them to Windows. The tests now optionally omit
>> strict class space placement tests, since these tests heavily
>> depend on ASLR and were the reason they were excluded on
>> Windows. However I think even without checking for class
>> space placement they make sense, just to see that the VM
>> comes up and lives with the many different settings we can
>> run in.
>>
>> ---
>>
>> Tests:
>>
>> - I ran the patch through Oracles submit repo
>> - I ran tests manually for aarch64, zero, linux 32bit and
>> windows x64
>> - The whole battery of nightly tests at SAP, including ppc,
>> ppcle and aarch64, unfortunately excluding windows because of
>> unrelated errors. Windows x64 tests will be redone tonight.
>>
>>
>> Thank you,
>>
>> Thomas
>>
>> [1]
>> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-April/008804.html
>> [2]
>> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-April/008757.html
>> [3] https://bugs.openjdk.java.net/browse/JDK-8193266
>> [4] https://bugs.openjdk.java.net/browse/JDK-8221173
>> [5]
>> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-April/008765.html
>> [6] https://bugs.openjdk.java.net/browse/JDK-8243535
>>
>
More information about the aarch64-port-dev
mailing list