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

8258912: Remove JVM options CountJNICalls and CountJVMCalls #2050

Closed
wants to merge 1 commit into from

Conversation

@hseigel
Copy link
Member

@hseigel hseigel commented Jan 12, 2021

Please review this change to remove the old non-product JVM -XX flags CountRuntimeCalls, CountJNICalls, CountJVMCalls, and CountRemovableExceptions. Additionally, this change removes non-product flag WarnOnStalledSpinLock because, as a result of this change, it is no longer used.

The change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.

Thanks, Harold


Progress

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

Issue

  • JDK-8258912: Remove JVM options CountJNICalls and CountJVMCalls

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 12, 2021

👋 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.

Loading

@openjdk openjdk bot added the rfr label Jan 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

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

  • hotspot

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.

Loading

@openjdk openjdk bot added the hotspot label Jan 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 12, 2021

Webrevs

Loading

Copy link
Contributor

@coleenp coleenp left a comment

LGTM!

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

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

8258912: Remove JVM options CountJNICalls and CountJVMCalls

Reviewed-by: coleenp, lfoltan, shade

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

  • a483869: 8225045: javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java fails on linux-x64
  • 44c8379: 8256019: JLabel HTML text does not support translucent text colors
  • 0957d9e: 8259519: The java.awt.datatransfer.DataFlavor#ioInputStreamClass field is redundant
  • 65bed64: 8253635: Implement toString() for SSLEngineImpl
  • c6d798c: 8259629: aarch64 builds fail after JDK-8258932
  • 4be2173: 8259319: Illegal package access when SunPKCS11 requires SunJCE's classes
  • e4df209: 7018932: Drawing very large coordinates with a dashed Stroke can cause Java to hang
  • 5f7ccce: 8226810: Failed to launch JVM because of NullPointerException occured on System.props
  • d6a2105: 8259343: [macOS] Update JNI error handling in Cocoa code.
  • c338f11: 8259349: -XX:AvgMonitorsPerThreadEstimate=1 does not work right

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

Loading

@openjdk openjdk bot added the ready label Jan 12, 2021
Copy link
Member

@lfoltan lfoltan left a comment

LGTM.
Lois

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Harold,

The call counters seem like useful statistics to me, though ones that may only be checked during specific types of development work. I think some consultation may be in order here rather than "take it away and see if anyone complains".

At a minimum I'd like to know whether our benchmarking/optimisation experts ever utilise these when figuring out code changes - paging @cl4es and @shipilev :)

Thanks,
David

Loading

Copy link
Contributor

@shipilev shipilev left a comment

I never used these flags, and I don't think they are very useful. I think most of that data is supposed to be delivered by JFR these days, instead of ad-hoc profiling facilities. If there are no relevant JFR events, they could be added later.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 13, 2021

Mailing list message from David Holmes on hotspot-dev:

On 13/01/2021 12:38 pm, David Holmes wrote:

On Tue, 12 Jan 2021 15:17:52 GMT, Harold Seigel <hseigel at openjdk.org> wrote:

Please review this change to remove the old non-product JVM -XX flags CountRuntimeCalls, CountJNICalls, CountJVMCalls, and CountRemovableExceptions. Additionally, this change removes non-product flag WarnOnStalledSpinLock because, as a result of this change, it is no longer used.

The change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and tiers 3-5 on Linux x64.

Thanks, Harold

Hi Harold,

The call counters seem like useful statistics to me, though ones that may only be checked during specific types of development work. I think some consultation may be in order here rather than "take it away and see if anyone complains".

At a minimum I'd like to know whether our benchmarking/optimisation experts ever utilise these when figuring out code changes - paging @cl4es and @shipilev :)

Seems Claes and Alexey are okay with removing these. I don't know who
else may use them. If it turns out to be an issue we can revert, but
there are probably better means of profiling (though this one is simple
and lightweight).

Thanks,
David

Loading

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 14, 2021

Thanks Coleen, Lois, and Aleksey for reviewing this change and David for his comments!

/integrate

Loading

@openjdk openjdk bot closed this Jan 14, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 14, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 14, 2021

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

  • be57cf1: 8226416: MonitorUsedDeflationThreshold can cause repeated async deflation requests
  • c822eda: 8259699: Reduce char[] copying in URLEncoder.encode(String, Charset)
  • ff3e6e4: 8259773: Incorrect encoding of AVX-512 kmovq instruction
  • b8ef2ba: 8259563: The CPU model name is printed multiple times when using -Xlog:os+cpu
  • b040a3d: 8259631: Reapply pattern match instanceof use in HttpClientImpl
  • 3462f7a: 8256955: Move includes of events.hpp out of header files
  • 8b8b1f9: 8259706: C2 compilation fails with assert(vtable_index == Method::invalid_vtable_index) failed: correct sentinel value
  • ae9187d: 8256109: Create implementation for NSAccessibilityButton protocol
  • 5513f98: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy
  • 51e14f2: Merge
  • ... and 40 more: https://git.openjdk.java.net/jdk/compare/ccac7aaea380bc36c8dd0fe6e724c964489eeb6a...master

Your commit was automatically rebased without conflicts.

Pushed as commit 38a1201.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants