[16] RFR(T) 8248426: NMT: VirtualMemoryTracker::split_reserved_region() does not properly update summary counting

[Thomas Stüfe]

Hi Zhengyu,

sorry for the wait.

On Tue, Jun 30, 2020 at 3:51 PM Zhengyu Gu <zgu at redhat.com> wrote:

> Hi Thomas,
>
> On 6/30/20 12:26 AM, Thomas Stüfe wrote:
> >
> >
> > On Mon, Jun 29, 2020 at 4:29 PM Zhengyu Gu <zgu at redhat.com
> > <mailto:zgu at redhat.com>> wrote:
> >
> >     Hi Thomas,
> >
> >     On 6/29/20 2:30 AM, Thomas Stüfe wrote:
> >      >
> >      >     - splitting the reserved region
> >      >
> >      >     Could we not simply add the call
> >      >     to VirtualMemorySummary::record_released_memory(size,
> >      >     reserved_rgn->flag()); to
> >      >     VirtualMemoryTracker::split_reserved_region() instead?
> >
> >     I assume you could. Then removing and adding reserved region calls
> >     are asymmetrical.
> >
> >
> > How so?
>
> I don't like to mix different level calls in one method.
>
> >
> >     For this particular case, remove_released_region() turns out to be
> >     exactly what you suggested, since reserved_rgn->same_region(addr,
> size)
> >     == true.
> >
> >     ...
> >     VirtualMemorySummary::record_released_memory(size,
> >     reserved_rgn->flag());
> >
> >     ...
> >
> >     return _reserved_regions->remove(rgn);
> >
> >
> >
> > One other disadvantage is that
> > using VirtualMemoryTracker::remove_released_region() will do the region
> > lookup again, so we pay twice for the lookup.
>
> Yep.
>
> How about refactoring remove_released_region() method into two? it
> addresses both problems.
>
> http://cr.openjdk.java.net/~zgu/JDK-8248426/webrev.01/index.html
>
>
This looks good, but why did you remove the "!same region" condition? I
believe that is needed for the case when CDS' first mapping encounters
errors, so before it rebuilds CDS at another location it removes (a) all
mappings and then (b) the enclosing reservation. (a) should be ignored by
NMT but (b) should not. I may be wrong, I have had no coffee yet.

Cheers, Thomas

Thanks,
>
> -Zhengyu
>
> >
> > But if you are still unconvinced, I won't hold you up. The change
> > certainly works as it is now and is okay for me, it just would not be my
> > preferred solution.
> >
> > Cheers, Thomas
> >
> >     Thanks,
> >
> >     -Zhengyu
> >
> >
> >      >
> >      >     Thanks, Thomas
> >      >
> >      >
> >      >
> >      >
> >      >
> >      >     On Fri, Jun 26, 2020 at 10:54 PM Zhengyu Gu <zgu at redhat.com
> >     <mailto:zgu at redhat.com>
> >      >     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>> wrote:
> >      >
> >      >         Hi,
> >      >
> >      >         Please review this trivial patch that fixes summary
> >     counting in
> >      >         VirtualMemoryTracker::split_reserved_region().
> >      >
> >      >         The method uses internal method to remove a reserved
> region,
> >      >         which does
> >      >         not update counting information. It should use high level
> >     tracking
> >      >         method VirtualMemoryTracker::remove_released_region()
> >     instead.
> >      >
> >      >         Without patch, NMT summary reports uncategorized memory,
> e.g.
> >      >
> >      >         -                   Unknown (reserved=1060416KB,
> >     committed=0KB)
> >      >                                       (mmap: reserved=1060416KB,
> >      >         committed=0KB)
> >      >
> >      >
> >      >
> >      >
> >      >         Bug: https://bugs.openjdk.java.net/browse/JDK-8248426
> >      >         Webrev:
> >     http://cr.openjdk.java.net/~zgu/JDK-8248426/webrev.00/
> >      >
> >      >         Test:
> >      >             hotspot_nmt
> >      >             Submit test in progress
> >      >
> >      >
> >      >         Thanks,
> >      >
> >      >         -Zhengyu
> >      >
> >
>
>