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

8258836: JNI local refs exceed capacity getDiagnosticCommandInfo #2130

Closed

Conversation

jerboaa
Copy link
Contributor

@jerboaa jerboaa commented Jan 18, 2021

This patch adds some explicit capacity for local refs. New regression test
fails prior and passes after the patch.

Thoughts?


Progress

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

Issue

  • JDK-8258836: JNI local refs exceed capacity getDiagnosticCommandInfo

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 18, 2021

👋 Welcome back sgehwolf! 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
Copy link

openjdk bot commented Jan 18, 2021

@jerboaa The following labels will be automatically applied to this pull request:

  • jmx
  • 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.

@jerboaa jerboaa force-pushed the JDK-8258836-check-jni-mbeanserver branch from 4de0ff2 to 5b03755 Compare January 18, 2021 14:13
@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org rfr Pull request is ready for review labels Jan 18, 2021
@mlbridge
Copy link

mlbridge bot commented Jan 18, 2021

Webrevs

@jerboaa
Copy link
Contributor Author

jerboaa commented Jan 19, 2021

Thanks for the review! More thoughts?

Copy link
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

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

There are serveral places where EXCEPTION_CHECK_AND_FREE is called while there is a pending PushLocalFrame (or 2). Corresponding PopLocalFrames are needed if there was an exception. The error handling has already proven somewhat tricky and error prone. This just makes it worse. I'm not sure if there is a good solution to this, or just continue adding PopLocalFrames in more places.

@plummercj
Copy link
Contributor

I wonder if you actually have to call PopLocalFrame before returning. I couldn't find anything in the spec that indicates you need too. But there's no indication that you don't either.

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

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

The test looks good. You might want to say /reviewers 2 to keep bots from assuming this approval is enough.

@openjdk
Copy link

openjdk bot commented Jan 21, 2021

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

8258836: JNI local refs exceed capacity getDiagnosticCommandInfo

Reviewed-by: cjplummer, 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 15 new commits pushed to the master branch:

  • 5aca934: 8260304: (se) EPollSelectorImpl wakeup mechanism broken on Linux 32-bit
  • 53fecba: 8258805: Japanese characters not entered by mouse click on Windows 10
  • a887177: 8246788: ZoneRules invariants can be broken
  • 874aef4: 8259707: LDAP channel binding does not work with StartTLS extension
  • c5ad713: 8260250: Duplicate check in DebugInformationRecorder::recorders_frozen
  • bf5e801: 8259922: MethodHandles.collectArguments does not throw IAE if pos is outside the arity range
  • 0ea5862: 8260053: Optimize Tokens' use of Names
  • 18eb6d9: 8255348: NPE in PKIXCertPathValidator event logging code
  • a97f3c1: 8258853: Support separate function declaration and definition with ENABLE_IF-based SFINAE
  • 154e1d6: 8259009: G1 heap summary should be shown in "Heap Parameters" window on HSDB
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/d066f2b06c08d41e47cd7a4b1f047f40df1a5972...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 Pull request is ready to be integrated label Jan 21, 2021
@jerboaa
Copy link
Contributor Author

jerboaa commented Jan 21, 2021

/reviewer 2

@openjdk
Copy link

openjdk bot commented Jan 21, 2021

@jerboaa Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

  • /reviewer credit @openjdk-bot
  • /reviewer credit duke
  • /reviewer credit @user1 @user2

@jerboaa
Copy link
Contributor Author

jerboaa commented Jan 21, 2021

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jan 21, 2021

@jerboaa
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Jan 21, 2021
@jerboaa
Copy link
Contributor Author

jerboaa commented Jan 21, 2021

I wonder if you actually have to call PopLocalFrame before returning. I couldn't find anything in the spec that indicates you need too. But there's no indication that you don't either.

@plummercj OK, I did go over the code once more and changed the EXCEPTION_CHECK_AND_FREE macro slightly so that it now also handles popping of local frames. If the popping isn't needed, so be it, but I guess this is now a more correct version. Leaves pushing/popping of local frames consistent no matter if we short-return or not. Do you think that's OK now?

Copy link
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

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

Getting close:

  • Line 188 where there is a JNU_ThrowOutOfMemoryError needs a PopLocalFrame before it.
  • Copyrights need updating

@jerboaa
Copy link
Contributor Author

jerboaa commented Jan 22, 2021

Getting close:

* Line 188 where there is a JNU_ThrowOutOfMemoryError needs a PopLocalFrame before it.
* Copyrights need updating

Should be fixed now. Thoughts?

@jerboaa
Copy link
Contributor Author

jerboaa commented Jan 22, 2021

@plummercj Could you look at the latest version of this please? Much appreciated.

Copy link
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

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

Looks good.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 22, 2021
@jerboaa
Copy link
Contributor Author

jerboaa commented Jan 25, 2021

Thanks for the review!

@jerboaa
Copy link
Contributor Author

jerboaa commented Jan 25, 2021

/integrate

@openjdk openjdk bot closed this Jan 25, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 25, 2021
@openjdk
Copy link

openjdk bot commented Jan 25, 2021

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

Your commit was automatically rebased without conflicts.

Pushed as commit af155fc.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated jmx jmx-dev@openjdk.org serviceability serviceability-dev@openjdk.org
4 participants