8373441: Remove DCmdFactory::_enabled

[iklam]

The DCmdFactory::_enabled is always set to true and there doesn't seem to be a reason to set it to false.

This PR removes this field and simplified the creation of DCmdFactory objects.

The related _hidden field is also currently not used, but may be used in the future when deprecating DCmds, so we leave it unchanged.

Note that now jmm_GetDiagnosticCommandInfo() always set dcmdInfo::enabled to true to be compatible with Java code.

---

Progress

• [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• [x] Change must not contain extraneous whitespace
• [x] Commit message must refer to an issue

Issue

• JDK-8373441: Remove DCmdFactory::_enabled (Enhancement - P4)

Reviewers

• Johan Sjölen (@jdksjolen - Reviewer) 🔄 Re-review required (review applies to 8a047713)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28794/head:pull/28794
$ git checkout pull/28794

Update a local copy of the PR:
$ git checkout pull/28794
$ git pull https://git.openjdk.org/jdk.git pull/28794/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28794

View PR using the GUI difftool:
$ git pr show -t 28794

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28794.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk[bot]]

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8373441: Remove DCmdFactory::_enabled

Reviewed-by: kevinw, fparain, jsjolen

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 73 new commits pushed to the master branch:

• d02abfe765a1e67c5e37f3450aa5a0d8fb97a208: 8158801: [TEST_BUG] Mixing tests fail because of focus workaround trick
• 1e357e9e976bfb0abc9d4e14bfb1572693622af8: 8373623: Refactor Serialization tests for Records to JUnit
• 817e3dfde9eaa467ea0dca9b70282e914cdde093: 8350711: [JMH] test Signatures.RSASSAPSS failed for 2 threads config
• ... and 70 more: https://git.openjdk.org/jdk/compare/74dca863c2e61c13884c3454b8da7be125235970...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk[bot]]

@iklam The following labels will be automatically applied to this pull request:

• hotspot
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge[bot]]

Webrevs

• 01: Full - Incremental (98176aa7)
• 00: Full (8a047713)

[openjdk[bot]]

@iklam jmx has been added to this pull request based on files touched in new commit(s).

[iklam]

The Java API has "enabled" handled in src/jdk.management/share/classes/com/sun/management/internal/DiagnosticCommandInfo.java (and read in src/jdk.management/share/classes/com/sun/management/internal/DiagnosticCommandImpl.java)

In the public API docs we do specify a Descriptor for diagnostic commands:

https://docs.oracle.com/en/java/javase/25/docs/api/jdk.management/com/sun/management/DiagnosticCommandMBean.html dcmd.enabled boolean True if the diagnostic command is enabled, false otherwise

dcmd.enabled is already not very meaningful as it's always true, but if it becomes truly meaningless, it needs thought on whether it stays, maybe with a comment that it is always true, or can be removed.

The Java api deals with remote connections, so interactions between JDK versions need considering. If dcmd.enabled is not found, if a client app ever checks this Descriptor, does the app think it's not enabled?

Maybe it stays in the Descriptor for compatibility reasons, but can be removed from DiagnosticCommandInfo.

This could be a follow up issue if we aren't doing it now.

For compatibility reasons, I don't want to touch the Java APIs in this PR. I updated the comments in DiagnosticCommandMBean.java to say

 *     <th scope="row">dcmd.enabled</th><td>boolean</td>
 *     <td>This field is always true -- diagnostic commands cannot be disabled.</td>

[dholmes-ora]

If we are just changing the implementation then I guess this would be okay. But it seems the API for this allows for specific DCmds to be disabled (how?) - or at least intended for it to be possible. As per the doc:

When the set of diagnostic commands currently supported by the Java Virtual Machine is modified, the DiagnosticCommandMBean emits a Notification with a type of "jmx.mbean.info.changed" and a userData that is the new MBeanInfo.

[iklam]

If we are just changing the implementation then I guess this would be okay. But it seems the API for this allows for specific DCmds to be disabled (how?) - or at least intended for it to be possible. As per the doc:

When the set of diagnostic commands currently supported by the Java Virtual Machine is modified, the DiagnosticCommandMBean emits a Notification with a type of "jmx.mbean.info.changed" and a userData that is the new MBeanInfo.

The above paragraph is about new commands being added:

int DCmdFactory::register_DCmdFactory(DCmdFactory* factory) {
  MutexLocker ml(DCmdFactory_lock, Mutex::_no_safepoint_check_flag);
  factory->_next = _DCmdFactoryList;
  _DCmdFactoryList = factory;
  if (_send_jmx_notification && !factory->_hidden
      && (factory->_export_flags & DCmd_Source_MBean)) {
    DCmdFactory::push_jmx_notification_request();
  }
  return 0; // Actually, there's no checks for duplicates
}

Which will eventually cause DiagnosticCommandImpl::createDiagnosticFrameworkNotification() to be called by HotSpot to dispatch the Notification.

I checked older source code like JDK 8, which has a DCmdFactory::set_enabled() method but there are no callers. If there had been a way to disable commands dynamically, that has been lost for a very long time.

[dholmes-ora]

For the history see

https://bugs.openjdk.org/browse/JDK-7104647?focusedId=12336354&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12336354

The "enabled" capability came from the JRockit jrcmd tool:

https://docs.oracle.com/cd/E13150_01/jrockit_jvm/jrockit/geninfo/diagnos/ctrlbreakhndlr.html

"You can enable or disable any diagnostic command using the system property -Djrockit.ctrlbreak.enable=<true|false>, where name is the name of the diagnostic command."

But as far as I can see the DCmd framework never specified a mechanism for disabling a DCmd. @fparain may recall more details.

[fparain]

The "enabled" capability probably fall in the category of features that were initially implemented to emulate JRockit jrcmd tool but were finally not integrated in HotSpot (I remember another feature like that, to execute an user shell script when some events were triggered). I don't remember having used or debugged this enable/disable feature. It looks that dead code that have been here for too long. Thank you for having found it and cleaned it up.

[fparain]

More context: https://openjdk.org/jeps/137

1-1 Overview

The diagnostic command framework is fully implemented in native code and relies on HotSpot's internal exception mechanism. The rationale for a pure native implementation is to be able to execute diagnostic commands even in critical situations like an out-of-memory condition. All diagnostic commands are registered in a single list, and two flags control the way a user can interact with them. The hidden flag prevents a diagnostic command from appearing in the list of available commands returned by the help command. However, it's still possible to get the detailed help message for a hidden command with the help syntax, but it requires knowing the name of the hidden command. The second flag is enabled and it controls whether a command can be invoked or not. When listed with the help commands, disabled commands appear with a [disabled] label in their descriptions. If the user tries to invoke a disabled command, an error message is returned and the command is not run. This error message can be customized on a per-command basis. The framework just provides these two flags with their semantics; it doesn't provide any policy or mechanism to set or modify these flags. These actions will be delegated to the JVM or specific diagnostic commands.

The diagnostic command framework was developed at the same time we had requests to implement "Commercial features", the "enabled" flag might also have been intended to guard those commercial features (gone now).

[kevinjwalls]

Thanks @fparain Frederic for the extra context. Had wondered if this feature had ever been used, particularly all the mechanism around the possible JMX Notification. After this change, we may look at whether it should be removed also.

[iklam]

Thanks @kevinjwalls @fparain @kevinjwalls for the review /integrate

[openjdk[bot]]

Going to push as commit 3f07710270dbe7268f21828dff20e2eb810b1e70. Since your change was applied there have been 77 commits pushed to the master branch:

• 87d881fee01c42f5847031a63d50873b3d438f7a: 8368493: Disable most test JSSE debug output by default, and increase the test default maximum output log size
• 30be94086aad42b99a15a05fe5115f552e8efb8b: 8373625: CPUTimeCounters creates a total counter for unsupported GCs
• 2241218ef64ed6cb51f962f3ab6db1a766f1744f: 8373631: Improve classes in the "jdk.jpackage.internal.util.function" package
• ... and 74 more: https://git.openjdk.org/jdk/compare/74dca863c2e61c13884c3454b8da7be125235970...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@iklam Pushed as commit 3f07710270dbe7268f21828dff20e2eb810b1e70.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.