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

8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected #16961

Closed
wants to merge 3 commits into from

Conversation

sspitsyn
Copy link
Contributor

@sspitsyn sspitsyn commented Dec 5, 2023

This is a trivial fix for a regression caused by:
8308614 Enabling JVMTI ClassLoad event slows down vthread creation by factor 10

The fix of 8308614 just triggered a known issue:
8316283 field watch events are not always posted with -Xcomp option

The fix is just a work around with the extra checks with the JvmtiExport::should_post_field_access() and JvmtiExport::should_post_field_modification().

Testing:

  • The test runtime/jni/FastGetField/FastGetField.java does not fail anymore with this fix
  • In progress: Test with tiers 1-6

Progress

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

Issue

  • JDK-8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected (Bug - P2)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16961/head:pull/16961
$ git checkout pull/16961

Update a local copy of the PR:
$ git checkout pull/16961
$ git pull https://git.openjdk.org/jdk.git pull/16961/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16961

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16961.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 5, 2023

👋 Welcome back sspitsyn! 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 Pull request is ready for review label Dec 5, 2023
@openjdk
Copy link

openjdk bot commented Dec 5, 2023

@sspitsyn 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 serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Dec 5, 2023
@mlbridge
Copy link

mlbridge bot commented Dec 5, 2023

Webrevs

Comment on lines 557 to 562
// If interp_only_mode has been enabled then we must eagerly create JvmtiThreadState
// objects for globally enabled virtual thread filtered events. Otherwise,
// it is an important optimization to create JvmtiThreadState objects lazily.
if (JvmtiThreadState::seen_interp_only_mode()) {
if (JvmtiThreadState::seen_interp_only_mode() ||
JvmtiExport::should_post_field_access() ||
JvmtiExport::should_post_field_modification()){
Copy link
Member

Choose a reason for hiding this comment

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

The comment needs updating to explain the extra checks.

Can't say I see the connection with 8316283 as no -Xcomp is involved in the current failures AFAICS.

Copy link
Contributor Author

@sspitsyn sspitsyn Dec 5, 2023

Choose a reason for hiding this comment

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

The -Xcomp flag is not a root cause but a trigger. There is a general issue that a frame can be not deoptimized. The -Xcomp is a stress that helps to reproduce the issue as the -Xcomp option and interp_only_mode work in opposite directions.

Thank you for the suggestion. I'll add a comment. Current suggestion is below:

 // If interp_only_mode has been enabled then we must eagerly create JvmtiThreadState
 // objects for globally enabled virtual thread filtered events. Otherwise,
 // it is an important optimization to create JvmtiThreadState objects lazily.
+  // This optimization is disabled when watchpoint capabilities are present. It is to
+  // work around a bug with virtual thread frames which can be not deoptimized in time. 

@openjdk openjdk bot removed the rfr Pull request is ready for review label Dec 5, 2023
Copy link
Member

@dcubed-ojdk dcubed-ojdk left a comment

Choose a reason for hiding this comment

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

Thumbs up. This is a trivial fix.

You'll need to fix the whitespace complaint before integration.

@sspitsyn
Copy link
Contributor Author

sspitsyn commented Dec 5, 2023

Dan, thank you a lot for quick review! I'll fix the whitespace issue.

@openjdk
Copy link

openjdk bot commented Dec 5, 2023

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

8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected

Reviewed-by: dcubed

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

  • 430564c: 8308715: Create a mechanism for Implicitly Declared Class javadoc
  • c8fa758: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS
  • 9e57010: 8315149: Add hsperf counters for CPU time of internal GC threads
  • b0d1450: 8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted
  • acaf2c8: 8318590: JButton ignores margin when painting HTML text
  • d3df3eb: 8294699: Launcher causes lingering busy cursor
  • fddc02e: 8321225: [JVMCI] HotSpotResolvedObjectTypeImpl.isLeafClass shouldn't create strong references
  • 640d7f3: 8314327: Issues with JShell when using "local" execution engine
  • db5613a: 8317288: [macos] java/awt/Window/Grab/GrabTest.java: Press on the outside area didn't cause ungrab
  • b1cb374: 8320349: Simplify FileChooserSymLinkTest.java by using single-window testUI
  • ... and 20 more: https://git.openjdk.org/jdk/compare/155abc576a0212932825485380d4e2a9c7dd2fdc...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 ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 5, 2023
@sspitsyn
Copy link
Contributor Author

sspitsyn commented Dec 5, 2023

/integrate

@openjdk
Copy link

openjdk bot commented Dec 5, 2023

Going to push as commit 905137d.
Since your change was applied there have been 30 commits pushed to the master branch:

  • 430564c: 8308715: Create a mechanism for Implicitly Declared Class javadoc
  • c8fa758: 8320860: add-opens/add-exports require '=' in JAVA_TOOL_OPTIONS
  • 9e57010: 8315149: Add hsperf counters for CPU time of internal GC threads
  • b0d1450: 8321053: Use ByteArrayInputStream.buf directly when parameter of transferTo() is trusted
  • acaf2c8: 8318590: JButton ignores margin when painting HTML text
  • d3df3eb: 8294699: Launcher causes lingering busy cursor
  • fddc02e: 8321225: [JVMCI] HotSpotResolvedObjectTypeImpl.isLeafClass shouldn't create strong references
  • 640d7f3: 8314327: Issues with JShell when using "local" execution engine
  • db5613a: 8317288: [macos] java/awt/Window/Grab/GrabTest.java: Press on the outside area didn't cause ungrab
  • b1cb374: 8320349: Simplify FileChooserSymLinkTest.java by using single-window testUI
  • ... and 20 more: https://git.openjdk.org/jdk/compare/155abc576a0212932825485380d4e2a9c7dd2fdc...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 5, 2023
@openjdk openjdk bot closed this Dec 5, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 5, 2023
@openjdk
Copy link

openjdk bot commented Dec 5, 2023

@sspitsyn Pushed as commit 905137d.

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

@dholmes-ora
Copy link
Member

@dcubed-ojdk I would not consider this a trivial fix at all - the need to add the additional conditions is not at all obvious! And even if they were, that would make this a small/simple fix, not "trivial" as defined for the "one review needed" rule.

Comment on lines +560 to +561
// This optimization is disabled when watchpoint capabilities are present. It is to
// work around a bug with virtual thread frames which can be not deoptimized in time.
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: "This optimization is also disabled when ..."

The phrase "which can be not deoptimized in time." is unclear. Are we racing with deoptimization?

Copy link
Contributor Author

@sspitsyn sspitsyn Dec 6, 2023

Choose a reason for hiding this comment

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

Than you for the suggestion. In fact, I did not finish with the scalability related optimizations and will continue in 23. Will correct this comment as you suggest when there is any chance.

The phrase "which can be not deoptimized in time." is unclear. Are we racing with deoptimization?

Yes, this comment can be not fully correct as you noted.
I do not fully understand optimization vs deoptimization mechanisms. I've already spent a lot of time trying to isolate this deoptimization issue but still need to continue this work in 23. Good news is that it can be reliably reproducible with some extra tweaks but only with a full run of any of the 4-6 tiers. It is not reproducible locally yet. My understanding is that the deoptimization needs some time to happen. We mark frames to deoptimize and they are really deoptimized upon return of the execution control. However, there are some subtle details depending on the execution path. There can be more then one bug in this area.

@sspitsyn
Copy link
Contributor Author

sspitsyn commented Dec 6, 2023

@dcubed-ojdk I would not consider this a trivial fix at all - the need to add the additional conditions is not at all obvious!
And even if they were, that would make this a small/simple fix, not "trivial" as defined for the "one review needed" rule.

Sorry, David. It was kind of obvious to me as this tweak is a work around the field watch related regression.
Of course, it would better to wait for you to finish review but it was not clear if you are going to complete it or not.
As you know, it is very uncomfortable to do last minute push even if it is trivial. :(

@sspitsyn sspitsyn deleted the b12 branch January 23, 2024 01:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
3 participants