[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    RFR(s-m): 8243535: NMT may show wrong numbers for CDS and CCS
From:       Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date:       2020-05-26 6:38:15
Message-ID: CAA-vtUygRfaWiC3MdtCvsTb=mnRPWLwkp6Bvz0+C2Tp3VkTKiA () mail ! gmail ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]