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

8256718: Obsolete the long term deprecated and aliased Trace flags #1525

Closed
wants to merge 3 commits into from

Conversation

hseigel
Copy link
Member

@hseigel hseigel commented Nov 30, 2020

Please review this change to obsolete the deprecated and aliased Trace flags. The now empty aliased_logging_flags support was left in arguments.cpp for use by trace flags that get deprecated and aliased in the future.

With this change, users will get the following example messages when using these obsolete flags, depending on whether -XX:+... or -XX:-... was specified:

VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. Please use -Xlog:class+path=info instead.

VM warning: Ignoring option TraceClassPaths; support was removed in 16.0. Please use -Xlog:class+path=off instead.

The change was tested with tiers1and 2 on Linux, Windows, and MacOS, and tiers 3-5 on Linux x64 and with JCK lang and vm tests.

Thanks, Harold


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8256718: Obsolete the long term deprecated and aliased Trace flags

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1525/head:pull/1525
$ git checkout pull/1525

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 30, 2020

👋 Welcome back hseigel! 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 openjdk bot added the rfr label Nov 30, 2020
@openjdk
Copy link

openjdk bot commented Nov 30, 2020

@hseigel 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.

@openjdk openjdk bot added serviceability hotspot labels Nov 30, 2020
@hseigel
Copy link
Member Author

hseigel commented Nov 30, 2020

/label add hotspot-runtime

@openjdk openjdk bot added the hotspot-runtime label Nov 30, 2020
@openjdk
Copy link

openjdk bot commented Nov 30, 2020

@hseigel
The hotspot-runtime label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Nov 30, 2020

Webrevs

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Hi Harold,

The fix looks okay to me.
I was more focusing on the serviceability flags.
I'm not sure the flag TraceJVMTIObjectTagging should be mentioned in the src/java.base/share/man/java.1 the same way as the TraceRedefineClasses.

Thanks,
Serguei

@openjdk
Copy link

openjdk bot commented Nov 30, 2020

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

8256718: Obsolete the long term deprecated and aliased Trace flags

Reviewed-by: sspitsyn, iklam, dholmes, coleenp

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

  • 3da30e9: 8257241: CDS should not handle disableEagerInitialization for archived lambda proxy classes
  • 7104400: 8257164: Share LambdaForms for VH linkers/invokers
  • 3e89981: 8257623: vmTestbase/nsk/jvmti/ResourceExhausted/resexhausted001/TestDescription.java shouldn't use timeout
  • 93b6ab5: 8256818: SSLSocket that is never bound or connected leaks socket resources
  • 692b273: 8257189: Handle concurrent updates of MH.form better
  • 6704266: 8257565: epsilonBarrierSet.hpp should not include barrierSetAssembler
  • 0b8c780: 8256256: UL should not use heap allocation for output string
  • 2508bc7: 8257140: Crash in JvmtiTagMap::flush_object_free_events()
  • cfb50a9: 8253916: ResourceExhausted/resexhausted001 crashes on Linux-x64
  • 287b829: 8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing
  • ... and 47 more: https://git.openjdk.java.net/jdk/compare/6eb25d7cb42fb313172d90a56f231743cf6a7884...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 openjdk bot added the ready label Nov 30, 2020
iklam
iklam approved these changes Dec 1, 2020
Copy link
Member

@iklam iklam left a comment

LGTM. Thanks for fixing the CDS tests.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Harold,
I don't think we need to add all this new infrastructure for obsolete-aliased flags just so we can continue to print the -Xlog equivalents. I think simply adding these previously aliased flags as obsolete flags in the special flags table would suffice.
Thanks,
David

@dholmes-ora
Copy link
Member

dholmes-ora commented Dec 1, 2020

I should clarify this. I suggested in the bug report that we could continue to print the -Xlog equivalents, but when I wrote that I was thinking that the existing deprecated-aliased code would simply be changed to obsoleted-aliased code. I didn't consider that we might need to keep the deprecated-aliased code for future conversions to UL. But I can see now that we might want to do that. In which case I'd prefer to no longer print the -Xlog equivalents, rather than duplicate the deprecated-aliased code into an obsolete-aliased form.
My intent was to get rid of this special aliased flag handling code in the near future.
Thanks,
David

Copy link
Contributor

@coleenp coleenp left a comment

I agree with David. We should remove the helpful messages at least for most of the obsolete Print/Trace flags. Not sure about the big 3 though.

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved
{ "TraceJVMTIObjectTagging", "-Xlog:jvmti+objecttagging=", "debug", "16.0" },
{ "TraceRedefineClasses", "-Xlog:redefine+class=", "info", "16.0" },
{ "PrintJNIResolving", "-Xlog:jni+resolve=", "debug", "16.0" },
{ NULL, NULL, NULL, NULL }
Copy link
Contributor

@coleenp coleenp Dec 1, 2020

Choose a reason for hiding this comment

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

I think if we wanted to give a message that the flag was obsolete and to suggest changing the command line, we should only do it for -XX:+TraceClassLoading and -XX:+TraceExceptions (I'd originally thought -XX:+TraceClassUnloading was important enough to release note but now I'm not so sure.) The rest of the flags should either go in the table that they're no longer recognized.

Copy link
Member

@dholmes-ora dholmes-ora Dec 1, 2020

Choose a reason for hiding this comment

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

Keeping the message for any flag requires keeping all the supporting code. I don't see the "big 3" are special. They have been deprecated since 9 and we have clearly told people this when they use them. We're also release-noting this for 16 (again - this was documented when UL was added). I don't think we have to pander to anyone who hasn't updated their launch scripts by now.

Copy link
Contributor

@coleenp coleenp Dec 2, 2020

Choose a reason for hiding this comment

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

For these three, I kind of like the pandering. I'm not sure that the release note will reach people using these, especially -XX:+TraceExceptions. I guess they've been getting a deprecation message since 9 so maybe it won't be such a surprise. I never stand in the way of removing code that people won't need, if you think this is the case.

Copy link
Member

@dholmes-ora dholmes-ora Dec 2, 2020

Choose a reason for hiding this comment

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

I think we should remove them. There is a risk that the user might not notice the difference between the old and new messages and so not realize they don't actually have active logging any more (until they go to access the logs to diagnose a problem).

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved
@hseigel
Copy link
Member Author

hseigel commented Dec 1, 2020

Thanks everyone for the reviews. Please review the updated commit which contains the following changes:

  1. Moves most of the flags to the normal location for obsolete flags.

  2. Keeps the extra -Xlog message for the three most commonly used flags.

  3. Removes the removed_develop_logging_flags struct and support functions.

  4. Removes the aliased_logging_flags struct and support functions based on Coleen's statement that there are no existing flags that would use it.

Thanks! Harold

iklam
iklam approved these changes Dec 1, 2020
coleenp
coleenp approved these changes Dec 2, 2020
Copy link
Contributor

@coleenp coleenp left a comment

I approve if you remove or leave the big 3 as aliases.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

I'm still pushing for treating all flags the same and removing all the aliased-flag code.

Coleen seems to be okay either way. :)

Thanks,
David

{ "TraceJVMTIObjectTagging", "-Xlog:jvmti+objecttagging=", "debug", "16.0" },
{ "TraceRedefineClasses", "-Xlog:redefine+class=", "info", "16.0" },
{ "PrintJNIResolving", "-Xlog:jni+resolve=", "debug", "16.0" },
{ NULL, NULL, NULL, NULL }
Copy link
Member

@dholmes-ora dholmes-ora Dec 2, 2020

Choose a reason for hiding this comment

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

I think we should remove them. There is a risk that the user might not notice the difference between the old and new messages and so not realize they don't actually have active logging any more (until they go to access the logs to diagnose a problem).

@hseigel
Copy link
Member Author

hseigel commented Dec 2, 2020

Please review this latest webrev. It removes all the aliasing code for the obsolete tracing flags and does not suggest logging alternatives in their obsolete flag messages.

Thanks, Harold

Copy link
Contributor

@sspitsyn sspitsyn left a comment

The update looks good to me. It is a nice simplification.

iklam
iklam approved these changes Dec 2, 2020
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Looks good!

Thanks,
David

@hseigel
Copy link
Member Author

hseigel commented Dec 3, 2020

/integrate

@hseigel
Copy link
Member Author

hseigel commented Dec 3, 2020

Thanks Coleen, Ioi, Serguei, and David for all the reviews.
Harold

@openjdk openjdk bot closed this Dec 3, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 3, 2020
@openjdk
Copy link

openjdk bot commented Dec 3, 2020

@hseigel Since your change was applied there have been 66 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit e4497c9.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-runtime integrated serviceability
5 participants