RFR: 8332122: [nmt] Totals for malloc should show total peak

[Thomas Stuefe]

On Tue, 21 May 2024 16:27:20 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

> Hi all, 
> 
> This PR addresses [8332122](https://bugs.openjdk.org/browse/JDK-8332122). 
> 
> With this patch, we enable printing global malloc peaks in NMT when printing summary information. 
> 
> Testing: 
> - [x] Added test case passes. 
> 
> Thanks, 
> Sonia

Thanks for doing this. Mostly okay, small nits remain.

test/hotspot/jtreg/runtime/NMT/PeakMallocTest.java line 38:

> 36:  *
> 37:  */
> 38: 

Please write a comment somewhere below this explaining the -Xint (because we want to reduce background malloc noise) and the small heap.

(I am surprised 8MB is enough, btw. I would increase this to 32MB, just to be sure.)

test/hotspot/jtreg/runtime/NMT/PeakMallocTest.java line 50:

> 48: 
> 49:         // Measure early malloc total and peak
> 50:         OutputAnalyzer output = NMTTestUtils.startJcmdVMNativeMemory();

I would give the command the "scale=1" sub option, then you don't have to fiddle around later translating KB to B

test/hotspot/jtreg/runtime/NMT/PeakMallocTest.java line 55:

> 53:         System.out.println("Early malloc total: " + earlyTotal);
> 54:         System.out.println("Early malloc peak: " + earlyPeak);
> 55: 

Please delete one empty line

test/hotspot/jtreg/runtime/NMT/PeakMallocTest.java line 58:

> 56: 
> 57:         // Allocate a large amount of memory and then free
> 58:         long allocSize = Math.max(8 * earlyPeak, 1000000000); // MAX(earlyPeak * 8, 1GB)

Please use the correct value for GB (e.g. 1024 * 1024 * 1024).

One thing that just occurred to me was that in debug JVMs, we zap that malloc memory with a pattern. So, the whole 1GB are fully touched, which means this test would really need 1GB of space.

I would therefore reduce the allocated memory to, say, 250MB. That size should still be large enough to stick out of any background malloc noise.

test/hotspot/jtreg/runtime/NMT/PeakMallocTest.java line 74:

> 72:         if ((currTotal < earlyTotal * 0.9) || (currTotal > earlyTotal * 1.1)) {
> 73:             throw new Exception("Global malloc measurement is incorrect");
> 74:         }

Can you please make the fudge factor a constant at the beginning of the class, and increase it to 20%? And I would change the error output to include current and expected value. Same below, when printing an error about the peak delta.

-------------

Marked as reviewed by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19333#pullrequestreview-2070096158
PR Review Comment: https://git.openjdk.org/jdk/pull/19333#discussion_r1609264759
PR Review Comment: https://git.openjdk.org/jdk/pull/19333#discussion_r1609262229
PR Review Comment: https://git.openjdk.org/jdk/pull/19333#discussion_r1609259328
PR Review Comment: https://git.openjdk.org/jdk/pull/19333#discussion_r1609261066
PR Review Comment: https://git.openjdk.org/jdk/pull/19333#discussion_r1609258937