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

8258414: OldObjectSample events too expensive #2780

Closed
wants to merge 2 commits into from

Conversation

@mgronlun
Copy link

@mgronlun mgronlun commented Mar 1, 2021

Hi Florian,

thanks for your great attempt to fix this issue, it is indeed a complex area with many interrelations.

I think your suggestion has two problems that we need to solve:

  1. If we separate the Leak Profiler stacktraces from the "regular" traces, there is a disconnect at the stacktrace record site, because the stacktrace id is cached to the thread. This means other events (ObjectAllocationSample and other memory allocation events) will attempt to reuse the id. Since this id is no longer valid for reuse, it must be handled.

  2. The division into two separate stacktrace repositories now also separates the stacktrace id assignment, with a consequence that separate stacktraces can now end up having overlapping and conflicting ids.

I also appreciated your effort in trying to incorporate the count of the traces written, to be tallied in the "regular" process. I do think we can simplify this a bit by not having them tallied (they are only a very small number, max 256 by default).

Please take a look and let me know what you think about this suggestion.

Thanks!
Markus

PS If we go with this suggestion, I will put you down as a contributor.


Progress

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

Issue

Reviewers

Contributors

  • Florian David <florian.david@datadoghq.com>

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 1, 2021

👋 Welcome back mgronlun! 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 Mar 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 1, 2021

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

  • hotspot-jfr

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.

@openjdk openjdk bot added the hotspot-jfr label Mar 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 1, 2021

@mgronlun Could not parse flodav as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 1, 2021

Webrevs

@@ -196,9 +220,9 @@ traceid JfrStackTraceRepository::add_trace(const JfrStackTrace& stacktrace) {
}

// invariant is that the entry to be resolved actually exists in the table
const JfrStackTrace* JfrStackTraceRepository::lookup(unsigned int hash, traceid id) const {
const JfrStackTrace* JfrStackTraceRepository::lookup(unsigned int hash, traceid id) {

Could the comment be extended to make it explicit that the lookup will be done only in the leak profiler stacktrace repo? Or, better yet the method renamed to lookup_in_leak_profiler() (but it might be too long ...).

Copy link
Author

@mgronlun mgronlun Mar 11, 2021

Thanks, Jaroslav - yes, indeed.

Copy link
Author

@mgronlun mgronlun Mar 11, 2021

I also attempted to have the artefact tagging happen lazily, e.g. methods and classes, making it a part of the stack trace resolution process. There is a problem with that approach concerning class unloading because a referenced class can unload, but since it does not yet have the proper tag, the framework does not intercept it. I elaborated on setting an "interest" tag to handle this, but that approach opens additional challenges related to visibility and concurrency. Hard problem to solve; I was unable to do it (yet).

Probably can live without lazy tagging in the first iteration - still it will be much better than the current state.
And then we can keep on adding incremental improvements based on cost-benefit weighing.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 11, 2021

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

8258414: OldObjectSample events too expensive

Co-authored-by: Florian David <florian.david@datadoghq.com>
Reviewed-by: jbachorik

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

  • 0bbe064: 8263354: Accumulated C2 code cleanups
  • aa33443: 8262454: Handshake timeout improvements, single target, kill unfinished thread
  • ff25939: 8263426: Reflow JfrNetworkUtilization::send_events
  • e25ad73: 8263430: Uninitialized Method* variables after JDK-8233913
  • 9f6b1d7: 8263436: Silly array comparison in GaloisCounterMode.overlapDetection
  • ad1f605: 8263353: assert(CompilerOracle::option_matches_type(option, value)) failed: Value must match option type
  • cf1c021: 8263480: ProblemList two jpackage tests on Windows
  • f3bd801: 8263403: [JVMCI] output written to tty via HotSpotJVMCIRuntime can be garbled
  • b92abac: 8263433: Shenandoah: Don't expect forwarded objects in set_concurrent_mark_in_progress()
  • 15dacca: 8263465: JDK-8236847 causes tier1 build failure on linux-aarch64
  • ... and 153 more: https://git.openjdk.java.net/jdk/compare/03d888f463c0a6e3fee70ed8ad606fc0a3082636...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 Mar 11, 2021
@mgronlun
Copy link
Author

@mgronlun mgronlun commented Mar 12, 2021

/contributor add Florian David florian.david@datadoghq.com

@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@mgronlun
Contributor Florian David <florian.david@datadoghq.com> successfully added.

@mgronlun
Copy link
Author

@mgronlun mgronlun commented Mar 12, 2021

/integrate

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

@openjdk openjdk bot commented Mar 12, 2021

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

  • 0bbe064: 8263354: Accumulated C2 code cleanups
  • aa33443: 8262454: Handshake timeout improvements, single target, kill unfinished thread
  • ff25939: 8263426: Reflow JfrNetworkUtilization::send_events
  • e25ad73: 8263430: Uninitialized Method* variables after JDK-8233913
  • 9f6b1d7: 8263436: Silly array comparison in GaloisCounterMode.overlapDetection
  • ad1f605: 8263353: assert(CompilerOracle::option_matches_type(option, value)) failed: Value must match option type
  • cf1c021: 8263480: ProblemList two jpackage tests on Windows
  • f3bd801: 8263403: [JVMCI] output written to tty via HotSpotJVMCIRuntime can be garbled
  • b92abac: 8263433: Shenandoah: Don't expect forwarded objects in set_concurrent_mark_in_progress()
  • 15dacca: 8263465: JDK-8236847 causes tier1 build failure on linux-aarch64
  • ... and 153 more: https://git.openjdk.java.net/jdk/compare/03d888f463c0a6e3fee70ed8ad606fc0a3082636...master

Your commit was automatically rebased without conflicts.

Pushed as commit a9b156d.

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

@jbachorik
Copy link

@jbachorik jbachorik commented Mar 12, 2021

Yahoo! Thanks, Markus!

@mgronlun
Copy link
Author

@mgronlun mgronlun commented Mar 12, 2021

Thank you, Jaroslav for contributing and reviewing!

@jbachorik
Copy link

@jbachorik jbachorik commented Apr 20, 2021

/backport jdk16u

@openjdk
Copy link

@openjdk openjdk bot commented Apr 20, 2021

@jbachorik Unknown command backport - for a list of valid commands use /help.

@jbachorik
Copy link

@jbachorik jbachorik commented Apr 20, 2021

/help

@openjdk
Copy link

@openjdk openjdk bot commented Apr 20, 2021

@jbachorik Available commands:

  • covered - used when employer has signed the OCA
  • help - shows this text
  • signed - used after signing the OCA
  • test - used to run tests

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