Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GR-56601] Initial support for jcmd. #9963

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

graalvmbot
Copy link
Collaborator

@graalvmbot graalvmbot commented Oct 24, 2024

This PR is based on #9232 (there is not much code left from the original PR though) and contains the following changes:

  • Added jcmd support on Linux and macOS (see JCmdFeature).
    • the jcmd support builds on top of the attach API (see PosixAttachApiSupport and the files in com.oracle.svm.core.attach and com.oracle.svm.core.posix.attach)
    • the attach API uses SIGQUIT/SIGBREAK and domain sockets for communication
    • Native Image announces via a JVM performance counter that supports the attach API (see changes in com.oracle.svm.core.jvmstat.SystemCounters). jcmd can access the performance counters to detect which process are running.
    • Native Image already partially supported SIGQUIT/SIGBREAK. This PR extends the existing feature and renames it from DumpRuntimeCompilationOnSignalFeature to SigQuitFeature.
  • Implemented 15 diagnostic commands (see com.oracle.svm.core.dcmd.*) that can be triggered via jcmd
  • Various JFR changes and simplifications so that -XX:StartFlightRecording and jcmd <pid> JFR.start use the same code and have the same behavior.
  • Minor JFR fixes for JVMSupport.makeFilename and the JFR startup.
  • Minor changes to the heap dumping infrastructure so that we support everything that is needed for jcmd <pid> GC.heap_dump dump.hprof.
  • A few Posix infrastructure changes to get rid of some code duplication (primarily related to write operations on files/sockets).

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 24, 2024
Copy link
Collaborator

@roberttoyonaga roberttoyonaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for doing this PR @christianhaeubl! This looks really good to me.


@Override
public List<Class<? extends Feature>> getRequiredFeatures() {
return List.of(SigQuitFeature.class, AttachApiFeature.class);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the DCmdFeature also be included in this list?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it is indeed cleaner if DCmdFeature is included here as well.

return String.join(System.lineSeparator(), lines);
}

private static String[] getHelp(Target_jdk_jfr_internal_dcmd_AbstractDCmd cmd) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JFR.start DCMD from the JDK shows extended JFR configuration options (that aren't supported) as part of it's help text. It's not a big problem since you return a descriptive error message Warning! The .jfc option/setting 'xyz' doesn't exist or is not supported. The other option would be to print custom help text, copy-pasting the parts we support in Native Image. However, maybe that's not really worth the duplication and maintenance of keeping it up to date.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, there is a bit too much output. Most extended configuration options are already hidden due to the substitution of Target_jdk_jfr_internal_dcmd_DCmdStart.jfcOptions(). I don't think that it is worth to duplicate the help text, so I think we will just live with the extra output for now.

fniephaus
fniephaus previously approved these changes Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants