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

8266936: Add a finalization JFR event #351

Closed

Conversation

jbachorik
Copy link

@jbachorik jbachorik commented Apr 19, 2022

Backport of openjdk/jdk#4731


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (1 reviews required, with at least 1 reviewer)

Issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17u-dev pull/351/head:pull/351
$ git checkout pull/351

Update a local copy of the PR:
$ git checkout pull/351
$ git pull https://git.openjdk.java.net/jdk17u-dev pull/351/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 351

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk17u-dev/pull/351.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 19, 2022

👋 Welcome back jbachorik! 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 changed the title Backport 72a976ef05fc2c62657920a560a0abc60b27c852 8266936: Add a finalization JFR event Apr 19, 2022
@openjdk
Copy link

openjdk bot commented Apr 19, 2022

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Apr 19, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 19, 2022

Webrevs

@egahlin
Copy link
Member

egahlin commented Apr 19, 2022

I think we should try to avoid backporting JFR events. It's a feature and those should be kept to a minimum. It's a slippery slope, why not backport any other events? In this case, the change also impacts a lot of files, making it risky.

@jbachorik
Copy link
Author

jbachorik commented Apr 20, 2022

I put the rationale (making the life of the users which tend to be stuck at LTS releases easier) in the backport request in the JIRA ticket.

why not backport any other events

TBH, I am not sure why there is such a great resistance to backporting events, especially if they happen to be a simple change and might bring really good value to the users (not saying this is the case, though). The JFR format is self describing and the events themselves are discoverable at runtime so it is not like introducing a new event would cause backward compatibility issues.

Well, I tried. We heard legitimate reasons from our customers why it would make sense to them to have this particular event in an LTS version but I knew it was going to be an uphill battle ...

@GoeLin
Copy link
Member

GoeLin commented Apr 26, 2022

Hi, I was about to ague for this change. But your backport is buggy.

Internal Error (/sapmnt/sapjvm_work/openjdk/nb/linuxx86_64/jdk17u-dev/src/hotspot/share/runtime/mutex.cpp:70), pid=1987518, tid=1987868

assert(!thread->is_active_Java_thread() || _safepoint_check_required != _safepoint_check_always) failed: This lock should always have a safepoint check for Java threads: JfrMsg_lock

Also, please enable pre-submit tests in your repo.
Labeling jdk17u-fix-request is the last you do, only once the change is reviewed and the instructions are followed. See the project wiki page for instructions please.

@egahlin
Copy link
Member

egahlin commented Apr 26, 2022

Well, I tried. We heard legitimate reasons from our customers why it would make sense to them to have this particular event in an LTS version but I knew it was going to be an uphill battle ...

Imagine another customer that is updating because of a severe vulnerability and suddenly the JVM crashes so they can't upgrade. Adding events is a slippery slope and with a crash (or other issue) we might end up in situation where users don't trust to use JFR in production.

@jbachorik jbachorik closed this Apr 26, 2022
@jbachorik jbachorik deleted the jbachorik-backport-72a976ef branch April 26, 2022 11:32
@jbachorik jbachorik restored the jbachorik-backport-72a976ef branch April 26, 2022 11:37
@jbachorik jbachorik reopened this Apr 26, 2022
@jbachorik
Copy link
Author

Imagine another customer that is updating because of a severe vulnerability and suddenly the JVM crashes so they can't upgrade.

I would argue that such crashing event is a haphazard even in a non-update release.

My point is if the event is already being used successfully in an already publicly released JDK version AND the changes are trivial enough to be able to reason about the correctness, I find an a-priori refusal of backporting events a bit heavyhanded.

I agree that if the changes tend to be too complex to be able to have a clean argument about their correctness we should refrain from such backports. But this would be no different from any other backport, AFAIK.

@jbachorik
Copy link
Author

Also, please enable pre-submit tests in your repo.

Sorry, didn't realize I had to enable them manually. Working on it.

@jbachorik jbachorik marked this pull request as draft April 26, 2022 11:46
@jbachorik
Copy link
Author

Moving back to draft until I have resolved the problem reported by @GoeLin

@openjdk openjdk bot removed the rfr Pull request is ready for review label Apr 26, 2022
@jbachorik jbachorik force-pushed the jbachorik-backport-72a976ef branch from 046f555 to 5c318ec Compare May 2, 2022 10:13
@egahlin
Copy link
Member

egahlin commented May 2, 2022

I would argue that such crashing event is a haphazard even in a non-update release.

Over time the stability of a release increases if new features are not added. If you are going to adapt a non-update release, you can do extensive testing. This may not be an option if you need to upgrade immediately due to a vulnerability.

If a customer ask why the JVM crashes after an update, it's possible to explain it was a bug that unfortunately came in with another bug fix. I can't explain why a Finalization event was needed for an update release.

I agree that if the changes tend to be too complex to be able to have a clean argument about their correctness we should refrain from such backports. But this would be no different from any other backport, AFAIK.

I think there is a difference between bug fixes and features, not just the complexity.

Bug fixes are meant to increase stability. Sometimes you need to backport features, but the bar should be high and there should be a good reason, for example to support new hardware or OS versions. Improve security. Things that are essential to be able to use the existing JDK release. I don't see this JFR event fall into that category. It's also a rather large change.

@jbachorik
Copy link
Author

jbachorik commented May 2, 2022

I think there is a difference between bug fixes and features, not just the complexity.

Bug fixes are meant to increase stability. Sometimes you need to backport features, but the bar should be high and there should be a good reason, for example to support new hardware or OS versions. Improve security. Things that are essential to be able to use the existing JDK release. I don't see this JFR event fall into that category. It's also a rather large change.

I am totally getting this. My intention was to bring this up with the maintainer as a part of the backport request process - there is a trade off in the benefit to the end user and the possible risk due to unforeseen code breakages.

Also, I was under impression that the backport PR was only to establish the veracity and correctness of the said backport and the decision whether it would be applied or not (if the changes are judged to be correct and true) would be up to the maintainer and is to be expressed by properly labelling the main JIRA ticket.

Additionally, there seems to be some catch-22 situation here. The maintainer will not really consider the backport request unless the PR is reviewed and the PR will not be reviewed unless the maintainer pre-approves the backport?


The backport is now fixed and all pre-submit checks are passing.
The change required to make things work was to mark JfrMsg_Lock as never requiring checkpoint check. I checked both usages of this lock and they are compatible with this mode so I changed it.

@jbachorik jbachorik marked this pull request as ready for review May 2, 2022 17:48
@openjdk openjdk bot added the rfr Pull request is ready for review label May 2, 2022
@theRealAph
Copy link

theRealAph commented May 3, 2022

My point is if the event is already being used successfully in an already publicly released JDK version AND the changes are trivial enough to be able to reason about the correctness, I find an a-priori refusal of backporting events a bit heavyhanded.

That's not a valid argument. Updates releases are not for feature enhancements.
Here's what the guidelines say:

Fix Approvals
In general we follow the common rules for the jdk-updates project.
In addition this list, we will consider

Build system improvements, particularly for stability and reproducibility
Changes necessary to make 17u work on new operating systems and hardware

However, if there is an enhancement that provides substantial benefit to OpenJDK 17u users and it is of low risk, we will consider it.

The crucial wording here is trivial enough, which this isn't, and substantial benefit. I think that @egahlin has called this one correctly.

@mlbridge
Copy link

mlbridge bot commented May 3, 2022

Mailing list message from Lindenmaier, Goetz on jdk-updates-dev:

Hi,

My point is if the event is already being used successfully in an already
publicly released JDK version _AND_ the changes are trivial enough to be
able to reason about the correctness, I find an a-priori refusal of backporting
events a bit heavyhanded.

Another problem is that this obviously does not hold:
You claimed testing would be fine, but there were
errors that slipped your testing, i.e. this is so complicated
you neither get it right by reasoning nor by testing.

If it was trivial, I would have considered the _substantial benefit_
argument ... but this is not trivial enough.

Best regards,
Goetz.

@jbachorik jbachorik closed this May 3, 2022
@jbachorik jbachorik deleted the jbachorik-backport-72a976ef branch May 3, 2022 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

4 participants