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

[Yumin Qi]

Hi, Thomas

   As long as there are no more output with 
-XX:NativeMemoryTracking=detail (I had wrong impression of it), it is OK 
for adding it to tests.


Thanks

Yumin


On 6/10/20 11:44 AM, Thomas Stüfe wrote:
> Hi Yumin,
>
> On Wed, Jun 10, 2020 at 7:42 PM Yumin Qi <yumin.qi at oracle.com 
> <mailto:yumin.qi at oracle.com>> wrote:
>
>     Hi, Thomas
>
>
>        Ignore my comment --- use_requested_addr is not address. Sorry for
>     this. BTW, I don't think you should change test cases to add NMT
>     tracking, that will give too much information on output. I agree with
>     Calvin you can always using vmoptions + group selection on jtreg for
>     testing your change.
>
>
>
> Thank you for the quick review!
>
> About NMT tracking, unless I am mistaken you would see no output of 
> NMT, since NMT is never reported. We only collect the data.
>
> The reason I added NMT tracking is because it is intricately 
> interwoven with CDS and metaspace initialization, and can easily break.
>
> Thanks, Thomas
>
>     Thanks
>
>     Yumin
>
>     On 6/10/20 10:11 AM, Yumin Qi wrote:
>     > Hi, Thomas
>     >
>     > Â Â  I verified your patch, it looks good to me. One minor comment:
>     >
>     > 2271     result = MAP_ARCHIVE_MMAP_FAILURE;
>     > 2272 log_debug(cds)("Failed to reserve spaces
>     > (use_requested_addr=%u)", (unsigned)use_requested_addr); It is not
>     > intuitive for an address printed as unsigned, could you using
>     > PTR_FORMAT? Thanks Yumin
>     >
>     > On 6/10/20 6:09 AM, Thomas Stüfe 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
>