Re: RFR: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null [v2]
> Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of > another possible cause of a NullPointerException - when the exception > supplying function returns a null result. > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change requires CSR request > [JDK-8354953](https://bugs.openjdk.org/browse/JDK-8354953) to be approved > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > ### Error > ⚠️ The pull request body must not be empty. > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905` \ > `$ git checkout pull/23905` > > Update a local copy of the PR: \ > `$ git checkout pull/23905` \ > `$ git pull https://git.openjdk.org/jdk.git pull/23905/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 23905` > > View PR using the GUI difftool: \ > `$ git pr show -t 23905` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/23905.diff";>https://git.openjdk.org/jdk/pull/23905.diff > > simon has updated the pull request incrementally with one additional commit since the last revision: 8350542: update orElseThrow JavaDoc from CSR specification. - Changes: - all: https://git.openjdk.org/jdk/pull/23905/files - new: https://git.openjdk.org/jdk/pull/23905/files/53df7aff..32448774 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=23905&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=23905&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23905.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905 PR: https://git.openjdk.org/jdk/pull/23905
Re: RFR: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null
On Mon, 28 Apr 2025 20:04:22 GMT, Chen Liang wrote: >> Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of >> another possible cause of a NullPointerException - when the exception >> supplying function returns a null result. >> - >> ### Progress >> - [ ] Change must be properly reviewed (1 review required, with at least 1 >> [Reviewer](https://openjdk.org/bylaws#reviewer)) >> - [x] Change requires CSR request >> [JDK-8354953](https://bugs.openjdk.org/browse/JDK-8354953) to be approved >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> >> ### Error >> ⚠️ The pull request body must not be empty. >> >> >> >> ### Reviewing >> Using git >> >> Checkout this PR locally: \ >> `$ git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905` \ >> `$ git checkout pull/23905` >> >> Update a local copy of the PR: \ >> `$ git checkout pull/23905` \ >> `$ git pull https://git.openjdk.org/jdk.git pull/23905/head` >> >> >> Using Skara CLI tools >> >> Checkout this PR locally: \ >> `$ git pr checkout 23905` >> >> View PR using the GUI difftool: \ >> `$ git pr show -t 23905` >> >> >> Using diff file >> >> Download this PR as a diff file: \ >> > href="https://git.openjdk.org/jdk/pull/23905.diff";>https://git.openjdk.org/jdk/pull/23905.diff >> >> > > Also please add some of your own text to the pull request body, like the > first sentence of the issue report: > >> Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of >> another possible cause of a NullPointerException - when the exception >> supplying function returns a null result. > > Once you have pushed the changes, Chris or I may review this for integration. @liach @RogerRiggs Sorry, I forgot a part of the specification! Now we are ready! - PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2836857568
Re: RFR: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null
On Wed, 26 Mar 2025 00:21:38 GMT, Chen Liang wrote: >> Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of >> another possible cause of a NullPointerException - when the exception >> supplying function returns a null result. >> - >> ### Progress >> - [ ] Change must be properly reviewed (1 review required, with at least 1 >> [Reviewer](https://openjdk.org/bylaws#reviewer)) >> - [x] Change requires CSR request >> [JDK-8354953](https://bugs.openjdk.org/browse/JDK-8354953) to be approved >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> >> ### Error >> ⚠️ The pull request body must not be empty. >> >> >> >> ### Reviewing >> Using git >> >> Checkout this PR locally: \ >> `$ git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905` \ >> `$ git checkout pull/23905` >> >> Update a local copy of the PR: \ >> `$ git checkout pull/23905` \ >> `$ git pull https://git.openjdk.org/jdk.git pull/23905/head` >> >> >> Using Skara CLI tools >> >> Checkout this PR locally: \ >> `$ git pr checkout 23905` >> >> View PR using the GUI difftool: \ >> `$ git pr show -t 23905` >> >> >> Using diff file >> >> Download this PR as a diff file: \ >> > href="https://git.openjdk.org/jdk/pull/23905.diff";>https://git.openjdk.org/jdk/pull/23905.diff >> >> > > You can comment > >> /oca verify > > and see if it's handled. @liach oca appears to be not recognized by bot > Oops sorry, it should be > >> /signed @liach I think @robilad didn't verified my account. How should I proceed? > I think Dalibor might be busy - You might email him again on the progress. Hello @liach! I'm going to email @robilad. @liach and @RealCLanger what's the next step to merge this into master? > Also please add some of your own text to the pull request body, like the > first sentence of the issue report: > >> Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of >> another possible cause of a NullPointerException - when the exception >> supplying function returns a null result. > > Once you have pushed the changes, Chris or I may review this for integration. @liach now the PR is ready for review. We're almost there! 🔝 - PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2752863934 PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2759919926 PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2763595208 PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2815432182 PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2836690616
Re: RFR: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null
On Mon, 28 Apr 2025 20:04:22 GMT, Chen Liang wrote: >> Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of >> another possible cause of a NullPointerException - when the exception >> supplying function returns a null result. >> - >> ### Progress >> - [ ] Change must be properly reviewed (1 review required, with at least 1 >> [Reviewer](https://openjdk.org/bylaws#reviewer)) >> - [x] Change requires CSR request >> [JDK-8354953](https://bugs.openjdk.org/browse/JDK-8354953) to be approved >> - [x] Change must not contain extraneous whitespace >> - [x] Commit message must refer to an issue >> >> ### Error >> ⚠️ The pull request body must not be empty. >> >> >> >> ### Reviewing >> Using git >> >> Checkout this PR locally: \ >> `$ git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905` \ >> `$ git checkout pull/23905` >> >> Update a local copy of the PR: \ >> `$ git checkout pull/23905` \ >> `$ git pull https://git.openjdk.org/jdk.git pull/23905/head` >> >> >> Using Skara CLI tools >> >> Checkout this PR locally: \ >> `$ git pr checkout 23905` >> >> View PR using the GUI difftool: \ >> `$ git pr show -t 23905` >> >> >> Using diff file >> >> Download this PR as a diff file: \ >> > href="https://git.openjdk.org/jdk/pull/23905.diff";>https://git.openjdk.org/jdk/pull/23905.diff >> >> > > Also please add some of your own text to the pull request body, like the > first sentence of the issue report: > >> Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of >> another possible cause of a NullPointerException - when the exception >> supplying function returns a null result. > > Once you have pushed the changes, Chris or I may review this for integration. @liach Can't I integrate? - PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2837084279
Integrated: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null
On Tue, 4 Mar 2025 16:34:19 GMT, simon wrote: > Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of > another possible cause of a NullPointerException - when the exception > supplying function returns a null result. > - > ### Progress > - [x] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change requires CSR request > [JDK-8354953](https://bugs.openjdk.org/browse/JDK-8354953) to be approved > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > ### Error > ⚠️ The pull request body must not be empty. > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905` \ > `$ git checkout pull/23905` > > Update a local copy of the PR: \ > `$ git checkout pull/23905` \ > `$ git pull https://git.openjdk.org/jdk.git pull/23905/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 23905` > > View PR using the GUI difftool: \ > `$ git pr show -t 23905` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/23905.diff";>https://git.openjdk.org/jdk/pull/23905.diff > > This pull request has now been integrated. Changeset: 44374a57 Author:Gustavo Simon Committer: Christoph Langer URL: https://git.openjdk.org/jdk/commit/44374a572096fc98b390ab2cb9063d832e110020 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null Reviewed-by: liach, clanger - PR: https://git.openjdk.org/jdk/pull/23905
Re: RFR: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null
On Tue, 4 Mar 2025 16:34:19 GMT, simon wrote: > Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of > another possible cause of a NullPointerException - when the exception > supplying function returns a null result. > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change requires CSR request > [JDK-8354953](https://bugs.openjdk.org/browse/JDK-8354953) to be approved > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > ### Error > ⚠️ The pull request body must not be empty. > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905` \ > `$ git checkout pull/23905` > > Update a local copy of the PR: \ > `$ git checkout pull/23905` \ > `$ git pull https://git.openjdk.org/jdk.git pull/23905/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 23905` > > View PR using the GUI difftool: \ > `$ git pr show -t 23905` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/23905.diff";>https://git.openjdk.org/jdk/pull/23905.diff > > @robilad As you asked, I have already sent you an e-mail verify my OCA. I will need for this PR. Cheers, looking forward! 😀 The fastest GH's reply was made now! hahaha Dears, I forgot that Dalibor told me that my OCA verification was not found. So I have just sent it right now. @robilad now you can verify my OCA! Thank you all for the help. Let's wait for the OCA verify process. Happy to contribute to Java. 😀 - PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2698302224 PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2752866613 PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2815972016 PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2815989924
RFR: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null
Javadoc for java.util.Optional.orElseThrow(Supplier) misses mentioning of another possible cause of a NullPointerException - when the exception supplying function returns a null result. - ### Progress - [ ] Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer)) - [x] Change requires CSR request [JDK-8354953](https://bugs.openjdk.org/browse/JDK-8354953) to be approved - [x] Change must not contain extraneous whitespace - [x] Commit message must refer to an issue ### Error ⚠️ The pull request body must not be empty. ### Reviewing Using git Checkout this PR locally: \ `$ git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905` \ `$ git checkout pull/23905` Update a local copy of the PR: \ `$ git checkout pull/23905` \ `$ git pull https://git.openjdk.org/jdk.git pull/23905/head` Using Skara CLI tools Checkout this PR locally: \ `$ git pr checkout 23905` View PR using the GUI difftool: \ `$ git pr show -t 23905` Using diff file Download this PR as a diff file: \ https://git.openjdk.org/jdk/pull/23905.diff";>https://git.openjdk.org/jdk/pull/23905.diff - Commit messages: - 8350542: fix extra trailing space. - 8350542: update orElseThrow JavaDoc from CSR specification. - 8350542: fix extra trailing space. - 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null Changes: https://git.openjdk.org/jdk/pull/23905/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23905&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8350542 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/23905.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23905/head:pull/23905 PR: https://git.openjdk.org/jdk/pull/23905
Re: RFR: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null
On Mon, 28 Apr 2025 14:32:59 GMT, Christoph Langer wrote: >> Thank you all for the help. Let's wait for the OCA verify process. Happy to >> contribute to Java. 😀 > > @gustavosimon Now the oca seems to be approved and the CSR is progressing. > Can you update your change to match the exact specification of the CSR > (wording). Please also enable GitHub actions in your fork > (https://github.com/gustavosimon/jdk/actions) and merge master to get a > current GHA test run (although this change is doc only and no regression is > expected). @RealCLanger I will do it today. The CSR seems to be approved now - then I just need to push the change to merge it. - PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2836313467
Re: RFR: 8350542: Optional.orElseThrow(Supplier) does not specify behavior when supplier returns null
On Tue, 4 Mar 2025 16:45:22 GMT, simon wrote: > @robilad As you asked, I have already sent you an e-mail verify my OCA. I > will need for this PR. Cheers, looking forward! 😀 @robilad Any luck into this matter? - PR Comment: https://git.openjdk.org/jdk/pull/23905#issuecomment-2752777225
Re: RFR: 8348828: Windows dll loading now resolves symlinks
On Mon, 28 Apr 2025 17:44:31 GMT, Benjamin Peterson wrote: >> This issue has the "oca" label so we cannot engage, sorry. > >> This issue has the "oca" label so we cannot engage, sorry. > > Ah, sorry. I only asked because I saw a colleague of yours helping out with > OCA verification over at > https://github.com/openjdk/jdk/pull/23905#issuecomment-2762865417. Also, the > [OCA FAQ](https://oca.opensource.oracle.com/?ojr=faq) suggests to "please > reach out to project maintainers (e.g., through comments on your GitHub PR, > Slack channels, or mailing lists)". Any breadcrumbs you can lay for me to > escape this forest are appreciated! Hello @benjaminp! I think you should wait for the OCA process — it's very fast. I'll just ping @robilad, as he might be able to help. - PR Comment: https://git.openjdk.org/jdk/pull/24694#issuecomment-2836349678
RFR: 8360122: Fix java.sql\Connection.java indentation
8360122: Refine formatting of Connection.java interface - ### Progress - [ ] Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer)) - [x] Change must not contain extraneous whitespace - [x] Commit message must refer to an issue ### Error ⚠️ The pull request body must not be empty. ### Reviewing Using git Checkout this PR locally: \ `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ `$ git checkout pull/25925` Update a local copy of the PR: \ `$ git checkout pull/25925` \ `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` Using Skara CLI tools Checkout this PR locally: \ `$ git pr checkout 25925` View PR using the GUI difftool: \ `$ git pr show -t 25925` Using diff file Download this PR as a diff file: \ https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff - Commit messages: - 8360122: Refine formatting of Connection.java interface Changes: https://git.openjdk.org/jdk/pull/25925/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8360122 Stats: 253 lines in 1 file changed: 46 ins; 64 del; 143 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8360122: Fix java.sql\Connection.java indentation
On Sun, 22 Jun 2025 01:13:26 GMT, simon wrote: > 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > @liach Can you review this PR ou assign someone that might be able to review? - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2995972169
Re: RFR: 8360122: Fix java.sql\Connection.java indentation
On Mon, 23 Jun 2025 14:51:44 GMT, Chen Liang wrote: > FYI people don't usually review on weekends (you opened this PR in a weekend) > or holidays. This PR is sent to core-libs-dev list so people will eventually > see and review it. Great, @liach! Thanks for the information! 😄 - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2996843586
Re: RFR: 8360122: Fix java.sql\Connection.java indentation
On Mon, 23 Jun 2025 14:50:24 GMT, Roger Riggs wrote: > The indentation fixes look ok. The reformatting of declarations seem > unnecessary and create lines that are longer than desirable to be able to do > side-by-side compares (100 char max). Generally, avoid just asking the IDE to > re-format the source, it leads to unnecessary changes. I'm going to undo the formatting changes where lines exceed 100 characters. Actually, I didn’t ask the IDE to reformat; I only suggested this formatting to improve readability. However, it can make comparisons in the diff view more difficult. @RogerRiggs Thanks for the review! 😃 - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2996839307
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 14:50:24 GMT, Roger Riggs wrote: >> simon has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 8360122: refactor code formatting to enforce 100 chars line length limit >> for improved readability > > The indentation fixes look ok. > The reformatting of declarations seem unnecessary and create lines that are > longer than desirable to be able to do side-by-side compares (100 char max). > Generally, avoid just asking the IDE to re-format the source, it leads to > unnecessary changes. @RogerRiggs I have just commited the changes to not exceed the 100 chars limit. - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-2997252128
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
> 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > simon has updated the pull request incrementally with one additional commit since the last revision: 8360122: refactor code formatting to enforce 100 chars line length limit for improved readability - Changes: - all: https://git.openjdk.org/jdk/pull/25925/files - new: https://git.openjdk.org/jdk/pull/25925/files/4310eaf9..3a7d0b5a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=00-01 Stats: 12 lines in 1 file changed: 8 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v4]
> 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > simon has updated the pull request incrementally with one additional commit since the last revision: 8360122: revert reformatting method signatures - Changes: - all: https://git.openjdk.org/jdk/pull/25925/files - new: https://git.openjdk.org/jdk/pull/25925/files/6d43550b..21696cf7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=02-03 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 17:35:50 GMT, Roger Riggs wrote:
>> simon has updated the pull request incrementally with one additional commit
>> since the last revision:
>>
>> 8360122: refactor code formatting to enforce 100 chars line length limit
>> for improved readability
>
> src/java.sql/share/classes/java/sql/Connection.java line 1597:
>
>> 1595: default boolean setShardingKeyIfValid(ShardingKey shardingKey,
>> 1596: ShardingKey superShardingKey,
>> 1597: int timeout) throws
>> SQLException {
>
> I haven't quite identified your preferred formatting style.
> From previous reformattings with multiple arguments, the exception was placed
> on a line by itself.
>
> (Generally, we defer to the original/previous author's choice of line breaks
> and formatting.
> Choosing only to reformat to use a consistent style within each file/class or
> package.)
>
> The issue title and PR title should be a bit more general, its more than just
> indentation cleanup.
@RogerRiggs My preferred formatting style is like this:
default boolean setShardingKeyIfValid(ShardingKey shardingKey,
ShardingKey superShardingKey,
int timeout) throws SQLException {
But as you said, there is previous reformattings that does not follow it. If
you agree, I will fix them. What do you thing about it?
About the issue title and PR title, we can make it more general like:
"Improve Readability and Maintainability of Connection.java"
What your suggestion on it?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2162305006
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v3]
> 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > simon has updated the pull request incrementally with one additional commit since the last revision: 8360122: revert reformatting method signatures - Changes: - all: https://git.openjdk.org/jdk/pull/25925/files - new: https://git.openjdk.org/jdk/pull/25925/files/3a7d0b5a..6d43550b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=01-02 Stats: 29 lines in 1 file changed: 8 ins; 5 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 19:54:24 GMT, Roger Riggs wrote:
>> @RogerRiggs My preferred formatting style is like this:
>>
>>
>> default boolean setShardingKeyIfValid(ShardingKey shardingKey,
>> ShardingKey superShardingKey,
>> int timeout) throws SQLException {
>>
>>
>> But as you said, there is previous reformattings that does not follow it. If
>> you agree, I will fix them. What do you thing about it?
>>
>> About the issue title and PR title, we can make it more general like:
>>
>> "Improve Readability and Maintainability of Connection.java"
>>
>> What your suggestion on it?
>
> I would backout the changes to signatures, fixing the bad indentation and
> leave it at that.
> Lance is one of the long time maintainers of the SQL classes and will want to
> take a look.
Great! I will do that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2162442176
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v4]
On Wed, 2 Jul 2025 14:37:04 GMT, Lance Andersen wrote: >> simon has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 8360122: revert reformatting method signatures > > src/java.sql/share/classes/java/sql/Connection.java line 303: > >> 301: >> //== >> 302: // Advanced features: >> 303: > > Why was this removed as this is not format related It was just a suggestion, but it’s not really related to the main change. I can remove it if you think it’s appropriate - PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2181347375
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v4]
On Wed, 2 Jul 2025 15:14:47 GMT, Lance Andersen wrote: >> simon has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 8360122: revert reformatting method signatures > > src/java.sql/share/classes/java/sql/Connection.java line 1175: > >> 1173: */ >> 1174: void setClientInfo(String name, String value) >> 1175: throws SQLClientInfoException; > > The throws clause could be moved up Fixed > src/java.sql/share/classes/java/sql/Connection.java line 1208: > >> 1206: */ >> 1207: void setClientInfo(Properties properties) >> 1208: throws SQLClientInfoException; > > The throws clause could be moved up Fixed > src/java.sql/share/classes/java/sql/Connection.java line 1232: > >> 1230: * @see java.sql.DatabaseMetaData#getClientInfoProperties >> 1231: */ >> 1232: String getClientInfo(String name) > > the throws clause could be moved up Fixed > src/java.sql/share/classes/java/sql/Connection.java line 1251: > >> 1249: */ >> 1250: Properties getClientInfo() >> 1251: throws SQLException; > > The throws clause could be moved up Fixed - PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2181380031 PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2181379915 PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2181379825 PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2181379234
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v5]
> 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > simon has updated the pull request incrementally with one additional commit since the last revision: 8360122: refactored method throws clauses to be inline - Changes: - all: https://git.openjdk.org/jdk/pull/25925/files - new: https://git.openjdk.org/jdk/pull/25925/files/21696cf7..c14699a8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=03-04 Stats: 20 lines in 1 file changed: 0 ins; 10 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 17:45:02 GMT, Lance Andersen wrote: >> simon has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 8360122: refactor code formatting to enforce 100 chars line length limit >> for improved readability > > I might not have time this week to look at this as I have some other > commitments. I do think we will want to narrow the changes being proposed @LanceAndersen I moved up the throws clauses of other methods in Connection.java, as @RogerRiggs suggested, to maintain consistent formatting in this class. - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-3030228941
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v2]
On Mon, 23 Jun 2025 17:45:02 GMT, Lance Andersen wrote: >> simon has updated the pull request incrementally with one additional commit >> since the last revision: >> >> 8360122: refactor code formatting to enforce 100 chars line length limit >> for improved readability > > I might not have time this week to look at this as I have some other > commitments. I do think we will want to narrow the changes being proposed @LanceAndersen Hope you're doing well! Could you review the change so that I can apply the necessary improvements and proceed with the integration? - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-3024321438
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v6]
> 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > simon has updated the pull request incrementally with one additional commit since the last revision: 8360122: reversed wrongful removal of comments - Changes: - all: https://git.openjdk.org/jdk/pull/25925/files - new: https://git.openjdk.org/jdk/pull/25925/files/c14699a8..69c84daa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25925&range=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/25925.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925 PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v3]
On Tue, 8 Jul 2025 18:53:08 GMT, Lance Andersen wrote: >> It was just a suggestion, but it’s not really related to the main change. I >> can undo it if you think it’s appropriate > > Please revert this change Fixed - PR Review Comment: https://git.openjdk.org/jdk/pull/25925#discussion_r2193408909
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v6]
On Wed, 9 Jul 2025 19:38:21 GMT, Chen Liang wrote: >>> The changes are OK. >> >> Great, @LanceAndersen! Thanks for reviewing. 😃 >> >> Can you integrate it when you think it is appropriate? I guess I do not have >> permission to do it. > >> Can you integrate it when you think it is appropriate? I guess I do not have >> permission to do it. > > They are done with GitHub comments parsed by bots, so if there is a line: > >> /integrate > > in a comment from you, the bot will integrate this patch (by marking this > ready for a committer sponsor) Got it, @liach! Thanks 😄! I'm doing right now! - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-3053794210
Re: RFR: 8360122: Fix java.sql\Connection.java indentation [v6]
On Wed, 9 Jul 2025 18:59:49 GMT, Lance Andersen wrote: > The changes are OK. Great, @LanceAndersen! Thanks for reviewing. 😃 Can you integrate it when you think it is appropriate? I guess I do not have permission to do it. - PR Comment: https://git.openjdk.org/jdk/pull/25925#issuecomment-3053780918
Integrated: 8360122: Fix java.sql\Connection.java indentation
On Sun, 22 Jun 2025 01:13:26 GMT, simon wrote: > 8360122: Refine formatting of Connection.java interface > > - > ### Progress > - [x] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/25925/head:pull/25925` \ > `$ git checkout pull/25925` > > Update a local copy of the PR: \ > `$ git checkout pull/25925` \ > `$ git pull https://git.openjdk.org/jdk.git pull/25925/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 25925` > > View PR using the GUI difftool: \ > `$ git pr show -t 25925` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/25925.diff";>https://git.openjdk.org/jdk/pull/25925.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/25925#issuecomment-2993856738) > This pull request has now been integrated. Changeset: 0f7808f3 Author:Gustavo Simon Committer: Chen Liang URL: https://git.openjdk.org/jdk/commit/0f7808f333556eed2a1381e5f9f67765ec3694f1 Stats: 238 lines in 1 file changed: 46 ins; 59 del; 133 mod 8360122: Fix java.sql\Connection.java indentation Reviewed-by: liach, lancea - PR: https://git.openjdk.org/jdk/pull/25925
Re: RFR: 8355652: Parse ClassFileFormatVersion from ClassFileVersion
On Sun, 20 Jul 2025 23:36:53 GMT, Chen Liang wrote: > > I think we should create some unit tests for this API and classes involved. > > Where do you think that is the most appropriate location in the project to > > do it? > > Unit tests for `java.lang.classfile` reside in `test/jdk/jdk/classfile`. They > can be run with `make test TEST=jdk/jdk/classfile`. Great! Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/26406#issuecomment-3094864531
Re: RFR: 8355652: Parse ClassFileFormatVersion from ClassFileVersion
On Sun, 20 Jul 2025 22:19:07 GMT, simon wrote: > 8355652: add new method to return ClassFileFormatVersion from > ClassFileVersion. > - > ### Progress > - [ ] Change must be properly reviewed (1 review required, with at least 1 > [Reviewer](https://openjdk.org/bylaws#reviewer)) > - [x] Change must not contain extraneous whitespace > - [x] Commit message must refer to an issue > > > > ### Reviewing > Using git > > Checkout this PR locally: \ > `$ git fetch https://git.openjdk.org/jdk.git pull/26406/head:pull/26406` \ > `$ git checkout pull/26406` > > Update a local copy of the PR: \ > `$ git checkout pull/26406` \ > `$ git pull https://git.openjdk.org/jdk.git pull/26406/head` > > > Using Skara CLI tools > > Checkout this PR locally: \ > `$ git pr checkout 26406` > > View PR using the GUI difftool: \ > `$ git pr show -t 26406` > > > Using diff file > > Download this PR as a diff file: \ > href="https://git.openjdk.org/jdk/pull/26406.diff";>https://git.openjdk.org/jdk/pull/26406.diff > > > Using Webrev > > [Link to Webrev > Comment](https://git.openjdk.org/jdk/pull/26406#issuecomment-3094832141) > @liach I know that I'm commenting on a weekend but it's to you see tomorrow. I'm commiting a 1.0 version for this implementation - to get your feedback about I think we should create some unit tests for this API and classes involved. Where do you think that is the most apprpriate location in the project to do it? - PR Comment: https://git.openjdk.org/jdk/pull/26406#issuecomment-3094860148
RFR: 8355652: Parse ClassFileFormatVersion from ClassFileVersion
8355652: add new method to return ClassFileFormatVersion from ClassFileVersion. - ### Progress - [ ] Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer)) - [x] Change must not contain extraneous whitespace - [x] Commit message must refer to an issue ### Error ⚠️ The pull request body must not be empty. ### Integration blocker ⚠️ Title mismatch between PR and JBS for issue [JDK-8355652](https://bugs.openjdk.org/browse/JDK-8355652) ### Warning ⚠️ Found trailing period in issue title for `8355652: add new method to return ClassFileFormatVersion from ClassFileVersion.` Found leading lowercase letter in issue title for `8355652: add new method to return ClassFileFormatVersion from ClassFileVersion.` ### Reviewing Using git Checkout this PR locally: \ `$ git fetch https://git.openjdk.org/jdk.git pull/26406/head:pull/26406` \ `$ git checkout pull/26406` Update a local copy of the PR: \ `$ git checkout pull/26406` \ `$ git pull https://git.openjdk.org/jdk.git pull/26406/head` Using Skara CLI tools Checkout this PR locally: \ `$ git pr checkout 26406` View PR using the GUI difftool: \ `$ git pr show -t 26406` Using diff file Download this PR as a diff file: \ https://git.openjdk.org/jdk/pull/26406.diff";>https://git.openjdk.org/jdk/pull/26406.diff - Commit messages: - 8355652: add new method to return ClassFileFormatVersion from ClassFileVersion. Changes: https://git.openjdk.org/jdk/pull/26406/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26406&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8355652 Stats: 17 lines in 2 files changed: 17 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/26406.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26406/head:pull/26406 PR: https://git.openjdk.org/jdk/pull/26406
Re: RFR: 8355652: Parse ClassFileFormatVersion from ClassFileVersion
On Mon, 21 Jul 2025 00:11:20 GMT, Chen Liang wrote:
>> 8355652: add new method to return ClassFileFormatVersion from
>> ClassFileVersion.
>> -
>> ### Progress
>> - [ ] Change must be properly reviewed (1 review required, with at least 1
>> [Reviewer](https://openjdk.org/bylaws#reviewer))
>> - [x] Change must not contain extraneous whitespace
>> - [x] Commit message must refer to an issue
>>
>>
>>
>> ### Reviewing
>> Using git
>>
>> Checkout this PR locally: \
>> `$ git fetch https://git.openjdk.org/jdk.git pull/26406/head:pull/26406` \
>> `$ git checkout pull/26406`
>>
>> Update a local copy of the PR: \
>> `$ git checkout pull/26406` \
>> `$ git pull https://git.openjdk.org/jdk.git pull/26406/head`
>>
>>
>> Using Skara CLI tools
>>
>> Checkout this PR locally: \
>> `$ git pr checkout 26406`
>>
>> View PR using the GUI difftool: \
>> `$ git pr show -t 26406`
>>
>>
>> Using diff file
>>
>> Download this PR as a diff file: \
>> > href="https://git.openjdk.org/jdk/pull/26406.diff";>https://git.openjdk.org/jdk/pull/26406.diff
>>
>>
>> Using Webrev
>>
>> [Link to Webrev
>> Comment](https://git.openjdk.org/jdk/pull/26406#issuecomment-3094832141)
>>
>
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassFileVersionImpl.java
> line 54:
>
>> 52: public Optional formatVersion() {
>> 53: try {
>> 54: return
>> Optional.of(ClassFileFormatVersion.fromMajor(majorVersion));
>
> You should check the minor version to be 0 for major versions 54 and later.
Do you mean something like this?
@Override
public Optional formatVersion() {
if (majorVersion >= 54 || minorVersion == 0) {
return Optional.empty();
}
try {
return Optional.of(ClassFileFormatVersion.fromMajor(majorVersion));
} catch (IllegalArgumentException e) {
return Optional.empty();
}
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/26406#discussion_r2218024287
RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation
This PR addresses intermittent failures in jtreg GC stress tests. The failures occur under these conditions: 1. Using a libgraal build with assertions enabled as the top tier JIT compiler. Such a libgraal build will cause a VM exit if an assertion or GraalError occurs in a compiler thread (as this catches more errors in testing). 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`) to a routine that performs a HotSpot heap allocation that fails. 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as described in 1. An OOME thrown in these specific conditions should not exit the VM as it not related to an OOME in the app or test. Instead, the failure should be treated as a bailout and the libgraal compiler should continue. To accomplish this, libgraal needs to be able to distinguish a GraalError caused by an OOME. This PR modifies the exception translation code to make this possible. - Commit messages: - improved exception translation between HotSpot and libgraal heaps Changes: https://git.openjdk.org/jdk/pull/20083/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20083&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8335553 Stats: 84 lines in 5 files changed: 50 ins; 22 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/20083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20083/head:pull/20083 PR: https://git.openjdk.org/jdk/pull/20083
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation
On Mon, 8 Jul 2024 19:01:05 GMT, Doug Simon wrote:
> This PR addresses intermittent failures in jtreg GC stress tests. The
> failures occur under these conditions:
> 1. Using a libgraal build with assertions enabled as the top tier JIT
> compiler. Such a libgraal build will cause a VM exit if an assertion or
> GraalError occurs in a compiler thread (as this catches more errors in
> testing).
> 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`)
> to a routine that performs a HotSpot heap allocation that fails.
> 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as
> described in 1.
>
> An OOME thrown in these specific conditions should not exit the VM as it not
> related to an OOME in the app or test. Instead, the failure should be treated
> as a bailout and the libgraal compiler should continue.
>
> To accomplish this, libgraal needs to be able to distinguish a GraalError
> caused by an OOME. This PR modifies the exception translation code to make
> this possible.
src/hotspot/share/utilities/exceptions.cpp line 114:
> 112: #endif // ASSERT
> 113:
> 114: if (h_exception.is_null() && !thread->can_call_java()) {
There is no reason to replace an existing exception object with a dummy
exception object in the case where the current thread cannot call into Java.
Since the exception object already exists, no Java call is necessary.
This change is necessary to allow the libgraal exception translation mechanism
to know that an OOME is being translated.
src/hotspot/share/utilities/exceptions.cpp line 208:
> 206: Handle h_loader, Handle
> h_protection_domain) {
> 207: // Check for special boot-strapping/compiler-thread handling
> 208: if (special_exception(thread, file, line, h_cause)) return;
This fixes a long standing bug where `special_exception` is being queried with
the *cause* of the exception being thrown instead of the *name* of the
exception being thrown.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1669153819
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1669148553
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
> This PR addresses intermittent failures in jtreg GC stress tests. The > failures occur under these conditions: > 1. Using a libgraal build with assertions enabled as the top tier JIT > compiler. Such a libgraal build will cause a VM exit if an assertion or > GraalError occurs in a compiler thread (as this catches more errors in > testing). > 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`) > to a routine that performs a HotSpot heap allocation that fails. > 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as > described in 1. > > An OOME thrown in these specific conditions should not exit the VM as it not > related to an OOME in the app or test. Instead, the failure should be treated > as a bailout and the libgraal compiler should continue. > > To accomplish this, libgraal needs to be able to distinguish a GraalError > caused by an OOME. This PR modifies the exception translation code to make > this possible. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: fixed TestTranslatedException - Changes: - all: https://git.openjdk.org/jdk/pull/20083/files - new: https://git.openjdk.org/jdk/pull/20083/files/ff544be3..aa32491c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20083&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20083&range=00-01 Stats: 19 lines in 2 files changed: 12 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/20083.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20083/head:pull/20083 PR: https://git.openjdk.org/jdk/pull/20083
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Tue, 9 Jul 2024 14:37:47 GMT, Yudi Zheng wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed TestTranslatedException
>
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 782:
>
>> 780: while (true) {
>> 781: // Trigger an OutOfMemoryError
>> 782: objArrayOop next = oopFactory::new_objectArray(0x7FFF,
>> CHECK_NULL);
>
> Shall we check for pending exception and break here?
The `CHECK_NULL` macro effectively does that.
> test/jdk/jdk/internal/vm/TestTranslatedException.java line 167:
>
>> 165: private static void assertThrowableEquals(Throwable originalIn,
>> Throwable decodedIn) {
>> 166: Throwable original = originalIn;
>> 167: Throwable decoded = decodedIn;
>
> What is the purpose of this renaming?
So that the printing down the bottom of this message shows the complete
throwable, not just the cause on which the comparison failed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1670656254
PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1670654917
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Wed, 10 Jul 2024 06:19:52 GMT, David Holmes wrote: >> Though I see this is inconsistent with `Exceptions::_throw_msg_cause` > > Okay I think I see how the logic works. If we were going to abort we would > never reach `_throw_cause` as the initial `_throw` would have exited. But for > the `!thread->can_call_Java()` case the original `_throw` would replace the > intended real exception with the dummy `VM_exception()`, which is then > "caught" and we try to replace with a more specific exception to be thrown > via `throw_cause`, which will again replace whichever exception is requested > with the dummy `VM_exception()` - so the end result is we will throw the > dummy regardless of whether the cause or wrapping exception is specified. So > your fix here makes sense. Great. Would you mind approving this PR as this is the only non-JVMCI file changed. - PR Review Comment: https://git.openjdk.org/jdk/pull/20083#discussion_r1672520461
Re: RFR: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation [v2]
On Tue, 9 Jul 2024 13:46:46 GMT, Doug Simon wrote: >> This PR addresses intermittent failures in jtreg GC stress tests. The >> failures occur under these conditions: >> 1. Using a libgraal build with assertions enabled as the top tier JIT >> compiler. Such a libgraal build will cause a VM exit if an assertion or >> GraalError occurs in a compiler thread (as this catches more errors in >> testing). >> 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`) >> to a routine that performs a HotSpot heap allocation that fails. >> 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as >> described in 1. >> >> An OOME thrown in these specific conditions should not exit the VM as it not >> related to an OOME in the app or test. Instead, the failure should be >> treated as a bailout and the libgraal compiler should continue. >> >> To accomplish this, libgraal needs to be able to distinguish a GraalError >> caused by an OOME. This PR modifies the exception translation code to make >> this possible. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fixed TestTranslatedException Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/20083#issuecomment-187035
Integrated: 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation
On Mon, 8 Jul 2024 19:01:05 GMT, Doug Simon wrote: > This PR addresses intermittent failures in jtreg GC stress tests. The > failures occur under these conditions: > 1. Using a libgraal build with assertions enabled as the top tier JIT > compiler. Such a libgraal build will cause a VM exit if an assertion or > GraalError occurs in a compiler thread (as this catches more errors in > testing). > 2. A libgraal compiler thread makes a call into the VM (via `CompilerToVM`) > to a routine that performs a HotSpot heap allocation that fails. > 3. The resulting OOME is wrapped in a GraalError, causing the VM to exit as > described in 1. > > An OOME thrown in these specific conditions should not exit the VM as it not > related to an OOME in the app or test. Instead, the failure should be treated > as a bailout and the libgraal compiler should continue. > > To accomplish this, libgraal needs to be able to distinguish a GraalError > caused by an OOME. This PR modifies the exception translation code to make > this possible. This pull request has now been integrated. Changeset: cf940e13 Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/cf940e139a76e5aabd52379b8a87065d82b2284c Stats: 103 lines in 6 files changed: 62 ins; 22 del; 19 mod 8335553: [Graal] Compiler thread calls into jdk.internal.vm.VMSupport.decodeAndThrowThrowable and crashes in OOM situation Reviewed-by: yzheng, never, dholmes - PR: https://git.openjdk.org/jdk/pull/20083
Re: RFR: 8335480: Only deoptimize threads if needed when closing shared arena [v2]
On Fri, 12 Jul 2024 20:59:26 GMT, Jorn Vernee wrote: >> This PR limits the number of cases in which we deoptimize frames when >> closing a shared Arena. The initial intent of this was to improve the >> performance of shared arena closure in cases where a lot of threads are >> accessing and closing shared arenas at the same time (see attached >> benchmark), but unfortunately even disabling deoptimization altogether does >> not have a great effect on that benchmark. >> >> Nevertheless, I think the extra logging/testing/benchmark code, and comments >> I've written, together with reducing the number of cases where we deoptimize >> (which makes it clearer exactly why we need to deoptimize in the first >> place), will be useful going forward. So, I've a create this PR out of them. >> >> In this PR: >> - Deoptimizing is now only done in cases where it's needed, instead of >> always. Which is in cases where we are not inside an `@Scoped` method, but >> are inside a compiled frame that has a scoped access somewhere inside of it. >> - I've separated the stack walking code (`for_scope_method`) from the code >> that checks for a reference to the arena being closed >> (`is_accessing_session`), and added logging code to the former. That also >> required changing vframe code to accept an `ouputStream*` rather than always >> printing to `tty`. >> - Added a new test (`TestConcurrentClose`), that tries to close many shared >> arenas at the same time, in order to stress that use case. >> - Added a new benchmark (`ConcurrentClose`), that stresses the cases where >> many threads are accessing and closing shared arenas. >> >> I've done several benchmark runs with different amounts of threads. The >> confined case stays much more consistent, while the shared cases balloons up >> in time spent quickly when there are more than 4 threads: >> >> >> Benchmark Threads Mode Cnt Score Error Units >> ConcurrentClose.sharedAccess 32 avgt 10 9017.397 ± 202.870 us/op >> ConcurrentClose.sharedAccess 24 avgt 10 5178.214 ± 164.922 us/op >> ConcurrentClose.sharedAccess 16 avgt 10 2224.420 ± 165.754 us/op >> ConcurrentClose.sharedAccess8 avgt 10 593.828 ± 8.321 us/op >> ConcurrentClose.sharedAccess7 avgt 10 470.700 ± 22.511 us/op >> ConcurrentClose.sharedAccess6 avgt 10 386.697 ± 59.170 us/op >> ConcurrentClose.sharedAccess5 avgt 10 291.157 ± 7.023 us/op >> ConcurrentClose.sharedAccess4 avgt 10 209.178 ± 5.802 us/op >> ConcurrentClose.sharedAccess1 avgt 10 ... > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > track has_scoped_access for compiled methods src/hotspot/share/prims/scopedMemoryAccess.cpp line 179: > 177: // > 178: // The safepoint at which we're stopped may be in between the > liveness check > 179: // and actual memory access, but is itself 'outside' of @Scoped > code what is `@Scoped code`? I don't see that annotation mentioned here: https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/lang/ScopedValue.html - PR Review Comment: https://git.openjdk.org/jdk/pull/20158#discussion_r1677474756
Re: RFR: 8335480: Only deoptimize threads if needed when closing shared arena [v2]
On Fri, 12 Jul 2024 20:59:26 GMT, Jorn Vernee wrote: >> This PR limits the number of cases in which we deoptimize frames when >> closing a shared Arena. The initial intent of this was to improve the >> performance of shared arena closure in cases where a lot of threads are >> accessing and closing shared arenas at the same time (see attached >> benchmark), but unfortunately even disabling deoptimization altogether does >> not have a great effect on that benchmark. >> >> Nevertheless, I think the extra logging/testing/benchmark code, and comments >> I've written, together with reducing the number of cases where we deoptimize >> (which makes it clearer exactly why we need to deoptimize in the first >> place), will be useful going forward. So, I've a create this PR out of them. >> >> In this PR: >> - Deoptimizing is now only done in cases where it's needed, instead of >> always. Which is in cases where we are not inside an `@Scoped` method, but >> are inside a compiled frame that has a scoped access somewhere inside of it. >> - I've separated the stack walking code (`for_scope_method`) from the code >> that checks for a reference to the arena being closed >> (`is_accessing_session`), and added logging code to the former. That also >> required changing vframe code to accept an `ouputStream*` rather than always >> printing to `tty`. >> - Added a new test (`TestConcurrentClose`), that tries to close many shared >> arenas at the same time, in order to stress that use case. >> - Added a new benchmark (`ConcurrentClose`), that stresses the cases where >> many threads are accessing and closing shared arenas. >> >> I've done several benchmark runs with different amounts of threads. The >> confined case stays much more consistent, while the shared cases balloons up >> in time spent quickly when there are more than 4 threads: >> >> >> Benchmark Threads Mode Cnt Score Error Units >> ConcurrentClose.sharedAccess 32 avgt 10 9017.397 ± 202.870 us/op >> ConcurrentClose.sharedAccess 24 avgt 10 5178.214 ± 164.922 us/op >> ConcurrentClose.sharedAccess 16 avgt 10 2224.420 ± 165.754 us/op >> ConcurrentClose.sharedAccess8 avgt 10 593.828 ± 8.321 us/op >> ConcurrentClose.sharedAccess7 avgt 10 470.700 ± 22.511 us/op >> ConcurrentClose.sharedAccess6 avgt 10 386.697 ± 59.170 us/op >> ConcurrentClose.sharedAccess5 avgt 10 291.157 ± 7.023 us/op >> ConcurrentClose.sharedAccess4 avgt 10 209.178 ± 5.802 us/op >> ConcurrentClose.sharedAccess1 avgt 10 ... > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > track has_scoped_access for compiled methods src/hotspot/share/jvmci/jvmciRuntime.cpp line 2186: > 2184: nm->set_has_wide_vectors(has_wide_vector); > 2185: nm->set_has_monitors(has_monitors); > 2186: nm->set_has_scoped_access(true); // conservative What does "conservative" imply here? That is, what performance penalty will be incurred for Graal compiled code until it completely supports this "scoped access" bit? - PR Review Comment: https://git.openjdk.org/jdk/pull/20158#discussion_r1677490130
Re: RFR: 8335480: Only deoptimize threads if needed when closing shared arena [v4]
On Tue, 16 Jul 2024 14:46:13 GMT, Jorn Vernee wrote:
>> This PR limits the number of cases in which we deoptimize frames when
>> closing a shared Arena. The initial intent of this was to improve the
>> performance of shared arena closure in cases where a lot of threads are
>> accessing and closing shared arenas at the same time (see attached
>> benchmark), but unfortunately even disabling deoptimization altogether does
>> not have a great effect on that benchmark.
>>
>> Nevertheless, I think the extra logging/testing/benchmark code, and comments
>> I've written, together with reducing the number of cases where we deoptimize
>> (which makes it clearer exactly why we need to deoptimize in the first
>> place), will be useful going forward. So, I've a create this PR out of them.
>>
>> In this PR:
>> - Deoptimizing is now only done in cases where it's needed, instead of
>> always. Which is in cases where we are not inside an `@Scoped` method, but
>> are inside a compiled frame that has a scoped access somewhere inside of it.
>> - I've separated the stack walking code (`for_scope_method`) from the code
>> that checks for a reference to the arena being closed
>> (`is_accessing_session`), and added logging code to the former. That also
>> required changing vframe code to accept an `ouputStream*` rather than always
>> printing to `tty`.
>> - Added a new test (`TestConcurrentClose`), that tries to close many shared
>> arenas at the same time, in order to stress that use case.
>> - Added a new benchmark (`ConcurrentClose`), that stresses the cases where
>> many threads are accessing and closing shared arenas.
>>
>> I've done several benchmark runs with different amounts of threads. The
>> confined case stays much more consistent, while the shared cases balloons up
>> in time spent quickly when there are more than 4 threads:
>>
>>
>> Benchmark Threads Mode Cnt Score Error Units
>> ConcurrentClose.sharedAccess 32 avgt 10 9017.397 ± 202.870 us/op
>> ConcurrentClose.sharedAccess 24 avgt 10 5178.214 ± 164.922 us/op
>> ConcurrentClose.sharedAccess 16 avgt 10 2224.420 ± 165.754 us/op
>> ConcurrentClose.sharedAccess8 avgt 10 593.828 ± 8.321 us/op
>> ConcurrentClose.sharedAccess7 avgt 10 470.700 ± 22.511 us/op
>> ConcurrentClose.sharedAccess6 avgt 10 386.697 ± 59.170 us/op
>> ConcurrentClose.sharedAccess5 avgt 10 291.157 ± 7.023 us/op
>> ConcurrentClose.sharedAccess4 avgt 10 209.178 ± 5.802 us/op
>> ConcurrentClose.sharedAccess1 avgt 10 ...
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> JVMCI support
src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethod.java
line 62:
> 60:
> 61: /**
> 62: * Returns true if this method has a {@code Scoped} annotation.
Can you please make this a qualified name:
`jdk.internal.misc.ScopedMemoryAccess.Scoped`.
That makes it easier for someone not familiar with the code base to find.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20158#discussion_r1679575238
RFR: 8337972: Problem list jdk/internal/util/ReferencedKeyTest.java
Problem list until [JDK-8336926](https://bugs.openjdk.org/browse/JDK-8336926) is fixed to reduce the noise in testing. - Commit messages: - Problem list jdk/internal/util/ReferencedKeyTest.java Changes: https://git.openjdk.org/jdk/pull/20488/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20488&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337972 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20488.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20488/head:pull/20488 PR: https://git.openjdk.org/jdk/pull/20488
Re: RFR: 8337972: Problem list jdk/internal/util/ReferencedKeyTest.java
On Wed, 7 Aug 2024 08:33:06 GMT, Doug Simon wrote: > Problem list until [JDK-8336926](https://bugs.openjdk.org/browse/JDK-8336926) > is fixed to reduce the noise in testing. Great, thanks. - PR Comment: https://git.openjdk.org/jdk/pull/20488#issuecomment-2275017545
Withdrawn: 8337972: Problem list jdk/internal/util/ReferencedKeyTest.java
On Wed, 7 Aug 2024 08:33:06 GMT, Doug Simon wrote: > Problem list until [JDK-8336926](https://bugs.openjdk.org/browse/JDK-8336926) > is fixed to reduce the noise in testing. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/20488
RFR: 8294394: running jlink in GraalVM must copy libgraal into the output image
This PR adds a new jlink plugin (`--copy-files=`) that copies specified files from the current image into the output image. This is useful in the context of GraalVM where libgraal (e.g. `lib/libjvmcicompiler.so`) is produced after the final jlink step in the GraalVM JDK build process. The plugin makes it possible for jlink in a GraalVM JDK to propagate libgraal. The advantages of this solution over packaging libgraal as a module are: * No need to complicate and slow down the GraalVM JDK build process with an extra jlink step. * No need to pay the footprint of libgraal twice in a GraalVM JDK (i.e., once in,`lib/libjvmcicompiler.so` and again in a jmod file). - Commit messages: - add CopyFilesPlugin Changes: https://git.openjdk.org/jdk/pull/10448/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=10448&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8294394 Stats: 205 lines in 4 files changed: 205 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/10448.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10448/head:pull/10448 PR: https://git.openjdk.org/jdk/pull/10448
Re: RFR: 8294394: running jlink in GraalVM must copy libgraal into the output image
On Tue, 27 Sep 2022 11:30:26 GMT, Doug Simon wrote: > This PR adds a new jlink plugin (`--copy-files=`) that copies > specified files from the current image into the output image. > This is useful in the context of GraalVM where libgraal (e.g. > `lib/libjvmcicompiler.so`) is produced after the final jlink step in the > GraalVM JDK build process. The plugin makes it possible for jlink in a > GraalVM JDK to propagate libgraal. > > The advantages of this solution over packaging libgraal as a module are: > * No need to complicate and slow down the GraalVM JDK build process with an > extra jlink step. > * No need to pay the footprint of libgraal twice in a GraalVM JDK (i.e., once > in,`lib/libjvmcicompiler.so` and again in a jmod file). I can appreciate that this may be going against the design of a modular JDK. However, in the context of the possible jlink optimizations you're hinting at, jlink can still know about all classes and resources since the CopyFiles plugin is limited to copying files that already exist within the source image. That said, we'll revisit the GraalVM build process and see if we can avoid the need for the CopyFiles plugin. - PR: https://git.openjdk.org/jdk/pull/10448
Re: RFR: 8237467: effect of jlink plugins for vendor information and command-line options should be sticky
On Tue, 27 Sep 2022 19:11:07 GMT, Mandy Chung wrote:
>> This PR adds a new jlink plugin (`--save-jlink-argfiles=`) to
>> support persisting jlink options.
>>
>>
>>> echo "--add-modules jdk.internal.vm.ci --add-options=-Dfoo=xyzzy
>>> --vendor-version="XyzzyVM 3.14.15"
>>> --vendor-bug-url=https://bugs.xyzzy.com/"; > my_image.argfile
>>> export ALL_MODULES=$(java --list-modules | sed 's:@.*::g' | tr '\n' ',')
>>> jlink --keep-packaged-modules=my_image/jmods --add-modules $ALL_MODULES
>>> --output=my_image --save-jlink-argfiles my_image.argfile
>>> my_image/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy
>>> my_image/bin/jlink --add-modules=java.base --output=my_image2
>>> my_image2/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy
>> foo = xyzzy
>> java.vendor.url.bug = https://bugs.xyzzy.com/
>> java.vendor.version = XyzzyVM 3.14.15
>> OpenJDK Runtime Environment XyzzyVM 3.14.15 (build
>> 20-internal-2022-09-22-0951036.dnsimon...)
>> OpenJDK 64-Bit Server VM XyzzyVM 3.14.15 (build
>> 20-internal-2022-09-22-0951036.dnsimon..., mixed mode)
>>> my_image2/bin/java -d jdk.internal.vm.ci | head -1
>> jdk.internal.vm.ci@20-internal
>
> test/jdk/tools/jlink/plugins/SaveJlinkArgfilesPluginTest.java line 81:
>
>> 79: // Check that the primary image creation ignored the saved args
>> 80: oa.shouldHaveExitValue(0);
>> 81: oa.shouldNotMatch("yzzy");
>
> Can this check for `java.vendor.version = XyzzyVM 3.14.15`, that will also
> help the reader to understand what the test tries to verify.
Not sure follow as this is a negative match. I don't want it to match
`java.vendor.url.bug = https://bugs.xyzzy.com/`, `java.vendor.version = XyzzyVM
3.14.15` or `foo = xyzzy`. I'll expand it out to 3 tests.
-
PR: https://git.openjdk.org/jdk/pull/10445
Re: RFR: 8237467: effect of jlink plugins for vendor information and command-line options should be sticky [v2]
> This PR adds a new jlink plugin (`--save-jlink-argfiles=`) to > support persisting jlink options. > > >> echo "--add-modules jdk.internal.vm.ci --add-options=-Dfoo=xyzzy >> --vendor-version="XyzzyVM 3.14.15" --vendor-bug-url=https://bugs.xyzzy.com/"; >> > my_image.argfile >> export ALL_MODULES=$(java --list-modules | sed 's:@.*::g' | tr '\n' ',') >> jlink --keep-packaged-modules=my_image/jmods --add-modules $ALL_MODULES >> --output=my_image --save-jlink-argfiles my_image.argfile >> my_image/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy >> my_image/bin/jlink --add-modules=java.base --output=my_image2 >> my_image2/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy > foo = xyzzy > java.vendor.url.bug = https://bugs.xyzzy.com/ > java.vendor.version = XyzzyVM 3.14.15 > OpenJDK Runtime Environment XyzzyVM 3.14.15 (build > 20-internal-2022-09-22-0951036.dnsimon...) > OpenJDK 64-Bit Server VM XyzzyVM 3.14.15 (build > 20-internal-2022-09-22-0951036.dnsimon..., mixed mode) >> my_image2/bin/java -d jdk.internal.vm.ci | head -1 > jdk.internal.vm.ci@20-internal Doug Simon has updated the pull request incrementally with one additional commit since the last revision: changes to address review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/10445/files - new: https://git.openjdk.org/jdk/pull/10445/files/583981a7..9502cd75 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10445&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10445&range=00-01 Stats: 47 lines in 5 files changed: 9 ins; 22 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/10445.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10445/head:pull/10445 PR: https://git.openjdk.org/jdk/pull/10445
Re: RFR: 8237467: jlink plugin to save the argument files as input to jlink in the output image [v3]
> This PR adds a new jlink plugin (`--save-jlink-argfiles=`) to > support persisting jlink options. > > >> echo "--add-modules jdk.internal.vm.ci --add-options=-Dfoo=xyzzy >> --vendor-version="XyzzyVM 3.14.15" --vendor-bug-url=https://bugs.xyzzy.com/"; >> > my_image.argfile >> export ALL_MODULES=$(java --list-modules | sed 's:@.*::g' | tr '\n' ',') >> jlink --keep-packaged-modules=my_image/jmods --add-modules $ALL_MODULES >> --output=my_image --save-jlink-argfiles my_image.argfile >> my_image/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy >> my_image/bin/jlink --add-modules=java.base --output=my_image2 >> my_image2/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy > foo = xyzzy > java.vendor.url.bug = https://bugs.xyzzy.com/ > java.vendor.version = XyzzyVM 3.14.15 > OpenJDK Runtime Environment XyzzyVM 3.14.15 (build > 20-internal-2022-09-22-0951036.dnsimon...) > OpenJDK 64-Bit Server VM XyzzyVM 3.14.15 (build > 20-internal-2022-09-22-0951036.dnsimon..., mixed mode) >> my_image2/bin/java -d jdk.internal.vm.ci | head -1 > jdk.internal.vm.ci@20-internal Doug Simon has updated the pull request incrementally with one additional commit since the last revision: SaveJlinkArgfilesPlugin should verify that jdk.jlink is in the output image - Changes: - all: https://git.openjdk.org/jdk/pull/10445/files - new: https://git.openjdk.org/jdk/pull/10445/files/9502cd75..91938caa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10445&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10445&range=01-02 Stats: 16 lines in 2 files changed: 9 ins; 1 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/10445.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10445/head:pull/10445 PR: https://git.openjdk.org/jdk/pull/10445
Re: RFR: 8237467: jlink plugin to save the argument files as input to jlink in the output image [v4]
> This PR adds a new jlink plugin (`--save-jlink-argfiles=`) to > support persisting jlink options. > > >> echo "--add-modules jdk.internal.vm.ci --add-options=-Dfoo=xyzzy >> --vendor-version="XyzzyVM 3.14.15" --vendor-bug-url=https://bugs.xyzzy.com/"; >> > my_image.argfile >> export ALL_MODULES=$(java --list-modules | sed 's:@.*::g' | tr '\n' ',') >> jlink --keep-packaged-modules=my_image/jmods --add-modules $ALL_MODULES >> --output=my_image --save-jlink-argfiles my_image.argfile >> my_image/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy >> my_image/bin/jlink --add-modules=java.base --output=my_image2 >> my_image2/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy > foo = xyzzy > java.vendor.url.bug = https://bugs.xyzzy.com/ > java.vendor.version = XyzzyVM 3.14.15 > OpenJDK Runtime Environment XyzzyVM 3.14.15 (build > 20-internal-2022-09-22-0951036.dnsimon...) > OpenJDK 64-Bit Server VM XyzzyVM 3.14.15 (build > 20-internal-2022-09-22-0951036.dnsimon..., mixed mode) >> my_image2/bin/java -d jdk.internal.vm.ci | head -1 > jdk.internal.vm.ci@20-internal Doug Simon has updated the pull request incrementally with one additional commit since the last revision: improved text for help message - Changes: - all: https://git.openjdk.org/jdk/pull/10445/files - new: https://git.openjdk.org/jdk/pull/10445/files/91938caa..0c17bc89 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10445&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10445&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/10445.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10445/head:pull/10445 PR: https://git.openjdk.org/jdk/pull/10445
Re: RFR: 8237467: jlink plugin to save the argument files as input to jlink in the output image [v3]
On Tue, 27 Sep 2022 22:38:48 GMT, Mandy Chung wrote: >> Doug Simon has updated the pull request incrementally with one additional >> commit since the last revision: >> >> SaveJlinkArgfilesPlugin should verify that jdk.jlink is in the output image > > Looks good. Thanks for the reviews @mlchung ! - PR: https://git.openjdk.org/jdk/pull/10445
Integrated: 8237467: jlink plugin to save the argument files as input to jlink in the output image
On Tue, 27 Sep 2022 11:12:57 GMT, Doug Simon wrote: > This PR adds a new jlink plugin (`--save-jlink-argfiles=`) to > support persisting jlink options. > > >> echo "--add-modules jdk.internal.vm.ci --add-options=-Dfoo=xyzzy >> --vendor-version="XyzzyVM 3.14.15" --vendor-bug-url=https://bugs.xyzzy.com/"; >> > my_image.argfile >> export ALL_MODULES=$(java --list-modules | sed 's:@.*::g' | tr '\n' ',') >> jlink --keep-packaged-modules=my_image/jmods --add-modules $ALL_MODULES >> --output=my_image --save-jlink-argfiles my_image.argfile >> my_image/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy >> my_image/bin/jlink --add-modules=java.base --output=my_image2 >> my_image2/bin/java -XshowSettings:properties --version 2>&1 | grep yzzy > foo = xyzzy > java.vendor.url.bug = https://bugs.xyzzy.com/ > java.vendor.version = XyzzyVM 3.14.15 > OpenJDK Runtime Environment XyzzyVM 3.14.15 (build > 20-internal-2022-09-22-0951036.dnsimon...) > OpenJDK 64-Bit Server VM XyzzyVM 3.14.15 (build > 20-internal-2022-09-22-0951036.dnsimon..., mixed mode) >> my_image2/bin/java -d jdk.internal.vm.ci | head -1 > jdk.internal.vm.ci@20-internal This pull request has now been integrated. Changeset: 4f44fd63 Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/4f44fd63080d40d53a7751ebae93415aeb9b4a47 Stats: 414 lines in 6 files changed: 413 ins; 0 del; 1 mod 8237467: jlink plugin to save the argument files as input to jlink in the output image Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/10445
Withdrawn: 8294394: running jlink in GraalVM must copy libgraal into the output image
On Tue, 27 Sep 2022 11:30:26 GMT, Doug Simon wrote: > This PR adds a new jlink plugin (`--copy-files=`) that copies > specified files from the current image into the output image. > This is useful in the context of GraalVM where libgraal (e.g. > `lib/libjvmcicompiler.so`) is produced after the final jlink step in the > GraalVM JDK build process. The plugin makes it possible for jlink in a > GraalVM JDK to propagate libgraal. > > The advantages of this solution over packaging libgraal as a module are: > * No need to complicate and slow down the GraalVM JDK build process with an > extra jlink step. > * No need to pay the footprint of libgraal twice in a GraalVM JDK (i.e., once > in,`lib/libjvmcicompiler.so` and again in a jmod file). This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/10448
RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime
Libgraal is compiled ahead of time and should not need any JVMCI Java code to be dynamically loaded at runtime. Prior to this PR, this is not the case due to: * Code to copy system properties from the HotSpot heap into the libgraal heap. This is in `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should be moved to `java.base/jdk.internal.vm.VMSupport`. * Code to translate exceptions from the HotSpot heap into the libgraal heap and vice versa. This code should be moved from `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to `java.base/jdk.internal.vm.VMSupport`. This PR makes the above changes. As a result, it's possible to build a JDK image that includes (and uses) libgraal but does not include `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces footprint and better isolates the JAVMCI and the Graal compiler as accessing these components from Java code is now impossible. - Commit messages: - move JVMCI exception translation support into VMSupport - move JVMCITraceMark blocks within scope of C2V_BLOCK (GR-40773) - move JVMCI system properties support into VMSupport - only resolve jdk.internal.vm.ci if it is present on the file system Changes: https://git.openjdk.org/jdk/pull/11513/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8298099 Stats: 1414 lines in 20 files changed: 607 ins; 706 del; 101 mod Patch: https://git.openjdk.org/jdk/pull/11513.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11513/head:pull/11513 PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v2]
> Libgraal is compiled ahead of time and should not need any JVMCI Java code to > be dynamically loaded at runtime. Prior to this PR, this is not the case due > to: > > * Code to copy system properties from the HotSpot heap into the libgraal > heap. This is in > `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and > `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should be > moved to `java.base/jdk.internal.vm.VMSupport`. > * Code to translate exceptions from the HotSpot heap into the libgraal heap > and vice versa. This code should be moved from > `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to > `java.base/jdk.internal.vm.VMSupport`. > > This PR makes the above changes. As a result, it's possible to build a JDK > image that includes (and uses) libgraal but does not include > `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces > footprint and better isolates the JAVMCI and the Graal compiler as accessing > these components from Java code is now impossible. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: generalized ClassLoader::has_jvmci_module to is_module_resolvable - Changes: - all: https://git.openjdk.org/jdk/pull/11513/files - new: https://git.openjdk.org/jdk/pull/11513/files/b91f31fe..3e89d402 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=00-01 Stats: 9 lines in 3 files changed: 3 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/11513.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11513/head:pull/11513 PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v2]
On Mon, 5 Dec 2022 13:32:38 GMT, Alan Bateman wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> generalized ClassLoader::has_jvmci_module to is_module_resolvable
>
> src/hotspot/share/classfile/classLoader.cpp line 1419:
>
>> 1417:
>> 1418: // Returns true if jdk.internal.vm.ci is present on the file system.
>> 1419: bool ClassLoader::has_jvmci_module() {
>
> Would it be more useful to pass the module name so that the function tests if
> the module is is in the run-time image so that ClassLoader doesn't need to
> know the name "jdk.internal.vm.ci"?
Yes, good idea:
[3e89d40253b70251f9a2facce4b1d8d69701c045](https://github.com/openjdk/jdk/pull/11513/commits/3e89d40253b70251f9a2facce4b1d8d69701c045)
I also fixed a bug due in the size computation of `path`. Ideally, I'd factor
out and re-use the same code in `ClassLoader::add_to_exploded_build_list`.
However, the latter uses a `ResourceMark` which is not available when calling
`is_module_resolvable` early in VM startup before `JavaThread` is initialized.
-
PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v2]
On Mon, 5 Dec 2022 13:49:28 GMT, Doug Simon wrote: >> Libgraal is compiled ahead of time and should not need any JVMCI Java code >> to be dynamically loaded at runtime. Prior to this PR, this is not the case >> due to: >> >> * Code to copy system properties from the HotSpot heap into the libgraal >> heap. This is in >> `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and >> `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should >> be moved to `java.base/jdk.internal.vm.VMSupport`. >> * Code to translate exceptions from the HotSpot heap into the libgraal heap >> and vice versa. This code should be moved from >> `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to >> `java.base/jdk.internal.vm.VMSupport`. >> >> This PR makes the above changes. As a result, it's possible to build a JDK >> image that includes (and uses) libgraal but does not include >> `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces >> footprint and better isolates the JAVMCI and the Graal compiler as accessing >> these components from Java code is now impossible. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > generalized ClassLoader::has_jvmci_module to is_module_resolvable src/hotspot/share/classfile/classLoader.hpp line 378: > 376: > 377: // Determines if the `module_name` module is resolvable. > 378: static bool is_module_resolvable(const char* module_name); Is "resolvable" the right concept here? Or should it be something like "findable" instead? - PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v3]
> Libgraal is compiled ahead of time and should not need any JVMCI Java code to > be dynamically loaded at runtime. Prior to this PR, this is not the case due > to: > > * Code to copy system properties from the HotSpot heap into the libgraal > heap. This is in > `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and > `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should be > moved to `java.base/jdk.internal.vm.VMSupport`. > * Code to translate exceptions from the HotSpot heap into the libgraal heap > and vice versa. This code should be moved from > `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to > `java.base/jdk.internal.vm.VMSupport`. > > This PR makes the above changes. As a result, it's possible to build a JDK > image that includes (and uses) libgraal but does not include > `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces > footprint and better isolates the JAVMCI and the Graal compiler as accessing > these components from Java code is now impossible. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: share code to create the exploded path for a module and avoid stack-based variable length array - Changes: - all: https://git.openjdk.org/jdk/pull/11513/files - new: https://git.openjdk.org/jdk/pull/11513/files/3e89d402..7b147f2f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=01-02 Stats: 28 lines in 1 file changed: 15 ins; 11 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11513.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11513/head:pull/11513 PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v2]
On Mon, 5 Dec 2022 15:58:19 GMT, Alan Bateman wrote: >> src/hotspot/share/classfile/classLoader.hpp line 378: >> >>> 376: >>> 377: // Determines if the `module_name` module is resolvable. >>> 378: static bool is_module_resolvable(const char* module_name); >> >> Is "resolvable" the right concept here? Or should it be something like >> "findable" instead? > > Assuming --limit-modules isn't used, it is testing if the module is > "observable". I assume this function should therefore be named `is_module_observable`? - PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v2]
On Mon, 5 Dec 2022 17:17:16 GMT, Doug Simon wrote: >> Assuming --limit-modules isn't used, it is testing if the module is >> "observable". > > I assume this function should therefore be named `is_module_observable`? How about this: // Determines if the named module is present in the // modules jimage file or in the exploded modules directory. static bool is_module_observable(const char* module_name); - PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v4]
> Libgraal is compiled ahead of time and should not need any JVMCI Java code to > be dynamically loaded at runtime. Prior to this PR, this is not the case due > to: > > * Code to copy system properties from the HotSpot heap into the libgraal > heap. This is in > `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and > `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should be > moved to `java.base/jdk.internal.vm.VMSupport`. > * Code to translate exceptions from the HotSpot heap into the libgraal heap > and vice versa. This code should be moved from > `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to > `java.base/jdk.internal.vm.VMSupport`. > > This PR makes the above changes. As a result, it's possible to build a JDK > image that includes (and uses) libgraal but does not include > `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces > footprint and better isolates the JAVMCI and the Graal compiler as accessing > these components from Java code is now impossible. Doug Simon has updated the pull request incrementally with one additional commit since the last revision: renamed is_module_resolvable to is_module_observable - Changes: - all: https://git.openjdk.org/jdk/pull/11513/files - new: https://git.openjdk.org/jdk/pull/11513/files/7b147f2f..29e100b3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=02-03 Stats: 9 lines in 4 files changed: 1 ins; 4 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/11513.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11513/head:pull/11513 PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v4]
On Tue, 6 Dec 2022 05:28:24 GMT, David Holmes wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> renamed is_module_resolvable to is_module_observable
>
> src/hotspot/share/jvmci/jvmci.cpp line 234:
>
>> 232: if (thread != nullptr && thread->is_Java_thread()) {
>> 233: ResourceMark rm;
>> 234: JavaThreadState state = ((JavaThread*) thread)->thread_state();
>
> Please use `JavaThread::cast(thread)`
I've made this change. Out of interest, I grep'ed through `src/hotspot` and
found a few other instances of `(JavaThread*)` style casts. While most of these
are probably older code, I'm wondering what the guidelines are in this area. I
assume `JavaThread::cast` should be preferred always given the assertion
checking it does?
-
PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v5]
> Libgraal is compiled ahead of time and should not need any JVMCI Java code to > be dynamically loaded at runtime. Prior to this PR, this is not the case due > to: > > * Code to copy system properties from the HotSpot heap into the libgraal > heap. This is in > `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and > `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should be > moved to `java.base/jdk.internal.vm.VMSupport`. > * Code to translate exceptions from the HotSpot heap into the libgraal heap > and vice versa. This code should be moved from > `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to > `java.base/jdk.internal.vm.VMSupport`. > > This PR makes the above changes. As a result, it's possible to build a JDK > image that includes (and uses) libgraal but does not include > `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces > footprint and better isolates the JAVMCI and the Graal compiler as accessing > these components from Java code is now impossible. Doug Simon has updated the pull request incrementally with two additional commits since the last revision: - incorporate review feedback [skip ci] - removed hard-coded module name [skip ci] - Changes: - all: https://git.openjdk.org/jdk/pull/11513/files - new: https://git.openjdk.org/jdk/pull/11513/files/29e100b3..342d5e3c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=03-04 Stats: 6 lines in 4 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/11513.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11513/head:pull/11513 PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v5]
On Tue, 6 Dec 2022 13:02:40 GMT, Alan Bateman wrote:
>> Doug Simon has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - incorporate review feedback [skip ci]
>> - removed hard-coded module name [skip ci]
>
> src/java.base/share/classes/jdk/internal/vm/TranslatedException.java line 44:
>
>> 42: */
>> 43: @SuppressWarnings("serial")
>> 44: final class TranslatedException extends Exception {
>
> Would it be possible to re-format this to avoid the wildly long (150+) lines?
> This seems to be moving jdk.vm.ci.hotspot.TranslatedException and hard to see
> what is going on.
Is there some tool support for formatting Java source code to meet OpenJDK
coding guidelines? Rather unfortunate that one has to do this manually and
reviewers have to spend time manually checking it.
> src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 66:
>
>> 64: * only contains String keys and values.
>> 65: */
>> 66: private static byte[] serializePropertiesToByteArray(Properties p,
>> boolean filter) throws IOException {
>
> I think we need more context as to why there are non-String system properties
> in use here.
This parameter exists to allow a caller to pass in a `Properties` object that
is guaranteed to have only String keys and so does not need the extra filtering
done in this method. I'll refactor the code to make it clearer.
> src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 106:
>
>> 104: }
>> 105: if (props.get("oome") != null) {
>> 106: throw new OutOfMemoryError("forced OOME");
>
> I don't think code in java.base should be checking for a property named
> "oome". Is this for a test that sets this system property on the command
> line?
Sorry, that's debug code that I will remove.
-
PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v6]
> Libgraal is compiled ahead of time and should not need any JVMCI Java code to > be dynamically loaded at runtime. Prior to this PR, this is not the case due > to: > > * Code to copy system properties from the HotSpot heap into the libgraal > heap. This is in > `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and > `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should be > moved to `java.base/jdk.internal.vm.VMSupport`. > * Code to translate exceptions from the HotSpot heap into the libgraal heap > and vice versa. This code should be moved from > `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to > `java.base/jdk.internal.vm.VMSupport`. > > This PR makes the above changes. As a result, it's possible to build a JDK > image that includes (and uses) libgraal but does not include > `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces > footprint and better isolates the JAVMCI and the Graal compiler as accessing > these components from Java code is now impossible. Doug Simon has updated the pull request incrementally with four additional commits since the last revision: - formatting to avoid very long lines [skip ci] - removed debug code [skip ci] - clarify Properties filtering [skip ci] - remove debug code [skip ci] - Changes: - all: https://git.openjdk.org/jdk/pull/11513/files - new: https://git.openjdk.org/jdk/pull/11513/files/342d5e3c..bdbe7cf2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=04-05 Stats: 66 lines in 2 files changed: 23 ins; 14 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/11513.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11513/head:pull/11513 PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v5]
On Tue, 6 Dec 2022 15:41:20 GMT, Doug Simon wrote:
>> src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 66:
>>
>>> 64: * only contains String keys and values.
>>> 65: */
>>> 66: private static byte[] serializePropertiesToByteArray(Properties p,
>>> boolean filter) throws IOException {
>>
>> I think we need more context as to why there are non-String system
>> properties in use here.
>
> This parameter exists to allow a caller to pass in a `Properties` object that
> is guaranteed to have only String keys and so does not need the extra
> filtering done in this method. I'll refactor the code to make it clearer.
Hopefully this makes it clearer:
https://github.com/openjdk/jdk/pull/11513/commits/5c610798fe4eaed7efeb2eebcf1e5db47714caee
-
PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v6]
On Wed, 7 Dec 2022 18:42:43 GMT, Alan Bateman wrote:
>> Doug Simon has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - formatting to avoid very long lines [skip ci]
>> - removed debug code [skip ci]
>> - clarify Properties filtering [skip ci]
>> - remove debug code [skip ci]
>
> src/java.base/share/classes/jdk/internal/vm/TranslatedException.java line 128:
>
>> 126: // Try create with reflection first.
>> 127: try {
>> 128: Class cls = Class.forName(className);
>
> A question about the Class.forName usage here. Class.forName uses the
> defining class loader of the current class so I'm wondering if the exceptions
> to be decoded are always of a type defined to the boot loader?
> jdk.internal.vm.ci is defined to the boot loader so this code hasn't really
> changed, it's just "new" to java.base in this PR.
The exceptions can be of any type and defined by any loader. In the case that
Class.forName fails, the name of the original exception type is shown as part
of `TranslationException.toString()`. Combined with the stack trace, this has
always been sufficient to understand the origin of an exception thrown on a
HotSpot/libjvmci mixed stack.
-
PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v7]
> Libgraal is compiled ahead of time and should not need any JVMCI Java code to > be dynamically loaded at runtime. Prior to this PR, this is not the case due > to: > > * Code to copy system properties from the HotSpot heap into the libgraal > heap. This is in > `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and > `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should be > moved to `java.base/jdk.internal.vm.VMSupport`. > * Code to translate exceptions from the HotSpot heap into the libgraal heap > and vice versa. This code should be moved from > `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to > `java.base/jdk.internal.vm.VMSupport`. > > This PR makes the above changes. As a result, it's possible to build a JDK > image that includes (and uses) libgraal but does not include > `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces > footprint and better isolates the JAVMCI and the Graal compiler as accessing > these components from Java code is now impossible. Doug Simon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 14 additional commits since the last revision: - Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8298099 - formatting to avoid very long lines [skip ci] - removed debug code [skip ci] - clarify Properties filtering [skip ci] - remove debug code [skip ci] - incorporate review feedback [skip ci] - removed hard-coded module name [skip ci] - renamed is_module_resolvable to is_module_observable - share code to create the exploded path for a module and avoid stack-based variable length array - generalized ClassLoader::has_jvmci_module to is_module_resolvable - ... and 4 more: https://git.openjdk.org/jdk/compare/a1a75a56...f5219b20 - Changes: - all: https://git.openjdk.org/jdk/pull/11513/files - new: https://git.openjdk.org/jdk/pull/11513/files/bdbe7cf2..f5219b20 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11513&range=05-06 Stats: 29076 lines in 626 files changed: 16520 ins; 7341 del; 5215 mod Patch: https://git.openjdk.org/jdk/pull/11513.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11513/head:pull/11513 PR: https://git.openjdk.org/jdk/pull/11513
Integrated: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime
On Mon, 5 Dec 2022 13:16:25 GMT, Doug Simon wrote: > Libgraal is compiled ahead of time and should not need any JVMCI Java code to > be dynamically loaded at runtime. Prior to this PR, this is not the case due > to: > > * Code to copy system properties from the HotSpot heap into the libgraal > heap. This is in > `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and > `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should be > moved to `java.base/jdk.internal.vm.VMSupport`. > * Code to translate exceptions from the HotSpot heap into the libgraal heap > and vice versa. This code should be moved from > `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to > `java.base/jdk.internal.vm.VMSupport`. > > This PR makes the above changes. As a result, it's possible to build a JDK > image that includes (and uses) libgraal but does not include > `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces > footprint and better isolates the JAVMCI and the Graal compiler as accessing > these components from Java code is now impossible. This pull request has now been integrated. Changeset: 8b69a2e4 Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/8b69a2e434ad2fa3369079622b57afb973d5bd9a Stats: 1438 lines in 20 files changed: 628 ins; 714 del; 96 mod 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime Reviewed-by: never - PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8298099: [JVMCI] decouple libgraal from JVMCI module at runtime [v7]
On Wed, 7 Dec 2022 19:49:47 GMT, Doug Simon wrote: >> Libgraal is compiled ahead of time and should not need any JVMCI Java code >> to be dynamically loaded at runtime. Prior to this PR, this is not the case >> due to: >> >> * Code to copy system properties from the HotSpot heap into the libgraal >> heap. This is in >> `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and >> `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should >> be moved to `java.base/jdk.internal.vm.VMSupport`. >> * Code to translate exceptions from the HotSpot heap into the libgraal heap >> and vice versa. This code should be moved from >> `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to >> `java.base/jdk.internal.vm.VMSupport`. >> >> This PR makes the above changes. As a result, it's possible to build a JDK >> image that includes (and uses) libgraal but does not include >> `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces >> footprint and better isolates the JAVMCI and the Graal compiler as accessing >> these components from Java code is now impossible. > > Doug Simon has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 14 additional commits since > the last revision: > > - Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8298099 > - formatting to avoid very long lines [skip ci] > - removed debug code [skip ci] > - clarify Properties filtering [skip ci] > - remove debug code [skip ci] > - incorporate review feedback [skip ci] > - removed hard-coded module name [skip ci] > - renamed is_module_resolvable to is_module_observable > - share code to create the exploded path for a module and avoid stack-based > variable length array > - generalized ClassLoader::has_jvmci_module to is_module_resolvable > - ... and 4 more: https://git.openjdk.org/jdk/compare/fbca1fb5...f5219b20 Sorry I checked with Alan over Slack who said to go ahead with merging. - PR: https://git.openjdk.org/jdk/pull/11513
Re: RFR: 8300487: Store cardinality as a field in BitSet
On Tue, 3 Jan 2023 23:25:39 GMT, fabioromano1 wrote: > The enanchment is useful for applications that make heavy use of BitSet > objects as sets of integers, and therefore they need to make a lot of calls > to cardinality() method, which actually require linear time in the number of > words in use by the bit set. > This optimization reduces the cost of calling cardinality() to constant time, > as it simply returns the value of the field, and it also try to make as > little effort as possible to update the field, when needed. > > Moreover, it has been implemented a new method for testing wheter a bit set > includes another bit set (i.e., the set of true bits of the parameter is a > subset of the true bits of the instance). (disclaimer - I am not a reviewer) I think it's not recommended to do wholesale reformatting in a PR; it tends to obscure history and blurs the line between your essential changes and the non-essential changes. - PR: https://git.openjdk.org/jdk/pull/11837
Re: RFR: 8300487: Store cardinality as a field in BitSet
On Wed, 11 Jan 2023 16:02:40 GMT, Sergey Kuksenko wrote: > It would be better to see benchmark results other than cardinality operation. > To understand how big performance regression is. Agreed - I suspect this is adding unnecessary overhead to the most common uses of BitSet; I'd be tempted to roll my own if this went in. Benchmarks could add some level of comfort. - PR: https://git.openjdk.org/jdk/pull/11837
Re: RFR: JDK-8286666: JEP 429: Implementation of Scoped Values (Incubator) [v38]
On Tue, 6 Dec 2022 21:14:07 GMT, Andrew Haley wrote: >> JEP 429 implementation. > > Andrew Haley has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 71 commits: > > - Merge from JDK mainline > - Add comment > - Merge https://github.com/openjdk/jdk into JDK-828 > - Feedback from reviewers > - Feedback from reviewers > - Unused variable > - Cleanup > - Cleanup > - Merge branch 'JDK-828' of https://github.com/theRealAph/jdk into > JDK-828 > - Update > src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/ScopedValue.java > >Co-authored-by: Paul Sandoz > - ... and 61 more: https://git.openjdk.org/jdk/compare/2cdc0195...46fa1389 src/hotspot/share/classfile/vmIntrinsics.hpp line 295: > 293:do_name( setScopedValueCache_name, > "setScopedValueCache") \ > 294:do_signature(setScopedValueCache_signature, > "([Ljava/lang/Object;)V") \ > 295: do_intrinsic(_findScopedValueBindings, java_lang_Thread, > findScopedValueBindings_name, void_object_signature, F_SN) \ As far as I can tell, there is not yet an intrinsic implementation for `findScopedValueBindings`. Is this a placeholder for a pending implementation? - PR: https://git.openjdk.org/jdk/pull/10952
Re: RFR: 8302976: C2 intrinsification of Float.floatToFloat16 and Float.float16ToFloat yields different result than the interpreter
On Wed, 22 Feb 2023 05:21:48 GMT, Joe Darcy wrote: > That said, I think it is reasonable on a given JVM invocation if > Float.floatToFloat16(f) gave the same result for input f regardless of in > what context it was called. Yes, I'm under the impression that for math API methods like this, the stability of input to output must be preserved for a single JVM invocation. Or are there existing methods for which the interpreter and compiled code execution is allowed to differ? - PR: https://git.openjdk.org/jdk/pull/12704
RFR: 8303431: [JVMCI] libgraal annotation API
This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
annotations. The main differences from `java.lang.reflect.AnnotatedElement` are:
* Each `Annotated` method explicitly specifies the annotation type(s) for which
it wants annotation data. That is, there is no direct equivalent of
`AnnotatedElement.getAnnotations()`.
* Annotation data is returned in a map-like object (of type
`jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
works better for libgraal as it avoids the need for annotation types to be
loaded and included in libgraal.
To demonstrate the new API, here's an example in terms
`java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
ResolvedJavaMethod method = ...;
ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
return switch (a.kind()) {
case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
case FULL_UNROLL_UNTIL_RETURN ->
LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
...
}
The same code using the new API:
ResolvedJavaMethod method = ...;
ResolvedJavaType explodeLoopType = ...;
AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
return switch (a.getEnum("kind").getName()) {
case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
case "FULL_UNROLL_UNTIL_RETURN" ->
LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
...
}
The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
parsing annotations and serializing/deserializing to/from a byte array. This
allows the annotation data to be passed from the HotSpot heap to the libgraal
heap.
-
Commit messages:
- made AnnotationDataDecoder package-private
- add annotation API to JVMCI
Changes: https://git.openjdk.org/jdk/pull/12810/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8303431
Stats: 2666 lines in 33 files changed: 2614 ins; 24 del; 28 mod
Patch: https://git.openjdk.org/jdk/pull/12810.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12810/head:pull/12810
PR: https://git.openjdk.org/jdk/pull/12810
RFR: 8303577: [JVMCI] OOME causes crash while translating exceptions
JDK-8297431 added code for special handling of OutOfMemoryError when translating an exception between libjvmci and HotSpot[1]. Unfortunately, this code was deleted by JDK-8298099 when moving the exception translation mechanism to VMSupport[2]. This causes the VM to crash when an OOME occurs while translating an exception from HotSpot to libjvmci. This PR revives the deleted code. This bug was found by running `test/jdk/java/util/concurrent/locks/Lock/OOMEInAQS.java` on libgraal. The fix now makes the test pass. [1] https://github.com/openjdk/jdk/commit/952e10055135613e8ea2b818a4f35842936f5633#diff-4d3a3b7e7e12e1d5b4cf3e4677d9e0de5e9df3bbf1bbfa0d8d43d12098d67dc4R222-R234 [2] https://github.com/openjdk/jdk/commit/8b69a2e434ad2fa3369079622b57afb973d5bd9a#diff-7292551772c27b7152af03cbbad90a897c5e37c6a97d4026be835e6d8fe1R121-R125 - Commit messages: - decodeAndThrowThrowable needs to handle error codes Changes: https://git.openjdk.org/jdk/pull/12857/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12857&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303577 Stats: 31 lines in 5 files changed: 22 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/12857.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12857/head:pull/12857 PR: https://git.openjdk.org/jdk/pull/12857
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v2]
> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
> annotations. The main differences from `java.lang.reflect.AnnotatedElement`
> are:
> * Each `Annotated` method explicitly specifies the annotation type(s) for
> which it wants annotation data. That is, there is no direct equivalent of
> `AnnotatedElement.getAnnotations()`.
> * Annotation data is returned in a map-like object (of type
> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
> works better for libgraal as it avoids the need for annotation types to be
> loaded and included in libgraal.
>
> To demonstrate the new API, here's an example in terms
> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>
> ResolvedJavaMethod method = ...;
> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
> return switch (a.kind()) {
> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
> case FULL_UNROLL_UNTIL_RETURN ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The same code using the new API:
>
>
> ResolvedJavaMethod method = ...;
> ResolvedJavaType explodeLoopType = ...;
> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
> return switch (a.getEnum("kind").getName()) {
> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
> case "FULL_UNROLL_UNTIL_RETURN" ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
> parsing annotations and serializing/deserializing to/from a byte array. This
> allows the annotation data to be passed from the HotSpot heap to the libgraal
> heap.
Doug Simon has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains four commits:
- added support for inherited annotations
- Merge branch 'master' into JDK-8303431
- made AnnotationDataDecoder package-private
- add annotation API to JVMCI
-
Changes: https://git.openjdk.org/jdk/pull/12810/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=01
Stats: 2752 lines in 33 files changed: 2700 ins; 24 del; 28 mod
Patch: https://git.openjdk.org/jdk/pull/12810.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12810/head:pull/12810
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v3]
> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
> annotations. The main differences from `java.lang.reflect.AnnotatedElement`
> are:
> * Each `Annotated` method explicitly specifies the annotation type(s) for
> which it wants annotation data. That is, there is no direct equivalent of
> `AnnotatedElement.getAnnotations()`.
> * Annotation data is returned in a map-like object (of type
> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
> works better for libgraal as it avoids the need for annotation types to be
> loaded and included in libgraal.
>
> To demonstrate the new API, here's an example in terms
> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>
> ResolvedJavaMethod method = ...;
> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
> return switch (a.kind()) {
> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
> case FULL_UNROLL_UNTIL_RETURN ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The same code using the new API:
>
>
> ResolvedJavaMethod method = ...;
> ResolvedJavaType explodeLoopType = ...;
> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
> return switch (a.getEnum("kind").getName()) {
> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
> case "FULL_UNROLL_UNTIL_RETURN" ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
> parsing annotations and serializing/deserializing to/from a byte array. This
> allows the annotation data to be passed from the HotSpot heap to the libgraal
> heap.
Doug Simon has updated the pull request incrementally with one additional
commit since the last revision:
fixed whitespace
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12810/files
- new: https://git.openjdk.org/jdk/pull/12810/files/8743e8b9..3dd5ef9c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=01-02
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/12810.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12810/head:pull/12810
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303577: [JVMCI] OOME causes crash while translating exceptions
On Fri, 3 Mar 2023 18:05:51 GMT, Vladimir Kozlov wrote: >> JDK-8297431 added code for special handling of OutOfMemoryError when >> translating an exception between libjvmci and HotSpot[1]. >> Unfortunately, this code was deleted by JDK-8298099 when moving the >> exception translation mechanism to VMSupport[2]. >> This causes the VM to crash when an OOME occurs while translating an >> exception from HotSpot to libjvmci. >> This PR revives the deleted code. >> >> This bug was found by running >> `test/jdk/java/util/concurrent/locks/Lock/OOMEInAQS.java` on libgraal. The >> fix now makes the test pass. >> >> Note that the code modified in >> `src/java.base/share/classes/jdk/internal/vm/VMSupport.java` is JVMCI >> specific and was original added by >> [JDK-8298099](https://git.openjdk.org/jdk/pull/11513). >> >> [1] >> https://github.com/openjdk/jdk/commit/952e10055135613e8ea2b818a4f35842936f5633#diff-4d3a3b7e7e12e1d5b4cf3e4677d9e0de5e9df3bbf1bbfa0d8d43d12098d67dc4R222-R234 >> [2] >> https://github.com/openjdk/jdk/commit/8b69a2e434ad2fa3369079622b57afb973d5bd9a#diff-7292551772c27b7152af03cbbad90a897c5e37c6a97d4026be835e6d8fe1R121-R125 > > Good. Thanks for the reviews @vnkozlov and @tkrodriguez . - PR: https://git.openjdk.org/jdk/pull/12857
Integrated: 8303577: [JVMCI] OOME causes crash while translating exceptions
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Fri, 3 Mar 2023 15:40:01 GMT, Doug Simon wrote: > JDK-8297431 added code for special handling of OutOfMemoryError when > translating an exception between libjvmci and HotSpot[1]. > Unfortunately, this code was deleted by JDK-8298099 when moving the exception > translation mechanism to VMSupport[2]. > This causes the VM to crash when an OOME occurs while translating an > exception from HotSpot to libjvmci. > This PR revives the deleted code. > > This bug was found by running > `test/jdk/java/util/concurrent/locks/Lock/OOMEInAQS.java` on libgraal. The > fix now makes the test pass. > > Note that the code modified in > `src/java.base/share/classes/jdk/internal/vm/VMSupport.java` is JVMCI > specific and was original added by > [JDK-8298099](https://git.openjdk.org/jdk/pull/11513). > > [1] > https://github.com/openjdk/jdk/commit/952e10055135613e8ea2b818a4f35842936f5633#diff-4d3a3b7e7e12e1d5b4cf3e4677d9e0de5e9df3bbf1bbfa0d8d43d12098d67dc4R222-R234 > [2] > https://github.com/openjdk/jdk/commit/8b69a2e434ad2fa3369079622b57afb973d5bd9a#diff-7292551772c27b7152af03cbbad90a897c5e37c6a97d4026be835e6d8fe1R121-R125 This pull request has now been integrated. Changeset: cac81ddc Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/cac81ddc9259168a5b12c290ae2ce7db25a729fc Stats: 31 lines in 5 files changed: 22 ins; 0 del; 9 mod 8303577: [JVMCI] OOME causes crash while translating exceptions Reviewed-by: kvn, never - PR: https://git.openjdk.org/jdk/pull/12857
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v3]
On Sun, 5 Mar 2023 22:37:38 GMT, Doug Simon wrote:
>> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for
>> accessing annotations. The main differences from
>> `java.lang.reflect.AnnotatedElement` are:
>> * Each `Annotated` method explicitly specifies the annotation type(s) for
>> which it wants annotation data. That is, there is no direct equivalent of
>> `AnnotatedElement.getAnnotations()`.
>> * Annotation data is returned in a map-like object (of type
>> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
>> works better for libgraal as it avoids the need for annotation types to be
>> loaded and included in libgraal.
>>
>> To demonstrate the new API, here's an example in terms
>> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>>
>> ResolvedJavaMethod method = ...;
>> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
>> return switch (a.kind()) {
>> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
>> case FULL_UNROLL_UNTIL_RETURN ->
>> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
>> ...
>> }
>>
>>
>> The same code using the new API:
>>
>>
>> ResolvedJavaMethod method = ...;
>> ResolvedJavaType explodeLoopType = ...;
>> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
>> return switch (a.getEnum("kind").getName()) {
>> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
>> case "FULL_UNROLL_UNTIL_RETURN" ->
>> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
>> ...
>> }
>>
>>
>> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
>> parsing annotations and serializing/deserializing to/from a byte array. This
>> allows the annotation data to be passed from the HotSpot heap to the
>> libgraal heap.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixed whitespace
This PR still needs work. I'll re-open it when ready.
-
PR: https://git.openjdk.org/jdk/pull/12810
Withdrawn: 8303431: [JVMCI] libgraal annotation API
On Wed, 1 Mar 2023 18:07:34 GMT, Doug Simon wrote:
> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
> annotations. The main differences from `java.lang.reflect.AnnotatedElement`
> are:
> * Each `Annotated` method explicitly specifies the annotation type(s) for
> which it wants annotation data. That is, there is no direct equivalent of
> `AnnotatedElement.getAnnotations()`.
> * Annotation data is returned in a map-like object (of type
> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
> works better for libgraal as it avoids the need for annotation types to be
> loaded and included in libgraal.
>
> To demonstrate the new API, here's an example in terms
> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>
> ResolvedJavaMethod method = ...;
> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
> return switch (a.kind()) {
> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
> case FULL_UNROLL_UNTIL_RETURN ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The same code using the new API:
>
>
> ResolvedJavaMethod method = ...;
> ResolvedJavaType explodeLoopType = ...;
> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
> return switch (a.getEnum("kind").getName()) {
> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
> case "FULL_UNROLL_UNTIL_RETURN" ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
> parsing annotations and serializing/deserializing to/from a byte array. This
> allows the annotation data to be passed from the HotSpot heap to the libgraal
> heap.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v4]
> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
> annotations. The main differences from `java.lang.reflect.AnnotatedElement`
> are:
> * Each `Annotated` method explicitly specifies the annotation type(s) for
> which it wants annotation data. That is, there is no direct equivalent of
> `AnnotatedElement.getAnnotations()`.
> * Annotation data is returned in a map-like object (of type
> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
> works better for libgraal as it avoids the need for annotation types to be
> loaded and included in libgraal.
>
> To demonstrate the new API, here's an example in terms
> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>
> ResolvedJavaMethod method = ...;
> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
> return switch (a.kind()) {
> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
> case FULL_UNROLL_UNTIL_RETURN ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The same code using the new API:
>
>
> ResolvedJavaMethod method = ...;
> ResolvedJavaType explodeLoopType = ...;
> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
> return switch (a.getEnum("kind").getName()) {
> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
> case "FULL_UNROLL_UNTIL_RETURN" ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
> parsing annotations and serializing/deserializing to/from a byte array. This
> allows the annotation data to be passed from the HotSpot heap to the libgraal
> heap.
Doug Simon has updated the pull request incrementally with one additional
commit since the last revision:
switched to use of lists and maps instead of arrays
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12810/files
- new: https://git.openjdk.org/jdk/pull/12810/files/3dd5ef9c..948d3aa3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=02-03
Stats: 1241 lines in 15 files changed: 292 ins; 718 del; 231 mod
Patch: https://git.openjdk.org/jdk/pull/12810.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12810/head:pull/12810
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v5]
> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
> annotations. The main differences from `java.lang.reflect.AnnotatedElement`
> are:
> * Each `Annotated` method explicitly specifies the annotation type(s) for
> which it wants annotation data. That is, there is no direct equivalent of
> `AnnotatedElement.getAnnotations()`.
> * Annotation data is returned in a map-like object (of type
> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
> works better for libgraal as it avoids the need for annotation types to be
> loaded and included in libgraal.
>
> To demonstrate the new API, here's an example in terms
> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>
> ResolvedJavaMethod method = ...;
> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
> return switch (a.kind()) {
> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
> case FULL_UNROLL_UNTIL_RETURN ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The same code using the new API:
>
>
> ResolvedJavaMethod method = ...;
> ResolvedJavaType explodeLoopType = ...;
> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
> return switch (a.getEnum("kind").getName()) {
> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
> case "FULL_UNROLL_UNTIL_RETURN" ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
> parsing annotations and serializing/deserializing to/from a byte array. This
> allows the annotation data to be passed from the HotSpot heap to the libgraal
> heap.
Doug Simon has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains seven commits:
- Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8303431
- switched to use of lists and maps instead of arrays
- fixed whitespace
- added support for inherited annotations
- Merge branch 'master' into JDK-8303431
- made AnnotationDataDecoder package-private
- add annotation API to JVMCI
-
Changes: https://git.openjdk.org/jdk/pull/12810/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=04
Stats: 2326 lines in 34 files changed: 2273 ins; 23 del; 30 mod
Patch: https://git.openjdk.org/jdk/pull/12810.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12810/head:pull/12810
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v5]
On Mon, 13 Mar 2023 19:23:39 GMT, Vladimir Kozlov wrote: >> Doug Simon has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains seven commits: >> >> - Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8303431 >> - switched to use of lists and maps instead of arrays >> - fixed whitespace >> - added support for inherited annotations >> - Merge branch 'master' into JDK-8303431 >> - made AnnotationDataDecoder package-private >> - add annotation API to JVMCI > > test/jdk/jdk/internal/vm/TestTranslatedException.java line 61: > >> 59: encodeDecode(throwable); >> 60: } >> 61: > > Why this was removed? Because it does exactly the same thing as `encodeDecodeTest`. It should have been cleaned up in the original PR that introduced this test. - PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v5]
On Tue, 14 Mar 2023 06:28:20 GMT, Tom Rodriguez wrote:
>> Doug Simon has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains seven commits:
>>
>> - Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8303431
>> - switched to use of lists and maps instead of arrays
>> - fixed whitespace
>> - added support for inherited annotations
>> - Merge branch 'master' into JDK-8303431
>> - made AnnotationDataDecoder package-private
>> - add annotation API to JVMCI
>
> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 2699:
>
>> 2697: typeArrayOop ba = typeArrayOop(res);
>> 2698: int ba_len = ba->length();
>> 2699: if (ba_len <= 256) {
>
> Is this really necessary? Resource allocation is very cheap.
Ok, good point. I'll remove the optimization.
-
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v6]
> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
> annotations. The main differences from `java.lang.reflect.AnnotatedElement`
> are:
> * Each `Annotated` method explicitly specifies the annotation type(s) for
> which it wants annotation data. That is, there is no direct equivalent of
> `AnnotatedElement.getAnnotations()`.
> * Annotation data is returned in a map-like object (of type
> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
> works better for libgraal as it avoids the need for annotation types to be
> loaded and included in libgraal.
>
> To demonstrate the new API, here's an example in terms
> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>
> ResolvedJavaMethod method = ...;
> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
> return switch (a.kind()) {
> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
> case FULL_UNROLL_UNTIL_RETURN ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The same code using the new API:
>
>
> ResolvedJavaMethod method = ...;
> ResolvedJavaType explodeLoopType = ...;
> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
> return switch (a.getEnum("kind").getName()) {
> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
> case "FULL_UNROLL_UNTIL_RETURN" ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
> parsing annotations and serializing/deserializing to/from a byte array. This
> allows the annotation data to be passed from the HotSpot heap to the libgraal
> heap.
Doug Simon has updated the pull request incrementally with one additional
commit since the last revision:
addressed review feedback
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12810/files
- new: https://git.openjdk.org/jdk/pull/12810/files/a85fa13a..abaf2375
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=05
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=04-05
Stats: 10 lines in 2 files changed: 0 ins; 9 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/12810.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12810/head:pull/12810
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v6]
On Tue, 14 Mar 2023 16:06:06 GMT, Doug Simon wrote:
>> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for
>> accessing annotations. The main differences from
>> `java.lang.reflect.AnnotatedElement` are:
>> * All methods in the `Annotated` interface explicitly specify requested
>> annotation type(s). That is, there is no equivalent of
>> `AnnotatedElement.getAnnotations()`.
>> * Annotation data is returned in a map-like object (of type
>> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
>> works better for libgraal as it avoids the need for annotation types to be
>> loaded and included in libgraal.
>>
>> To demonstrate the new API, here's an example in terms
>> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>>
>> ResolvedJavaMethod method = ...;
>> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
>> return switch (a.kind()) {
>> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
>> case FULL_UNROLL_UNTIL_RETURN ->
>> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
>> ...
>> }
>>
>>
>> The same code using the new API:
>>
>>
>> ResolvedJavaMethod method = ...;
>> ResolvedJavaType explodeLoopType = ...;
>> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
>> return switch (a.getEnum("kind").getName()) {
>> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
>> case "FULL_UNROLL_UNTIL_RETURN" ->
>> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
>> ...
>> }
>>
>>
>> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
>> parsing annotations and serializing/deserializing to/from a byte array. This
>> allows the annotation data to be passed from the HotSpot heap to the
>> libgraal heap.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> addressed review feedback
@jddarcy would you be able to help review this PR? Based on git log, you are
knowledgeable in `sun.reflect.annotation`. If not, please suggest who else I
can reach out to for a review.
-
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v6]
On Wed, 15 Mar 2023 19:23:52 GMT, Joe Darcy wrote: > I assume https://bugs.openjdk.org/browse/JDK-8303431 recounts the motivation > behind this change? Yes, it does. Thanks in advance. - PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v7]
> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
> annotations. The main differences from `java.lang.reflect.AnnotatedElement`
> are:
> * All methods in the `Annotated` interface explicitly specify requested
> annotation type(s). That is, there is no equivalent of
> `AnnotatedElement.getAnnotations()`.
> * Annotation data is returned in a map-like object (of type
> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
> works better for libgraal as it avoids the need for annotation types to be
> loaded and included in libgraal.
>
> To demonstrate the new API, here's an example in terms
> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>
> ResolvedJavaMethod method = ...;
> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
> return switch (a.kind()) {
> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
> case FULL_UNROLL_UNTIL_RETURN ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The same code using the new API:
>
>
> ResolvedJavaMethod method = ...;
> ResolvedJavaType explodeLoopType = ...;
> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
> return switch (a.getEnum("kind").getName()) {
> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
> case "FULL_UNROLL_UNTIL_RETURN" ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
> parsing annotations and serializing/deserializing to/from a byte array. This
> allows the annotation data to be passed from the HotSpot heap to the libgraal
> heap.
Doug Simon has updated the pull request incrementally with one additional
commit since the last revision:
[skip ci] formatting fixes
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12810/files
- new: https://git.openjdk.org/jdk/pull/12810/files/abaf2375..32131796
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=06
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=05-06
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/12810.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12810/head:pull/12810
PR: https://git.openjdk.org/jdk/pull/12810
Re: RFR: 8305425: Thread.isAlive0 doesn't need to call into the VM [v2]
Agreed, I believe there should be an hb relationship with this, if the
target is not alive.
On Mon, Apr 3, 2023, 04:50 Quan Anh Mai wrote:
> On Mon, 3 Apr 2023 09:36:39 GMT, David Holmes wrote:
>
> >> We have the strange situation where calling `t.isAlive()` on a
> `java.lang.Thread` `t`, will call into the VM (via `alive()` then
> `isAlive0()`) where the VM then examines the `eetop` field of `t` to
> extract its `JavaThread` pointer and compare it to null. We can simply read
> `eetop` directly in `Thread.alive()`:
> >>
> >> boolean alive() {
> >> return eetop != 0;
> >> }
> >>
> >> I also updated a comment in relation to `eetop`.
> >>
> >> Testing: tiers 1-3
> >>
> >> Thanks
> >
> > David Holmes has updated the pull request incrementally with one
> additional commit since the last revision:
> >
> > Comment from AlanB
>
> May I ask if we need some form of memory order here? Maybe an aquiring
> load is necessary?
>
> -
>
> PR Comment: https://git.openjdk.org/jdk/pull/13287#issuecomment-1494094776
>
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v7]
On Mon, 17 Apr 2023 15:48:53 GMT, Joe Darcy wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> [skip ci] formatting fixes
>
> src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 419:
>
>> 417: * @param type of the object representing a decoded error
>> 418: */
>> 419: public interface AnnotationDecoder {
>
> I think it would be better to include some bound on the type parameters to
> better capture their intention
> A extends java.lang.Annotatoin
> E extends java.lang.Enum
> etc.
These types are *alternatives* to `java.lang.Annotation`, `java.lang.Enum` etc.
That's the primary motivation for this PR, i.e. to be able to represent
annotations without having to reify them as `java.lang.Annotation` objects.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12810#discussion_r1169232602
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v7]
On Mon, 17 Apr 2023 15:32:56 GMT, Joe Darcy wrote:
>> Doug Simon has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> [skip ci] formatting fixes
>
> src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 234:
>
>> 232: * Encodes a list of annotations to a byte array. The byte array
>> can be decoded with {@link #decodeAnnotations(byte[], AnnotationDecoder)}.
>> 233: */
>> 234: public static byte[] encodeAnnotations(Collection
>> annotations) {
>
> I don't think it matters much in this use case, but it looks like
> encodeAnnotations could be changed to take a List rather than a Collection,
> as the comment implies.
Just above (line 228) you can see a call to this method where the argument
comes from `Map.values()` whose type is `Collection` so I'd prefer to leave it
as is rather than have to convert the argument to a `List`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12810#discussion_r1169240341
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v7]
On Mon, 17 Apr 2023 20:33:26 GMT, Doug Simon wrote:
>> src/java.base/share/classes/jdk/internal/vm/VMSupport.java line 234:
>>
>>> 232: * Encodes a list of annotations to a byte array. The byte array
>>> can be decoded with {@link #decodeAnnotations(byte[], AnnotationDecoder)}.
>>> 233: */
>>> 234: public static byte[] encodeAnnotations(Collection
>>> annotations) {
>>
>> I don't think it matters much in this use case, but it looks like
>> encodeAnnotations could be changed to take a List rather than a Collection,
>> as the comment implies.
>
> Just above (line 228) you can see a call to this method where the argument
> comes from `Map.values()` whose type is `Collection` so I'd prefer to leave
> it as is rather than have to convert the argument to a `List`.
Just above (line 228) you can see a call to this method where the argument
comes from `Map.values()` whose type is `Collection` so I'd prefer to leave it
as is rather than have to convert the argument to a `List`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12810#discussion_r1169240392
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v7]
On Mon, 17 Apr 2023 16:50:47 GMT, Joe Darcy wrote: > From the long-term perspective, it is likely that the set of kinds of > elements that can occur in an annotation will be expanded, for example, > method references are a repeated request. Easing future maintenance to gives > more inter-source linkage in this situation and error handling for this case > in the libgraal code would be prudent IMO. I'm not sure what you're suggesting in terms of how I should update this PR. Do you mean `AnnotationData.get` needs to somehow be more flexible? If so, could you please give a concrete example of what you're after. - PR Comment: https://git.openjdk.org/jdk/pull/12810#issuecomment-1512063544
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v7]
On Mon, 17 Apr 2023 16:50:47 GMT, Joe Darcy wrote: > the methods should phrase their operations in terms of these concepts... I think this is what you're suggesting: https://github.com/openjdk/jdk/pull/12810/commits/362738a61410cc8d60d8c4c4fc9e3e8ed0393aed - PR Comment: https://git.openjdk.org/jdk/pull/12810#issuecomment-1512074497
Re: RFR: 8303431: [JVMCI] libgraal annotation API [v8]
> This PR extends JVMCI with new API (`jdk.vm.ci.meta.Annotated`) for accessing
> annotations. The main differences from `java.lang.reflect.AnnotatedElement`
> are:
> * All methods in the `Annotated` interface explicitly specify requested
> annotation type(s). That is, there is no equivalent of
> `AnnotatedElement.getAnnotations()`.
> * Annotation data is returned in a map-like object (of type
> `jdk.vm.ci.meta.AnnotationData`) instead of in an `Annotation` object. This
> works better for libgraal as it avoids the need for annotation types to be
> loaded and included in libgraal.
>
> To demonstrate the new API, here's an example in terms
> `java.lang.reflect.AnnotatedElement` (which `ResolvedJavaType` implements):
>
> ResolvedJavaMethod method = ...;
> ExplodeLoop a = method.getAnnotation(ExplodeLoop.class);
> return switch (a.kind()) {
> case FULL_UNROLL -> LoopExplosionKind.FULL_UNROLL;
> case FULL_UNROLL_UNTIL_RETURN ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The same code using the new API:
>
>
> ResolvedJavaMethod method = ...;
> ResolvedJavaType explodeLoopType = ...;
> AnnotationData a = method.getAnnotationDataFor(explodeLoopType);
> return switch (a.getEnum("kind").getName()) {
> case "FULL_UNROLL" -> LoopExplosionKind.FULL_UNROLL;
> case "FULL_UNROLL_UNTIL_RETURN" ->
> LoopExplosionKind.FULL_UNROLL_UNTIL_RETURN;
> ...
> }
>
>
> The implementation relies on new methods in `jdk.internal.vm.VMSupport` for
> parsing annotations and serializing/deserializing to/from a byte array. This
> allows the annotation data to be passed from the HotSpot heap to the libgraal
> heap.
Doug Simon has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 12 commits:
- rephrased javadoc Annotated to more precisely describe which annotations are
returned
- fixed comment
- Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8303431
- [skip ci] formatting fixes
- addressed review feedback
- Merge remote-tracking branch 'openjdk-jdk/master' into JDK-8303431
- switched to use of lists and maps instead of arrays
- fixed whitespace
- added support for inherited annotations
- Merge branch 'master' into JDK-8303431
- ... and 2 more: https://git.openjdk.org/jdk/compare/525a91e3...362738a6
-
Changes: https://git.openjdk.org/jdk/pull/12810/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12810&range=07
Stats: 2319 lines in 34 files changed: 2268 ins; 23 del; 28 mod
Patch: https://git.openjdk.org/jdk/pull/12810.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/12810/head:pull/12810
PR: https://git.openjdk.org/jdk/pull/12810
