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

[Thomas Stüfe]

Hi,

I updated the webrev and rebased it to jdk head (now atop
of 4198213fc371: 8230940: Obsolete MonitorBound).

New webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev.01/webrev/
Delta to last:
http://cr.openjdk.java.net/~stuefe/webrevs/rework-cds-ccs-reservation/webrev_delta.01/webrev/index.html

@Ioi Lam <ioi.lam at oracle.com> :

>   // User may have specified an invalid base address. Should we ignore
>it or assert?
>guarantee(CompressedKlassPointers::is_valid_base((address)shared_base),
>             "SharedBaseAddress: " PTR_FORMAT " is not a valid base.",
>p2i(shared_base));
>
>This will cause the VM to crash. I think it's better (1) exit the VM
>properly with an error code, or (2) override the user's input.

I reworked this coding. I opted for ignoring this - if the user specifies a
SharedBaseAddress which is invalid on the given platform the VM will print
a warning and choose the default value. This mirrors (roughly) how we
handle mapping errors when we cannot attach at SharedBaseAddress due to
ASLR, which is also not treated as a fatal error.

>For testing the CDS relocation code, I would suggest running:
>
>cd test/hotspot/jtreg
>jtreg -javaoption:-XX:+UnlockDiagnosticVMOptions \
>       -javaoption:-XX:ArchiveRelocationMode=1 \
>       -javaoption:-XX:NativeMemoryTracking=detail
>       :hotspot_cds_relocation
>
>This will place the CCS at random locations picked by the OS.

Tested on my linux x64 box, all went well.

>metaspace.cpp:
>
>If your intention is to "shake things up a little", it's not a good idea
>to include it in a complex change set. If things indeed go wrong, we
>don't know who caused it (your CCS changes, or old bugs triggered by
>this debug code), and we will end up backing out the entire changeset.

I see your point and removed that code. I am not completely happy with it
because this code is supposed to test whether this patch works as intended
(removing all dependencies to Metaspace::reserve_alignment()). But since I
have enough test runs with an increased alignment, I can disable this and
will move this into a separate RFR.

>I would suggest putting this in a different RFE, and even push it now.

Cannot push it now, since it depends on this rework patch. In isolation it
will cause crashes in CDS, which is one of the reason for this rework.

@Nick Gasson <nick.gasson at arm.com> :

>On line 42 I'd prefer to use (4 << LogKlassAlignmentInBytes)*G as it
>currently is in metaspace.cpp instead of the literal 32.

Done.

As for the discussions about optimizing the compressed class pointer
encoding on aarch64, unless you have specific wishes about this patch I am
ignoring it for now; I understand you want to do any optimizations for that
in a follow up patch.

Thank you,

Thomas