RFR: JDK-8320061: [nmt] Multiple issues with peak accounting [v2]

[Thomas Stuefe]

On Fri, 24 Nov 2023 15:27:55 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

> The HotSpot code looks good generally. The test utilities are nice to see.

Hi Johan,

thanks for taking a look!

> 
> I've looked through the test changes, but it seems that some change what the test does while some simply refactor the tests to use the new utilities. It would have been nice to have those in separate commtis to make reviewing easier. What is the idea behind changing the tests?

The tests needed to be adapted, since they all expected entries to vanish when their current value is low. But that's exactly what this patch tries to fix: to preserve all the historical peaks. Because of that, a bunch of tests broke. 

I only touched tests that had been broken by the NMT change. I did not change other tests.

> src/hotspot/share/nmt/memReporter.cpp line 220:
> 
>> 218:   const size_t pk_arena = malloc_memory->arena_peak_size();
>> 219: 
>> 220:   if (amount_in_current_scale(MAX4(reserved_amount, pk_vm, pk_malloc, pk_arena)) > 0) {
> 
> This branch body is very long and there is no `else`, can we invert the condition and do an early return instead?

ok

> src/hotspot/share/nmt/memReporter.cpp line 336:
> 
>> 334:     // Omit printing if neither the current nor the historic peak malloc amount raises above
>> 335:     // reporting scale threshold.
>> 336:     if (amount_in_current_scale(MAX2(malloc_site->size(), malloc_site->peak_size())) == 0) {
> 
> This comment reads as if there are two peaks, one current and one historic. Just go for the simpler "Omit printing if the current value and the historic peak value both fall below the reporting scale threshold". Same with the other one.

Done

> src/hotspot/share/nmt/memReporter.cpp line 367:
> 
>> 365:     }
>> 366:     // Omit printing if neither the current nor the historic peak malloc amount raises above
>> 367:     // reporting scale threshold.
> 
> The comment is also wrong, this is not about malloc amount!

Copy paste error, thanks for catching. Corrected.

> test/hotspot/jtreg/runtime/NMT/HugeArenaTracking.java line 82:
> 
>> 80:     wb.NMTFreeArena(arena1);
>> 81: 
>> 82:     // Repeat report at GB level. Peak should be 0 now. Current usage is 1KB, since arena2 is left, but that
> 
> "Peak should be 0 now" <- that's gotta be incorrect.

Fixed

> test/hotspot/jtreg/runtime/NMT/MallocTestType.java line 52:
> 
>> 50:     wb.NMTFree(memAlloc3);                           // current +256K #1 peak +384K #2
>> 51:     long memAlloc1 = wb.NMTMalloc(512 * 1024);  // current +768K #2 peak +768K #2
>> 52:     wb.NMTFree(memAlloc2);                           // current +512K #1 peak +768K #2
> 
> Comment indent style

Done

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

PR Comment: https://git.openjdk.org/jdk/pull/16675#issuecomment-1825909664
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404523091
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404523750
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404524778
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404525346
PR Review Comment: https://git.openjdk.org/jdk/pull/16675#discussion_r1404526717