On Tue, 23 Jul 2024 21:46:50 GMT, Ashutosh Mehra <asme...@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
