On Tue, 25 Feb 2025 05:57:28 GMT, Ashutosh Mehra <asme...@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
