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

Thomas Stüfe thomas.stuefe at gmail.com
Thu May 7 14:21:19 UTC 2020 

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