RFR: 8344009: Improve compiler memory statistics [v4]

[Thomas Stuefe]

On Tue, 25 Feb 2025 05:57:28 GMT, Ashutosh Mehra <asmehra at openjdk.org> wrote:

>> Thomas Stuefe has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision:
>> 
>>   avoid Thread::current in high traffic chunk alloc path
>
> src/hotspot/share/compiler/compilationMemStatInternals.hpp line 92:
> 
>> 90: 
>> 91: // A very simple fixed-width FIFO buffer, used for the phase timeline
>> 92: template <typename T, int max>
> 
> Would `size` be a better name than `max`?

ok changed

> src/hotspot/share/compiler/compilationMemStatInternals.hpp line 160:
> 
>> 158:     void init(T v)        { start = cur = peak = v; }
>> 159:     void update(T v)      { cur = v; if (v > peak) peak = v; }
>> 160:     dT end_delta() const  { return (dT)cur - (dT)start; }
> 
> Should it be `return (dT)(cur - start); }`

hmm, I like to avoid the inner overflow is cur < start (if the phase released more memory than it allocated)

> src/hotspot/share/compiler/compilationMemStatInternals.hpp line 161:
> 
>> 159:     void update(T v)      { cur = v; if (v > peak) peak = v; }
>> 160:     dT end_delta() const  { return (dT)cur - (dT)start; }
>> 161:     size_t temporary_peak_size() const { return MIN2(peak - cur, peak - start); }
> 
> shouldn't it be `MAX2(peak - cur, peak - start)`? Why not just `peak - start`?

We are only interested in a rise that rose significantly above **both** the start and end point of the measurements. 

E.g.:
- if we have this: start = 0, end = 20MB, peak = 20MB, this is not a temporary peak and we already know that the end usage is 20MB.
- if we have this: start = 20MB, end = 0, peak = 20MB, this is not a temporary peak either, because we already know the starting footprint was 20MB.
- but if we have start = 0, end = 0, peak = 20MB, this is interesting since if we just print start and end we will miss the fact that in between those times we had temporarily allocated 20MB.

> src/hotspot/share/compiler/compilationMemoryStatistic.cpp line 149:
> 
>> 147:       int col = start_indent;
>> 148:       check_phase_trace_id(e.info.id);
>> 149:       if (omit_empty_phases && e._bytes.end_delta() == 0 && e._bytes.temporary_peak_size() == 0) {
> 
> `omit_empty_phases` is always false. Can it be just removed?

I hesitated to throw this out because the timeline can get very wordy; but I got used to the more expressive timeline with the 0 entries, so okay.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1970105585
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1970105238
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1970103141
PR Review Comment: https://git.openjdk.org/jdk/pull/23530#discussion_r1970110006