Fwd: RFR(s-m): 8243535: NMT may show wrong numbers for CDS and CCS

[Thomas Stüfe]

---------- Forwarded message ---------
From: Zhengyu Gu <zgu at redhat.com>
Date: Mon, Jun 8, 2020 at 2:11 PM
Subject: Re: RFR(s-m): 8243535: NMT may show wrong numbers for CDS and CCS
To: Thomas Stüfe <thomas.stuefe at gmail.com>, Ioi Lam <ioi.lam at oracle.com>


Hi Thomas,

NMT part looks good to me.


Thanks,

-Zhengyu

On 6/8/20 4:43 AM, Thomas Stüfe wrote:
> Hi Zhengyu and Ioi,
>
> would it be possible to get a review for this this week?
>
> I understand if this is not possible, but it would be nice to get this
> into 15.
>
> Thanks, Thomas
>
>
>
> On Tue, May 26, 2020 at 8:38 AM Thomas Stüfe <thomas.stuefe at gmail.com
> <mailto:thomas.stuefe at gmail.com>> wrote:
>
>     Hi,
>
>     please take a look at this fix:
>
>     JBS: https://bugs.openjdk.java.net/browse/JDK-8243535?filter=-3
>     Webrev:
>
http://cr.openjdk.java.net/~stuefe/webrevs/8243535--nmt-does-not-handle-os--split_reserved_space()/webrev.00/webrev/
>
>     NMT is not able to correctly follow os::split_reserved_memory
>     operations. This causes misreports for cds and class space (see JBS
>     for details).
>
>     The patch adds support to NMT for following split operations, and
>     fixes up the CDS side of things.
>
>     Changes in NMT:
>
>     - In virtualMemoryTracker.hpp/cpp, we now have a new
>     operation "split_reserved_region()". This one takes a reserved
>     region in NMT and splits it in two adjacent regions while keeping
>     the MEMFLAG tags and the original call stack intact.
>
>     - For that to work I needed to officially abandon the notion that we
>     collapse adjacent regions in NMT. In practice, we did never do this
>     due to a coding error, but now we also do not want this. See
>     discussion with Zhengyu about this here:
>
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-May/039525.html
>
>     - As a small cleanup I also moved the implementation of
>     (Virtual|Committed)MemoryRegion::compare() and
>     (Virtual|Committed)MemoryRegion::equals() up to the base class,
>     since they were identical, and simplified compare() a bit.
>
>     - In memTracker.hpp I just marshall the new call through like it is
>     done here
>
>     - MemTracker::record_virtual_memory_split_reserved gets now called
>     from the Posix version of os::split_reserved_memory() to register
>     the split. On Windows, this is not necessary, since a split involves
>     releasing and re-reserving.
>
>     ---
>
>     Changes to/for CDS:
>
>     CDS maps archive files into pre-reserved space (on Posix) and
>     mapping into reserved space is not handled by NMT on a general
>     level, that is why inside NMT there exists special logic to deal
>     with CDS archive file mappings. I left those alone.
>
>     But I simplified/changed the logic of registration somewhat. In
>     FileMapInfo::map_region(), before, we would register the archive
>     spaces with NMT explicitly after mapping. This was done with two
>     separate calls to NMT: the first one for the case where we map into
>     pre-reserved space (which inside NMT actually is a noop since this
>     case is ignored, see above) and once for the case that we mapped
>     archive files into unreserved space (windows only).
>
>     I needed a while to understand the logic. Then I simplified as
>     follows: I added a MEMFLAGS parameter to os::map_memory() which gets
>     uses when registering the mapping with NMT now. That way, no follow
>     up tag correction is needed, the mapping has the right tag right
>     away. This takes care of the case when we map into unreserved regions.
>
>     In addition to this, in
>     MetaspaceShared::reserve_address_space_for_archives(), when
>     establishing the space for archive and class space, I now register
>     those two spaces with NMT after creating them.
>
>     That makes for three different cases:
>
>     -> case 1 (posix):
>        1 we create the spaces
>     in MetaspaceShared::reserve_address_space_for_archives(). Register
>     them with NMT.
>        2 in FileMapInfo::map_region(), we then map the archive files in
>     one by one using os::map_memory(). This will attempt to register
>     these regions, but inside NMT this case is ignored.
>
>     -> case 2 (windows, first attempt)
>        1 we create the spaces
>     in MetaspaceShared::reserve_address_space_for_archives(). Register
>     them with NMT.
>        2 we then delete the archive space again to make space for
>     windows file mapping. This removes in NMT the registered address
>     range for the archive space.
>        3 in FileMapInfo::map_region(), we then map the archive files in
>     one by one using os::map_memory(). This will register each region
>     separately with NMT.
>
>     -> case 3 (windows, second attempt)
>        1 we create the spaces
>     in MetaspaceShared::reserve_address_space_for_archives(). Register
>     them with NMT.
>        2 in FileMapInfo::map_region(), we read the content of the
>     archive file sequentially. Nothing happens at NMT.
>
>     If all this sounds complicated, it is less complicated than it was
>     before :)
>
>     Note that since we do not merge adjacent regions, in detail report
>     the archive files may now appear as separate entities. I do not
>     really care, I even think this may be a feature. But if this is not
>     wanted, Zhengyu proposed to merge adjacent regions when displaying
>     them:
>
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-May/039534.html
 .
>     That would be a separate issue though.
>
>     Tests ran at SAP CI tonight sucessfully.
>
>     Thanks, Thomas
>
>
>
>
>
>
>
>