8278423: ExtendedDTraceProbes should be deprecated by eme64 · Pull Request #7110 · openjdk/jdk

eme64 · 2022-01-17T13:08:17Z

Deprecated ExtendedDTraceProbes.
Edited help messages and man pages accordingly, added the 3 flags to man pages.
Added flag to VMDeprecatedOptions test.
Replaced the flag with 3 flags in SDTProbesGNULinuxTest.java.

Checked that tests are not affected.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-8278423: ExtendedDTraceProbes should be deprecated
JDK-8279629: ExtendedDTraceProbes should be deprecated (CSR)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Harold Seigel (@hseigel - Reviewer) ⚠️ Review applies to b05ecfa
Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to af11b45
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7110/head:pull/7110
$ git checkout pull/7110

Update a local copy of the PR:
$ git checkout pull/7110
$ git pull https://git.openjdk.java.net/jdk pull/7110/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7110

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7110.diff

bridgekeeper · 2022-01-17T13:08:36Z

👋 Welcome back eme64! 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 · 2022-01-17T13:10:52Z

@eme64 The following label will be automatically applied to this pull request:

hotspot-runtime

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

…otspot/share/services/dtraceAttacher.hpp

mlbridge · 2022-01-18T08:10:42Z

Webrevs

dholmes-ora

Hi Emanuel,

There are a few changes needed as described in comments below. Also when we deprecate flags we add them to the VMDeprecatedOptions test.

Thanks,
David

dholmes-ora · 2022-01-18T13:09:57Z

src/hotspot/share/runtime/arguments.cpp

      }
    } else if (match_option(option, "-XX:+ExtendedDTraceProbes")) {
 #if defined(DTRACE_ENABLED)
+      warning("-XX:+ExtendedDTraceProbes is deprecated. Use a combination of -XX:+DTraceMethodProbes, -XX:+DTraceAllocProbes and -XX:+DTraceMonitorProbes instead.");


Because the flag is deprecated, a deprecation warning will already be generated for it, so this will cause two warnings to be produced. You either put the flag in the special_jvm_flags table and accept the standard deprecation message, or else you don't put it in the table and handle the deprecation (and later obsoletion) warning directly. In this case the direct approach seems best.

@dholmes-ora : The warning is actually not already generated, since this flag is handled separately, the handling is only done if none of the cases matches, and we land in the } else if (match_option(option, "-XX:", &tail)) { // -XX:xxxx case.
I had to add the warning separately now. I think this is a bug though, @TobiHartmann thought so too. All options should first be checked if they are deprecated.

Thank you very much, I added the flag to the Test.

@eme64 thanks for that correction - yes I thought we checked all args up front and then processed the specialized logic. The opposite does make some sense though as it means we can use the table and do specialized handling. When the flag is obsoleted we delete the specialized handling.

dholmes-ora · 2022-01-18T13:13:01Z

src/java.base/share/man/java.1

 .RE
 .TP
 .B \f[CB]\-XX:+ExtendedDTraceProbes\f[R]
+Deprecated. Use combination of these flags instead: -XX:+DTraceMethodProbes, -XX:+DTraceAllocProbes, -XX:+DTraceMonitorProbes


When a flag is deprecated we move it to the "Deprecated Java Options" section of the manpage.

Thank you @dholmes-ora

eme64 · 2022-01-18T16:04:12Z

/label add hotspot

eme64 · 2022-01-18T16:04:24Z

/label remove hotspot-runtime

openjdk · 2022-01-18T16:05:33Z

@eme64
The hotspot label was successfully added.

openjdk · 2022-01-18T16:06:54Z

@eme64
The hotspot-runtime label was successfully removed.

…278423

openjdk · 2022-01-18T16:14:48Z

⚠️ @eme64 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

hseigel · 2022-01-18T18:42:27Z

Can you replace the use of -XX:+ExtendedDTraceProbes in test/hotspot/jtreg/serviceability/7170638/SDTProbesGNULinuxTest.java with the three new flags ?

dholmes-ora

A few more tweaks below.

Thanks,
David

dholmes-ora · 2022-01-19T01:10:34Z

src/hotspot/os/aix/attachListener_aix.cpp

 #include "runtime/interfaceSupport.inline.hpp"
 #include "runtime/os.inline.hpp"
 #include "services/attachListener.hpp"
-#include "services/dtraceAttacher.hpp"


These changes are somewhat independent of the deprecation issue and could be split out into a separate RFE. The serviceability folk may have an opinion.

@dholmes-ora Ok, I reverted this and will do it in a separate RFE.

dholmes-ora · 2022-01-19T01:12:16Z

src/hotspot/share/runtime/arguments.cpp

    } else if (match_option(option, "-XX:+ExtendedDTraceProbes")) {
 #if defined(DTRACE_ENABLED)
+      warning("Option ExtendedDTraceProbes was deprecated in version 19 and will likely be removed in a future release.");
+      warning("Use a combination of -XX:+DTraceMethodProbes, -XX:+DTraceAllocProbes and -XX:+DTraceMonitorProbes instead.");


s/a/the/

Applies to all three uses.

Agreed, changing it

dholmes-ora · 2022-01-19T01:17:21Z

src/java.base/share/man/java.1

 .RE
 .TP
+.B \f[CB]\-XX:+ExtendedDTraceProbes\f[R]
+Deprecated. Use combination of these flags instead: -XX:+DTraceMethodProbes, -XX:+DTraceAllocProbes, -XX:+DTraceMonitorProbes


Delete "Deprecated" as we are in the deprecated options section.

The wording also needs updating as per the warning text ... though that might read a little odd here so I suggest a tweak:

Use the combination of -XX:+DTraceMethodProbes, -XX:+DTraceAllocProbes and -XX:+DTraceMonitorProbes instead of this deprecated flag.

I would also move that new text to the end, so we still describe the flag first (otherwise it again reads a little odd.)

We will also need to add those flags to the "ADVANCED SERVICEABILITY OPTIONS FOR JAVA" section.

@dholmes-ora thanks for the suggestions, I implemented them.

dholmes-ora · 2022-01-19T01:27:39Z

/label add serviceability

This is really a serviceability flag, though it only does anything interesting in compiler code these days, so adding serviceability team to emails.

openjdk · 2022-01-19T01:28:43Z

@dholmes-ora
The serviceability label was successfully added.

…: /src/hotspot/share/services/dtraceAttacher.hpp" This reverts commit 885b985.

eme64 · 2022-02-09T15:51:59Z

Can you replace the use of -XX:+ExtendedDTraceProbes in test/hotspot/jtreg/serviceability/7170638/SDTProbesGNULinuxTest.java with the three new flags ?

Thank you @hseigel , I patched it according to your suggestion.

hseigel

Other than the need to update the copyright dates to 2022, these changes look good.
Thanks, Harold

openjdk · 2022-02-09T18:50:06Z

@eme64 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:

8278423: ExtendedDTraceProbes should be deprecated

Reviewed-by: dholmes, hseigel, kvn, thartmann

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 466 new commits pushed to the master branch:

aa918a6: 8281033: Improve ImageCheckboxTest to test all available LaF
6fdfe04: 8281674: tools/javac/annotations/typeAnnotations/classfile/AnonymousExtendsTest.java fails with AssertionError
c3179a8: 8281462: Annotation toString output for enum not reusable for source input
4032fe7: 8281238: TYPE_USE annotations not printed in correct position in toString output
83ffbd2: 8277175: Add a parallel multiply method to BigInteger
0786ddb: 8281535: Create a regression test for JDK-4670051
c5ff6e4: 8223077: module path support for dynamic CDS archive
8886839: 8281622: JFR: Improve documentation of jdk.jfr.Relational
e75e8cd: 8279997: check_for_dynamic_dump should not exit vm
e73ee0c: 8281259: MutableBigInteger subtraction could be simplified
... and 456 more: https://git.openjdk.java.net/jdk/compare/62d03c283667c4122130ec9bbeec623e6a2840e5...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @hseigel, @vnkozlov, @TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

dholmes-ora

Hi Emanuel,

A few minor nits below.

Thanks,
David

dholmes-ora · 2022-02-10T05:54:17Z

src/hotspot/share/runtime/globals.hpp

  product(bool, ExtendedDTraceProbes,    false,                             \
-          "Enable performance-impacting dtrace probes")                     \
+          "(Deprecated) Enable performance-impacting dtrace probes. "       \
+          "Use a combination of -XX:+DTraceMethodProbes, "                  \


You missed changing 'a' to 'the' here.

dholmes-ora · 2022-02-10T06:13:17Z

src/java.base/share/man/java.1

-that affect the performance.
-By default, this option is disabled and \f[CB]dtrace\f[R] performs only
-standard probes.
+.B \f[CB]\-XX:+DTraceAllocProbes\f[R]


The three newly documented flags should all be marked "Linux and macOS". It is somewhat of a poor design that the flags are available on all platforms but only have an effect on systems with DTrace or SystemTap support - which (for our main platforms) is Linux and macOS.

dholmes-ora · 2022-02-10T06:16:16Z

src/java.base/share/man/java.1

+that affect the performance.
+By default, this option is disabled and \f[CB]dtrace\f[R] performs only
+standard probes.
+Use the combination of these flags instead: -XX:+DTraceMethodProbes, -XX:+DTraceAllocProbes, -XX:+DTraceMonitorProbes


The flags should be in a code font (use `-XX:...` in the markdown source).

dholmes-ora · 2022-02-10T06:16:51Z

src/java.base/share/man/java.1

 .TP
+.B \f[CB]\-XX:+ExtendedDTraceProbes\f[R]
+\f[B]Linux and macOS:\f[R] Enables additional \f[CB]dtrace\f[R] tool probes
+that affect the performance.


Existing grammatical nit: please delete 'the'.

dholmes-ora

Thanks for the updates.

David

vnkozlov

Good.

TobiHartmann

Looks good.

eme64 · 2022-02-12T09:34:28Z

Thanks @dholmes-ora , @vnkozlov , @TobiHartmann @hseigel for the reviews.
/integrate

openjdk · 2022-02-12T09:35:24Z

@eme64
Your change (at version 78d8e00) is now ready to be sponsored by a Committer.

dholmes-ora · 2022-02-12T13:07:12Z

/sponsor

openjdk · 2022-02-12T13:08:53Z

Going to push as commit 67077a0.
Since your change was applied there have been 467 commits pushed to the master branch:

58dae60: 8274524: SSLSocket.close() hangs if it is called during the ssl handshake
aa918a6: 8281033: Improve ImageCheckboxTest to test all available LaF
6fdfe04: 8281674: tools/javac/annotations/typeAnnotations/classfile/AnonymousExtendsTest.java fails with AssertionError
c3179a8: 8281462: Annotation toString output for enum not reusable for source input
4032fe7: 8281238: TYPE_USE annotations not printed in correct position in toString output
83ffbd2: 8277175: Add a parallel multiply method to BigInteger
0786ddb: 8281535: Create a regression test for JDK-4670051
c5ff6e4: 8223077: module path support for dynamic CDS archive
8886839: 8281622: JFR: Improve documentation of jdk.jfr.Relational
e75e8cd: 8279997: check_for_dynamic_dump should not exit vm
... and 457 more: https://git.openjdk.java.net/jdk/compare/62d03c283667c4122130ec9bbeec623e6a2840e5...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-12T13:09:12Z

@dholmes-ora @eme64 Pushed as commit 67077a0.

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

dholmes-ora · 2022-02-12T13:50:14Z

@eme64 the test fails in our CI as it encounters builds for which DTrace is not enabled - see JDK-8281675.

8278423: ExtendedDTraceProbes should be deprecated

1576b47

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 17, 2022

removed file with declarations that are never defined or used: /src/h…

885b985

…otspot/share/services/dtraceAttacher.hpp

eme64 marked this pull request as ready for review January 18, 2022 08:04

openjdk bot added the rfr Pull request is ready for review label Jan 18, 2022

eme64 added 2 commits January 18, 2022 12:24

Fix formatting issue

7cf96a7

Mode of speech edit

ecbee3a

dholmes-ora suggested changes Jan 18, 2022

View reviewed changes

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jan 18, 2022

openjdk bot removed the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 18, 2022

eme64 added 2 commits January 18, 2022 17:12

added flag to VMDeprecatedOptions Test

6edf5a8

Merge branch 'JDK-8278423' of https://github.com/eme64/jdk into JDK-8…

e936a0d

…278423

moved deprecated flag to deprecated section in manpages

0f161b0

dholmes-ora suggested changes Jan 19, 2022

View reviewed changes

openjdk bot added the serviceability serviceability-dev@openjdk.org label Jan 19, 2022

Revert "removed file with declarations that are never defined or used…

7a93eca

…: /src/hotspot/share/services/dtraceAttacher.hpp" This reverts commit 885b985.

eme64 marked this pull request as draft February 9, 2022 15:38

openjdk bot removed the rfr Pull request is ready for review label Feb 9, 2022

replaced with 3 flags in test

be3e0b8

updated warning messages and added 3 flags to man-pages

b05ecfa

eme64 marked this pull request as ready for review February 9, 2022 16:20

openjdk bot added the rfr Pull request is ready for review label Feb 9, 2022

hseigel approved these changes Feb 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 9, 2022

dholmes-ora reviewed Feb 10, 2022

View reviewed changes

fixes to documentation requested by reviewers

af11b45

dholmes-ora approved these changes Feb 10, 2022

View reviewed changes

vnkozlov approved these changes Feb 11, 2022

View reviewed changes

fix in response to suggestion by David Holmes

78d8e00

TobiHartmann approved these changes Feb 11, 2022

View reviewed changes

dholmes-ora approved these changes Feb 11, 2022

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 12, 2022

openjdk bot added the integrated Pull request has been integrated label Feb 12, 2022

openjdk bot closed this Feb 12, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 12, 2022

Conversation

eme64 commented Jan 17, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 17, 2022

Uh oh!

openjdk bot commented Jan 17, 2022

Uh oh!

mlbridge bot commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eme64 Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eme64 commented Jan 18, 2022

Uh oh!

eme64 commented Jan 18, 2022

Uh oh!

openjdk bot commented Jan 18, 2022

Uh oh!

openjdk bot commented Jan 18, 2022

Uh oh!

openjdk bot commented Jan 18, 2022

Uh oh!

hseigel commented Jan 18, 2022

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Jan 19, 2022

Uh oh!

openjdk bot commented Jan 19, 2022

Uh oh!

eme64 commented Feb 9, 2022

Uh oh!

hseigel left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

eme64 commented Jan 17, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 18, 2022 •

edited

Loading

eme64 Jan 18, 2022 •

edited

Loading

openjdk bot commented Feb 9, 2022 •

edited

Loading