RFR 8234270: [REDO] JDK-8204128 NMT might report incorrect numbers for Compiler area

[Zhengyu Gu]

Hi Thomas,

> 
>     Right, we certainly can.
> 
>     Updated webrev: http://cr.openjdk.java.net/~zgu/JDK-8234270/webrev.01/
> 
> 
> output.shouldContain("Test (reserved=2GB, committed=2GB)");
> 
> Does this work? Does the output round sufficiently to always show "2G" 
> even though the total can jitter by +- 10M?

NMT rounds it to nearest whole number, so 2GB +/- 10M won't make a dent.

> 
>     New patch also passed submit test:
>     [Mach5] mach5-one-zgu-JDK-8234270-2-20191126-0012-6997789: PASSED
> 
> 
> Some more remarks and questions:
> 
> http://cr.openjdk.java.net/~zgu/JDK-8234270/webrev.01/src/hotspot/share/memory/resourceArea.cpp.udiff.html
> @@ -31,10 +31,13 @@
> 
>   void ResourceArea::bias_to(MEMFLAGS new_flags) {
>     if (new_flags != _flags) {
>       MemTracker::record_arena_free(_flags);
>       MemTracker::record_new_arena(new_flags);
> +    size_t size = size_in_bytes();
> +    MemTracker::record_arena_size_change(-ssize_t(size), _flags);  (A)
> +    MemTracker::record_arena_size_change(ssize_t(size), new_flags);
>       _flags = new_flags;
>     }
> 
> Just aesthetics, but coding would be easier to understand if you 
> reordered things:
> 
>     if (new_flags != _flags) {
> +    size_t size = size_in_bytes();
> +    MemTracker::record_arena_size_change(-ssize_t(size), _flags);  (A)
>       MemTracker::record_arena_free(_flags);
>       MemTracker::record_new_arena(new_flags);
> +    MemTracker::record_arena_size_change(ssize_t(size), new_flags);
>       _flags = new_flags;
>     }
> 
> or, if you were extending record_arena_free/record_new_arena to take 
> last/initial arena size too and pass that on to 
> MallocMemory::deallocate()/allocate().
> 
> But I leave it up tp you if you change this. If you just reorder the 
> calls, I do not need another Webrev,

I will re-order the calls, as you suggested.

Thanks,

-Zhengyu

> 
> ...
> 
> Another unrelated question, what is the reason for the unusual creation 
> of MallocMemorySummarySnapshot with placement new? Why not just put it 
> as a member into MallocMemorySummary? I must be missing something.
> 
> 
>     -Zhengyu
> 
> 
> 
>      >
>      >    66     wb.NMTFreeArena(arena1);
>      >
>      > On Mon, Nov 25, 2019 at 2:30 PM Zhengyu Gu <zgu at redhat.com
>     <mailto:zgu at redhat.com>
>      > <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>> wrote:
>      >
>      >     Ping ... May I get a second review?
>      >
>      >     Thanks,
>      >
>      >     -Zhengyu
>      >
>      >     On 11/21/19 12:12 PM, yumin qi wrote:
>      >      > Hi, Zhengyu
>      >      >
>      >      >    The fix looks good to me.
>      >      >
>      >      > Thanks
>      >      > Yumin
>      >      >
>      >      >
>      >      >
>      >      > On Wed, Nov 20, 2019 at 5:49 AM Zhengyu Gu <zgu at redhat.com
>     <mailto:zgu at redhat.com>
>      >     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>
>      >      > <mailto:zgu at redhat.com <mailto:zgu at redhat.com>
>     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>>> wrote:
>      >      >
>      >      >     JDK-8204128 did not fix the original bug. But new
>     assertion
>      >     helped to
>      >      >     catch the problem, as it consistently failed in Oracle
>      >     internal tests.
>      >      >
>      >      >     The root cause is that, when NMT biases a resource area to
>      >     compiler, it
>      >      >     did not adjust tracking data to reflect that. When the
>     biased
>      >     resource
>      >      >     area is released, there is a possibility that its size is
>      >     greater than
>      >      >     total size recorded, and underflow a size_t counter.
>      >      >
>      >      >     JDK-8204128 patch also missed a long to ssize_t parameter
>      >     type change,
>      >      >     that resulted new test failure on Windows, because long is
>      >     4-bytes on
>      >      >     Windows.
>      >      >
>      >      >     Many thanks to Leonid Mesnik, who helped to run this patch
>      >     through
>      >      >     Oracle's internal stress tests.
>      >      >
>      >      >     Bug: https://bugs.openjdk.java.net/browse/JDK-8234270
>      >      >     Webrev:
>      > http://cr.openjdk.java.net/~zgu/JDK-8234270/webrev.00/index.html
>      >      >
>      >      >
>      >      >     Test:
>      >      >         hotspot_nmt
>      >      >         Submit test
>      >      >         Oracle internal stress tests.
>      >      >
>      >      >
>      >      >     Thanks,
>      >      >
>      >      >     -Zhengyu
>      >      >
>      >
>