[aarch64-port-dev ] RFR(M): 8243392: Remodel CDS/Metaspace storage reservation
Thomas Stüfe
thomas.stuefe at gmail.com
Thu May 21 06:26:31 UTC 2020
Hi all,
tests went through successfully at SAP two nights in a row. Ioi gave his
okay, and I would like to push today or tomorrow. Any remaining objections`?
The current, final version is:
full:
http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.04/webrev/
delta:
http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev_delta.04/webrev/
Thank you,
Thomas
On Wed, May 20, 2020 at 1:43 AM Ioi Lam <ioi.lam at oracle.com> wrote:
> Hi Thomas,
>
> The testing took a little longer than I expected. I've run your previous
> version:
>
>
> http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.03/webrev/
>
> In our CI tiers 1/4. No failures.
>
> I didn't run your latest version. I would suggesting testing it with your
> own CI as well as in jdk-submit before pushing (after you get OKs from
> other reviewers).
>
> Thumbs up.
> - Ioi
>
>
> On 5/19/20 7:40 AM, Thomas Stüfe wrote:
>
> Hi guys,
>
> new webrev:
>
> full:
> http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.04/webrev/
> delta:
> http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev_delta.04/webrev/
>
> Mostly format changes, breaking up at column 80 as Andrew requested.
>
> The only changes which matter is the removal of the unnecessary include in
> arguments.cpp as Coleen suggested; and to not validate SharedBaseAddress
> anymore at runtime, as Ioi requested.
>
> Also rebased to head.
>
> Thanks, Thomas
>
>
>
>
> On Tue, May 19, 2020 at 6:38 AM Ioi Lam <ioi.lam at oracle.com> wrote:
>
>>
>>
>> On 5/18/20 7:37 AM, Thomas Stüfe wrote:
>>
>> 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.
>>
>>
>> Is the call to check_SharedBaseAddress() necessary inside
>> MetaspaceShared::initialize_runtime_shared_and_meta_spaces()?
>> SharedBaseAddress is currently ignored at run time. Does your patch change
>> that behavior?
>>
>> >[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.
>>
>> The new code looks good.
>>
>> > [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.
>>
>>
>> OK.
>>
>> The rest of the code looks good to me. I am going to run your latest
>> patch in our CI and will report the results.
>>
>> Thanks
>> - Ioi
>>
>> ----
>>
>> @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