RFR: 8315362: NMT: summary diff reports threads count incorrectly [v7]

[Thomas Stuefe]

On Fri, 13 Oct 2023 09:46:59 GMT, Evgeny Ignatenko <duke at openjdk.org> wrote:

>> 8315362: NMT: summary diff reports threads count incorrectly
>
> Evgeny Ignatenko has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into pr/8315362
>  - Move thread count to MemBaseline
>  - Not check exact amount of added threads
>  - Remove bootclasspath from test
>  - Change thread_count var type to size_t
>  - Review comments
>  - Resolve conflicts
>  - 8315362: NMT: summary diff reports threads count incorrectly

Neat. Looks mostly good, thanks for going with my suggestions.

You need to get the GHA problem figured out. We need to see test runs. Also, I guess this will not be your only change. I'm not sure what is wrong on your side tbh. If you have enabled them, I guess one thing you could do is to close this and open a new PR and see if that helps.

src/hotspot/share/services/threadStackTracker.cpp line 55:

> 53:     ThreadCritical tc;
> 54:     VirtualMemoryTracker::add_reserved_region((address)base, size, stack, mtThreadStack);
> 55:     _thread_count++;

Move this out of the loop?

src/hotspot/share/services/threadStackTracker.cpp line 85:

> 83:       bool removed = _simple_thread_stacks->remove(site);
> 84:       assert(removed, "Must exist");
> 85:       _thread_count--;

Move out of loop

test/hotspot/jtreg/runtime/NMT/SummaryDiffThreadCount.java line 61:

> 59:         pb.command(new String[] { JDKToolFinder.getJDKTool("jcmd"), pid, "VM.native_memory", "summary.diff"});
> 60:         output = new OutputAnalyzer(pb.start());
> 61:         output.shouldMatch("threads #\\d+ \\+");

So the trailing + is the trick, right? Add a little comment above? Also, just to be sure, execute the test with the pre-patched version to check that it correctly fails.

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

PR Review: https://git.openjdk.org/jdk/pull/15490#pullrequestreview-1676134032
PR Review Comment: https://git.openjdk.org/jdk/pull/15490#discussion_r1358042177
PR Review Comment: https://git.openjdk.org/jdk/pull/15490#discussion_r1358042444
PR Review Comment: https://git.openjdk.org/jdk/pull/15490#discussion_r1358044071