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

[Yumin Qi]

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