[crac] RFR: 8350088: [CRaC] Stop & restart JFR recording on checkpoint [v4]

[Timofei Pushkin]

On Fri, 14 Feb 2025 17:21:17 GMT, Radim Vansa <rvansa at openjdk.org> wrote:

>> All JFR recordings are closed during checkpoint. After restore JDK starts new recordings with the same parameters as those running before checkpoint. The recordings dumped before checkpoint would be overwritten (as this is a new recording it won't append to the old one).
>> If `-XX:StartFlightRecorder=...` is passed on restore a new recording is started.
>
> Radim Vansa has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix module info

src/hotspot/share/jfr/jfr.cpp line 179:

> 177:     // trying to register new file descriptors. Instead we just record a request and
> 178:     // the recording will be started at the right moment from JDKResource.
> 179:     JfrUpcalls::request_start_after_restore(JavaThread::current());

If `request_start_after_restore` remains trapping (see my other comment in its body), `Jfr::after_restore` should also be marked as such so that the caller would do proper exception handling.

src/hotspot/share/jfr/jni/jfrUpcalls.cpp line 230:

> 228:     CLEAR_PENDING_EXCEPTION;
> 229:     ResourceMark rm(THREAD);
> 230:     log_error(jfr, system)("JfrUpcall failed for %s", unhide_internal_types_sym->as_C_string());

Can't we propagate the exception? If not, the method should probably not be marked as `TRAPS` and the `CHECK` above should be transformed to this exception clearing as well?

src/hotspot/share/jfr/jni/jfrUpcalls.cpp line 231:

> 229:     ResourceMark rm(THREAD);
> 230:     log_error(jfr, system)("JfrUpcall failed for %s", unhide_internal_types_sym->as_C_string());
> 231:     return;

This `return` is redundant

src/hotspot/share/jfr/recorder/jfrRecorder.cpp line 470:

> 468:   JavaThread* const thread = JavaThread::current();
> 469:   validate_recording_options(thread);
> 470:   launch_command_line_recordings(thread);

These two methods are marked as trapping but I think they should never actually propagate an exception... Maybe we could at least put an `assert(!HAS_PENDING_EXCEPTION)` here (would probably also require renaming `thread` to `THREAD`)?

src/java.base/share/classes/jdk/internal/crac/Core.java line 35:

> 33: public class Core {
> 34:     private static ClaimedFDs claimedFDs;
> 35:     private static JfrResource jfrResource = new JfrResource();

Suggestion:

    private static final JfrResource jfrResource = new JfrResource();

src/java.base/share/classes/jdk/internal/crac/JfrResource.java line 1:

> 1: package jdk.internal.crac;

Shouldn't there be a license header here?

src/java.base/share/classes/jdk/internal/crac/JfrResource.java line 25:

> 23: 
> 24:     public void setStartFlightRecorder(Runnable runnable) {
> 25:         this.startRecording = runnable;

Suggestion:

        startRecording = runnable;

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

PR Review Comment: https://git.openjdk.org/crac/pull/203#discussion_r1959331784
PR Review Comment: https://git.openjdk.org/crac/pull/203#discussion_r1959318389
PR Review Comment: https://git.openjdk.org/crac/pull/203#discussion_r1959318923
PR Review Comment: https://git.openjdk.org/crac/pull/203#discussion_r1959398189
PR Review Comment: https://git.openjdk.org/crac/pull/203#discussion_r1959358569
PR Review Comment: https://git.openjdk.org/crac/pull/203#discussion_r1959349907
PR Review Comment: https://git.openjdk.org/crac/pull/203#discussion_r1959350519