On Fri, 26 Jul 2024 06:08:03 GMT, Thomas Stuefe <stu...@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 > > 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. This code should not have been there. I forgot to remove it. There is no use of `total` here. > 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. Arena::tag_count() is declared as a constexpr. I wanted to avoid writing `static_cast<int>(Arena::Tag::tag_count)` every time I need tag_count, so I wrapped it in Arena::tag_count() and declared it with constexpr. Is that not sufficient to make it a constexpr? > 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? What do you mean by x macro? Do you have an example that shows the use of x macro? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1693443814 PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1693443227 PR Review Comment: https://git.openjdk.org/jdk/pull/20304#discussion_r1693445269
