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

Thomas Stüfe thomas.stuefe at gmail.com
Tue Nov 26 08:43:23 UTC 2019


Hi Zhengyu,



On Tue, Nov 26, 2019 at 2:20 AM Zhengyu Gu <zgu at redhat.com> wrote:

> 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)
>
>
Okay.


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


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

...

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


More information about the hotspot-runtime-dev mailing list