RFR: 8337789: JEP 509: JFR CPU-Time Profiling (Experimental) [v47]

[Johannes Bechberger]

On Mon, 12 May 2025 17:58:01 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Simplify local trace stack
>
> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 112:
> 
>> 110:   }
>> 111: 
>> 112:   if (jt->is_JfrSampler_thread()) {
> 
> Sampler thread is a non-JavaThread so we shouldn’t get here.

Good catch.

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 369:
> 
>> 367:         if (count < 500) {
>> 368:           Atomic::release_store(&e->_state, state_empty(head));
>> 369:           Atomic::dec(&_size);
> 
> Could you explain this scenario in detail? If we failed the previous branches then we are in a different generation but then I don’t see why doing this is correct.

It should override the current event. But we possibly don't need it anyway.

> src/hotspot/share/jfr/periodic/sampling/jfrCPUTimeThreadSampler.cpp line 630:
> 
>> 628:  // assert(!stack.is_empty(), "invariant");
>> 629:   for (u4 i = 0; i < stack.size(); i++) {
>> 630:     JfrCPUTimeThreadSampler::record_event(thread, stack.at(i), tmp_stackframes);
> 
> Shouldn’t we return the trace to the _frame_store as with on_safepoint()?

Good catch.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2091192272
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2091189118
PR Review Comment: https://git.openjdk.org/jdk/pull/20752#discussion_r2091185865