RFR(M): 8243392: Remodel CDS/Metaspace storage reservation
Ioi Lam
ioi.lam at oracle.com
Tue May 19 23:41:03 UTC 2020
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
> <mailto: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
>> <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);
>>
>> [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 <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 hotspot-dev
mailing list