RFR: 8333994: NMT: call stacks should show source information [v3]

[Gerard Ziemski]

On Thu, 20 Jun 2024 09:41:48 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Patch adds printing of source file and line number to NMT call stacks that are printed in detail mode. Useful for hunting down leaks.
>
> Thomas Stuefe 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:
> 
>  - fix windows build
>  - Merge branch 'master' into JDK-8333994-NMT-call-stacks-should-show-source-information
>  - caching
>  - Merge branch 'master' into JDK-8333994-NMT-call-stacks-should-show-source-information
>  - exclude macos from testing source info
>  - copyrights
>  - test
>  - JDK-8333994-NMT-call-stacks-should-show-source-information

Looks good overall, I do have a few small questions/suggestions.

src/hotspot/share/nmt/memReporter.hpp line 157:

> 155:   MemDetailReporter(MemBaseline& baseline, outputStream* output, size_t scale = default_scale) :
> 156:     MemSummaryReporter(baseline, output, scale),
> 157:      _baseline(baseline), _stackprinter(output) { }

`NativeCallStackPrinter` is 293* 1024 == 293KB

Isn't there a way to create/destroy it on the fly? NMT is already memory hungry, we just keep increasing the price of admission ...

src/hotspot/share/utilities/nativeCallStack.cpp line 130:

> 128:       // cached?
> 129:       bool created = false;
> 130:       Entry* const cached_value = _cache.put_if_absent(pc, &created);

`cached_value` sounds generic, could we have `cached_stream_text` or something more descriptive?

src/hotspot/share/utilities/nativeCallStack.cpp line 137:

> 135:         stack->print_frame(&ss, pc);
> 136:         _out->print_raw_cr(cached_value->text);
> 137:       }

We could simplify to:


      if (created) {
        stringStream ss(cached_value->text, sizeof(cached_value->text));
        stack->print_frame(&ss, pc);
      }
      _out->print_raw_cr(cached_value->text);

src/hotspot/share/utilities/nativeCallStack.hpp line 136:

> 134: // a NativeCallStackPrinter improves performance by caching printed frames by address.
> 135: class NativeCallStackPrinter {
> 136:   struct Entry { char text[1024]; };

1024 is big enough? Would 512 suffice?

Not that important if we were creating/destroying it as needed, but right now it is always there...

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

Changes requested by gziemski (Committer).

PR Review: https://git.openjdk.org/jdk/pull/19655#pullrequestreview-2133388645
PR Review Comment: https://git.openjdk.org/jdk/pull/19655#discussion_r1649414362
PR Review Comment: https://git.openjdk.org/jdk/pull/19655#discussion_r1649418281
PR Review Comment: https://git.openjdk.org/jdk/pull/19655#discussion_r1649409549
PR Review Comment: https://git.openjdk.org/jdk/pull/19655#discussion_r1649416879