RFR(xs): 8247252: TestCombinedCompressedFlags.java failed src/hotspot/share/services/virtualMemoryTracker.cpp:388 Error: ShouldNotReachHere()

[Thomas Stüfe]

Thank you David. I will change the line before pushing.

I am currently waiting for test results from both our CI and jdk submit;
the latter seems stuck again. Will push when test results are done.

Cheers, Thomas

On Thu, Jun 11, 2020 at 9:07 AM David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> On 11/06/2020 4:50 pm, Thomas Stüfe wrote:
> > May I have a second review please?
>
> This all appears reasonable to me.
>
> Nit:
>
> test/hotspot/jtreg/runtime/cds/appcds/TestCombinedCompressedFlags.java
>
> One arg per line please.
>
> Thanks,
> David
>
> > Thanks!
> >
> > ..Thomas
> >
> > On Wed, Jun 10, 2020 at 3:09 PM Thomas Stüfe <thomas.stuefe at gmail.com>
> > wrote:
> >
> >> Hi all,
> >>
> >> could I please have reviews for this bug fix.
> >>
> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8247252
> >> webrev:
> >>
> http://cr.openjdk.java.net/~stuefe/webrevs/8247252-cds-nmt-ccps-off/webrev.00/webrev/
> >>
> >> This bug was introduced with "8243535 NMT may show wrong numbers for CDS
> >> and CCS".
> >>
> >> The error is that a VM loading a shared archive but having compressed
> >> class pointers disabled would reserve space for the CDS but fail to
> >> register it with the correct tag with NMT. So, inside NMT it still is
> >> marked as "mtUnknown". This leads to a follow up error when the actual
> >> archive files are to be mapped into this reserved region, since the
> region
> >> is expected to carry the "mtClassShared" tag for this to work.
> >>
> >> The fix is simple to add the missing registration in metaspaceShared.cpp
> >> when the cds has been reserved for the non-class-space case:
> >>
> >> @@ -2469,10 +2470,12 @@
> >>       archive_space_rs = ReservedSpace(archive_space_size,
> >> archive_space_alignment,
> >>                                        false /* bool large */,
> >> (char*)base_address);
> >>       if (archive_space_rs.is_reserved()) {
> >>         assert(base_address == NULL ||
> >>                (address)archive_space_rs.base() == base_address,
> "Sanity");
> >> +      // Register archive space with NMT.
> >> +      MemTracker::record_virtual_memory_type(archive_space_rs.base(),
> >> mtClassShared);
> >>         return archive_space_rs.base();
> >>       }
> >>       return NULL;
> >>     }
> >>
> >> All the rest is fluff:
> >>
> >> - added logging in a number of places to make analysis of these kinds of
> >> errors simpler
> >> - explicitly switched on NMT for a number of cds jtreg tests.
> >>
> >> ---
> >>
> >> The reason we did not see these errors before in our (SAPs) CI was that
> we
> >> run our tests with NMT (by default) switched off. Apparently Oracle runs
> >> the jtreg tests with NMT enabled via -vmoptions? We will change our CI
> to
> >> do this too. But it would be good if the test flags would be published
> >> somewhere, and we would all use the same flags.
> >>
> >> Thanks, Thomas
> >>
>