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

8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing #900

Closed
wants to merge 1 commit into from

Conversation

@fisk
Copy link
Contributor

@fisk fisk commented Oct 28, 2020

Today, when you crash, the GCLogPrecious::_lock is taken. This effectively limits you to only get clean crash reports if you crash or assert without holding a lock of rank tty or lower. It is arguably difficult to know what locks you are going to have when crashing. Therefore, I don't think the precious GC log should constrain possible crashing contexts in that fashion.

This patch inserts the precious GC lines into a linked list instead, that can be traversed concurrently, without holding any locks. This allows you to crash in contexts where "low" ranked locks are held.

I have manually verified that the hs_err precious GC log looks identical before and after my change.


Progress

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

Testing

Linux x32 Linux x64 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) (1/2 running)
Test (tier1) ✔️ (9/9 passed) (3/9 running)

Issue

  • JDK-8254877: GCLogPrecious::_lock rank constrains what locks you are allowed to have when crashing

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 28, 2020

👋 Welcome back eosterlund! 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 Oct 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2020

Hi @openjdk[bot], thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user openjdk[bot] for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

Loading

@openjdk openjdk bot added the hotspot-gc label Oct 28, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 28, 2020

Hi @mlbridge[bot], thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlbridge[bot] for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

Loading

GCLogPreciousLine* head = _head;
GCLogPreciousLine* tail = _tail;

if (head == NULL) {

Choose a reason for hiding this comment

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

I think this should be Atomic::load(&_head). This would make the head local variable unused.

Loading

}
if (_tail != NULL) {
tail->set_next(line);
}

Choose a reason for hiding this comment

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

Either 61 should be using tail or 62 should be using _tail. If the latter then the tail local variable is unused.

Loading

@fisk
Copy link
Contributor Author

@fisk fisk commented Oct 28, 2020

I had some offline discussions with StefanK. He would like to keep the stringStream that we have today, so that he can print the precious GC log from a debugger with less effort. He has an entirely different alternative. So I am going to close this PR, in favour of Stefan's solution.

Loading

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