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

[Thomas Stüfe]

Thanks Yumin!



On Thu, Jun 11, 2020, 00:30 Yumin Qi <yumin.qi at oracle.com> wrote:

> 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> 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
>>
>