[aarch64-port-dev ] RFR(M): 8243392: Remodel CDS/Metaspace storage reservation
Thomas Stüfe
thomas.stuefe at gmail.com
Mon May 18 14:37:30 UTC 2020
Hi all,
fourth webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.03/webrev/
I rebased atop of
changeset: 59325:1ba9a9a3f948
summary: 8244433: Remove saving of RSP in Assembler::pusha_uncached()
so, atop of "8241825: Make compressed oops and compressed class pointers
independent (x86_64, PPC, S390)".
The changes are very small this time, see notes below:
@Ioi:
>Hi Thomas,
>I am running your patch in out CI pipeline now. Some comments:
>[2] Does JDK-8243535 exist with the current jdk/jdk repo?
I assigned JDK-8243535 to me and I'm working on a patch. But I would like
to keep that separate from this work, since this patch is already large.
And I would like to fix JDK-8243535 after this remodel patch has been
pushed, which makes some minor things easier. That would leave a small time
window where NMT does mis-track class space as shared class space, but that
is not a big issue, it does not even disturb any test. Which in itself is
something we may want to fix, come to think about it.
> [3] I think this can be put in header file.
>
> bool Metaspace::class_space_is_initialized() {
> return _class_space_list != NULL;
> }
Done.
> [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.
The problem with that is that at the time this coding runs os::init() did
not yet run and we do not know yet allocation granularity.
So I changed verification of SharedBaseAddress: I added the alignment as
you suggested and moved it to a later time in VM initialization, to CDS
initialization.
>[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();});
I'd like to avoid keep the method LP64 only, but I rewrote the section
somewhat to be more clearer.
> [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 removed the #ifdef, but replaced it with an #ifdef ASSERT. Which is also
not needed, technically, but makes it clear that this section is debugging
checks only.
----
@Coleen:
>
http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.02/webrev/src/hotspot/share/memory/metaspace/
>virtualSpaceList.cpp.udiff.html
>
>Why did you make this change? Shouldn't the caller align it?
You are right, I removed the assert.
---
Thanks all for your review work,
..Thomas
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);
>
> [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);
>
>
> [3] I think this can be put in header file.
>
> bool Metaspace::class_space_is_initialized() {
> return _class_space_list != NULL;
> }
>
> [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 -->|
>
>
> _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().
>
> [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?
>
> [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?
>
> // 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.
>
> [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
>
> The rest of the code looks OK to me, but I may take a look at it again
> after getting more sleep :-)
>
> 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>
> 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