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

[Zhengyu Gu]

Hi Thomas,

Thanks for reviewing.

On 11/25/19 3:54 PM, Thomas Stüfe wrote:
> Hi Zhengyu,
> 
> Hi Zhengyu,
> 
> http://cr.openjdk.java.net/~zgu/JDK-8234270/webrev.00/src/hotspot/share/memory/arena.cpp.udiff.html
> 
> not sure I understand this change - why is it needed?:
> 
> @@ -360,11 +360,12 @@
>     }
>     if (k) k->set_next(_chunk);   // Append new chunk to end of linked list
>     else _first = _chunk;
>     _hwm  = _chunk->bottom();     // Save the cached hwm, max
>     _max =  _chunk->top();
> -  set_size_in_bytes(size_in_bytes() + len);
> +  size_t new_size = size_in_bytes() + _chunk->length();
> +  set_size_in_bytes(new_size);
>     void* result = _hwm;
>     _hwm += x;
>     return result;
>   }

Right, it is not needed. Removed.

> 
> http://cr.openjdk.java.net/~zgu/JDK-8234270/webrev.00/src/hotspot/share/services/mallocTracker.hpp.udiff.html
> 
> -  inline void resize(long sz) {
> +  inline void resize(ssize_t sz) {
>       if (sz != 0) {
> +      assert(sz >= 0 || _size >= size_t(-sz), "Must be");
>         Atomic::add(size_t(sz), &_size);
>         DEBUG_ONLY(_peak_size = MAX2(_size, _peak_size);)
>       }
>     }
> 
> assert looks fine but the Atomic::add() took me by surprise: when size 
> is reduced, we feed it knowingly a negative number we then to unsigned 
> and rely on the overflow? Just a nit, but would Atomic::sub() not be 
> clearer here?

Because Atomic::sub() is implemented with Atomic::add()

http://hg.openjdk.java.net/jdk/jdk/file/f34ad283fcd6/src/hotspot/share/runtime/atomic.hpp#l560


Atomic::sub(-sz, &_size) => Atomic::add(-(-sz), &_size) => 
Atomic::add(size_t(sz), &_size)

> 
> http://cr.openjdk.java.net/~zgu/JDK-8234270/webrev.00/test/hotspot/jtreg/runtime/NMT/HugeArenaTracking.java.html
> 
> The test is fine as it is. Some thoughts (feel free to ignore them):
> 
> - would be more interesting to shake the boat a little by varying 
> increase rate, e.g.:
> 
>    60     // Allocate 2GB+ from arena
>    61     long total = 0;
>    62     while (total < 2 * GB) {
>             long increase = <random_number_between_some_bytes_and_I dont 
> know, 100M? Capped at 2 GB>
>    63       wb.NMTArenaMalloc(arena1, increase);
>    64       total += increase;
>    65     }
> 
> and maybe test jcmd VM.native_memory before this point? I know its 
> annoying since the numbers are probably not exact, and parsing would be 
> necessary. Just an idea.

Right, we certainly can.

Updated webrev: http://cr.openjdk.java.net/~zgu/JDK-8234270/webrev.01/

New patch also passed submit test:
[Mach5] mach5-one-zgu-JDK-8234270-2-20191126-0012-6997789: PASSED


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