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

[Zhengyu Gu]

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

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