RFR: 8337031: Improvements to CompilationMemoryStatistic

[Thomas Stuefe]

On Tue, 23 Jul 2024 21:46:50 GMT, Ashutosh Mehra <asmehra at openjdk.org> wrote:

> Some minor improvements to CompilationMemoryStatistic. More details are in [JDK-8337031](https://bugs.openjdk.org/browse/JDK-8337031)
> 
> Testing:
>   test/hotspot/jtreg/compiler/print/CompileCommandPrintMemStat.java
>   test/hotspot/jtreg/serviceability/dcmd/compiler/CompilerMemoryStatisticTest.java

Hi Ashu,

generally okay, see remarks.

It seems awkward to have a size_t vector with a defined length, and then having to specify the length as input argument. I'd consider either use the good old style of 


void foo(const size_t array[NUM], ...);


(using array with a defined size, but careful since in foo sizeof(array) is still just a pointer)

or just write a small wrapper class holding a size_t vector and taking care of the copying.

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 58:

> 56: }
> 57: 
> 58: void ArenaStatCounter::init() {

Proposal: `reset()` ?

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 115:

> 113:   for (int tag = 0; tag < Arena::tag_count(); tag++) {
> 114:     total += _tags_size[tag];
> 115:   }

Do it with x-macro?

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 118:

> 116:   if (total != _current) {
> 117:     log_info(compilation, alloc)("WARNING!!! Total does not match current");
> 118:   }

Why do we calculate total? Just for this test? I would then put this into an ASSERT section, and make the info log an assert. 

However, I wonder if this is really needed. The logic updating both _current and _tags_size is pretty straightforward, I don't see how there could be a mismatch.

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 204:

> 202:   size_t _total;
> 203:   // usage per arena tag when total peaked
> 204:   size_t _tags_size_at_peak[Arena::tag_count()];

Can you please make sure Arena::tag_count() evaluates to constexpr? When in doubt, just use the enum value instead.

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 226:

> 224: 
> 225:   void set_total(size_t n) { _total = n; }
> 226:   void set_tags_size_at_peak(size_t* tags_size_at_peak, int nelements) {

const size_t*

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 228:

> 226:   void set_tags_size_at_peak(size_t* tags_size_at_peak, int nelements) {
> 227:     assert(nelements*sizeof(size_t) <= sizeof(_tags_size_at_peak), "overflow check");
> 228:     memcpy(_tags_size_at_peak, tags_size_at_peak, nelements*sizeof(size_t));

style, we do blanks between * (n * x, not n*x)

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 242:

> 240:     for (int tag = 0; tag < Arena::tag_count(); tag++) {
> 241:       st->print_cr("  " LEGEND_KEY_FMT ": %s", Arena::tag_name[tag], Arena::tag_desc[tag]);
> 242:     }

use x macro?

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 365:

> 363: 
> 364:   void add(const FullMethodName& fmn, CompilerType comptype,
> 365:            size_t total, size_t* tags_size_at_peak, int nelements,

const size_t*

src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 471:

> 469:     _the_table->add(fmn, ct,
> 470:                     arena_stat->peak(), // total
> 471:                     (size_t *)arena_stat->tags_size_at_peak(),

Cast should not be needed

src/hotspot/share/compiler/compilationMemoryStatistic.hpp line 46:

> 44:   size_t _peak;
> 45:   // Current bytes used by arenas per tag
> 46:   size_t _tags_size[Arena::tag_count()];

Proposal: `_current_by_tag`, referring to _current

src/hotspot/share/compiler/compilationMemoryStatistic.hpp line 53:

> 51: 
> 52:   // Peak composition:
> 53:   size_t _tags_size_at_peak[Arena::tag_count()];

`_peak_by_tag` ?

src/hotspot/share/memory/arena.cpp line 48:

> 46: 
> 47: #define ARENA_TAG_STRING_(str) #str
> 48: #define ARENA_TAG_STRING(name, str, desc) ARENA_TAG_STRING_(str),

Can you use STR/XSTR in macros.hpp?

src/hotspot/share/memory/arena.hpp line 86:

> 84: };
> 85: 
> 86: #define DO_ARENA_TAG(template) \

Please don't name this template, confuses my IDE. We usually call it DO or XX or something like that

src/hotspot/share/memory/arena.hpp line 97:

> 95: 
> 96: #define ARENA_TAG_ENUM_(name) tag_##name
> 97: #define ARENA_TAG_ENUM(name, str, desc) ARENA_TAG_ENUM_(name),

Here, and in other places: Please try to cut down the number of temp. macros. You can just as well do a 

enum class Tag {
#define XX(name, whatever, whatever2) tag_##name
DO_ARENA_TAG(XX)
#undef XX
  num_tags 
};


here.

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

PR Review: https://git.openjdk.org/jdk/pull/20304#pullrequestreview-2201007416
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692556908
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692554736
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692556339
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692574321
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692574925
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692577085
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692577477
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692578328
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692579046
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692557726
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692557957
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692559750
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692561100
PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1692564129