[GR-75880] Support Native Image JFR emergency dumps on Windows#13607
[GR-75880] Support Native Image JFR emergency dumps on Windows#13607graalvmbot wants to merge 11 commits into
Conversation
oracle-contributor-agreement
Bot
added
the
OCA Verified
| This section outlines the JFR features that are available in Native Image. | ||
|
|
||
| On Windows, Native Image supports local JFR recordings written to `.jfr` files. JFR emergency dumps on out-of-memory, remote JMX access to `FlightRecorderMXBean`, and JFR control through `jcmd` are not currently available on Windows. | ||
| On Windows, Native Image supports local JFR recordings written to `.jfr` files and JFR emergency dumps on out-of-memory. Remote JMX access to `FlightRecorderMXBean` and JFR control through `jcmd` are not currently available on Windows. |
There was a problem hiding this comment.
I think the recurring callback sampler will work, but what about the SIGPROF one? That probably relies on some POSIX specific code. Maybe it would be good to include a note about the JFR method sampling here too.
There was a problem hiding this comment.
Added a note here: the signal-handler-based sampler uses POSIX SIGPROF support and is not available on Windows; Windows uses the default recurring-callback sampler for JFR method profiling.
| protected abstract void setPid(String pid); | ||
|
|
||
| protected abstract void setSavedCwdText(String cwd); | ||
|
|
||
| protected abstract void setDumpPathText(String dumpPath); | ||
|
|
||
| protected abstract void setDumpPathToSavedCwd(); | ||
|
|
||
| protected abstract void setRepositoryLocationText(String repositoryLocation); |
There was a problem hiding this comment.
These names are a bit confusing/inconsistent: setPid, setSavedCwdText, setDumpPathText, setDumpPathToSavedCwd, setRepositoryLocationText.
There are fields in AbstractJfrEmergencyDumpSupport with the names pidText, cwdText, dumpPathText, but they don't correspond to the methods above. Instead, for example, setSavedCwdText corresponds to the byte[] char[] cached in the platform-dependent classes.
I suggest renaming to setSavedPid, setSavedCwd, setSavedDumpPath, setSavedDumpPathToSavedCwd, setSavedRepositoryLocation. What do you think?
There was a problem hiding this comment.
Agreed, renamed these hooks to make it clearer that they cache the Java strings in the platform-specific representation: savePidText, saveCwdText, saveDumpPathText, useSavedCwdAsDumpPath, and saveRepositoryLocationText.
| idx = writeDumpFilePrefix(idx); | ||
| idx = appendPidToPathBuffer(idx); | ||
| idx = writeChunkFileExtension(idx); |
There was a problem hiding this comment.
Is there a reason why some of these methods are named write* while others are named append*?
I think that we are appending in both cases.
There was a problem hiding this comment.
Renamed these for consistency: appendDumpFilePrefixToPathBuffer, appendPidTextToPathBuffer, and appendChunkFileExtensionToPathBuffer now all use append...ToPathBuffer naming.
| return createEmergencyChunkPath(); | ||
| } | ||
|
|
||
| protected final RawFileDescriptor createEmergencyChunkPath() { |
There was a problem hiding this comment.
Why must this be protected?
There was a problem hiding this comment.
It does not need to be protected. Made createEmergencyChunkPath() private.
| return createEmergencyChunkPath(pathBuffer(), idx); | ||
| } | ||
|
|
||
| protected final RawFileDescriptor createEmergencyChunkPath(RawFilePath path, int baseNameEndIndex) { |
There was a problem hiding this comment.
Why must this be protected? Since this only has one caller, why not inline this into createEmergencyChunkPath()?
There was a problem hiding this comment.
Done. Removed the second overload and inlined its logic into the now-private createEmergencyChunkPath().
| protected static final int DUMP_FILE_PREFIX_LEN = 12; | ||
| protected static final int EMERGENCY_CHUNK_CREATE_ATTEMPTS = 100; | ||
| protected static final String DUMP_FILE_PREFIX = "svm_oom_pid_"; | ||
| protected static final String CHUNKFILE_EXTENSION = ".jfr"; |
There was a problem hiding this comment.
Is this no longer used?
There was a problem hiding this comment.
Yes, these string constants became unused. Removed them; only the length constants and allocation-free char helpers remain for the emergency dump path.
roberttoyonaga
left a comment
roberttoyonaga
left a comment
There was a problem hiding this comment.
Thank you! This looks good to me.
Rebase the GR-75880 changes onto master after the dependent Windows JFR and heap dump support landed.
4 participants
Adds Native Image support for JFR emergency dumps on Windows.
Native Image already supports writing regular JFR recordings on Windows, but out-of-memory emergency dumps were still unavailable because the emergency-dump implementation was tied to POSIX directory and file handling. This PR makes emergency dumping an OS-independent feature with small platform-specific adapters.
Implementation overview:
AbstractJfrEmergencyDumpSupportfor the common emergency-dump flow: dump path handling, repository fallback, emergency chunk creation, chunk filename validation/sorting, dump file assembly, and lifecycle test hooks.PosixJfrEmergencyDumpSupportto use the shared base while keeping POSIX-specific repository scanning,openat-based file opening, and path-buffer handling local to the POSIX implementation.WindowsJfrEmergencyDumpSupport, using Win32 APIs for repository enumeration, file opening, file attributes, system time, path conversion, and cleanup.Reviewer notes:
emergency_chunk.jfr, so repeated fallback chunk creation does not depend on replacing the same file.graal-enterprisePR updates the JDK checksum metadata required by the new Windows support.