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

[Thomas Stüfe]

Thank you Ioi! I will correct the typo before pushing.

..Thomas

On Mon, Jun 8, 2020 at 7:18 PM Ioi Lam <ioi.lam at oracle.com> wrote:

> HI Thomas,
>
> The CDS changes look good to me. On the NMT part, I took a cursory look
> and didn't notice any problems, but I am not familiar with NMT to say
> anything definitive.
>
> One typo:
>
>    //  They also will be tracked individually by NMT and can be tagged
> with different flags.
>    //  Note that this may cause the original space to loose its content.
>
> loose -> lose
>
> Thanks
> - Ioi
>
> On 6/8/20 5:14 AM, Thomas Stüfe wrote:
> > ---------- 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
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
>
>