8265518: C1: Intrinsic support for Preconditions.checkIndex #3615
Conversation
|
|
|
@kelthuzadx The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
/label add hotspot-compiler |
|
@kelthuzadx |
|
@kelthuzadx |
|
/label add core-libs |
|
@kelthuzadx |
|
Can I have a review of this PR? Thanks! |
| if ((i < 0) || (i >= limit)) | ||
| throw new IndexOutOfBoundsException(); | ||
| return i; | ||
| return Objects.checkIndex(i, limit); |
There was a problem hiding this comment.
Changing Buffer.checkIndex to use Objects.checkIndex is okay.
|
@veresov please look on C1 changes. |
| inline_target->name() == ciSymbols::checkIndex_name()) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we need to keep this flag?
There was a problem hiding this comment.
Do we need to keep this flag?
Exactly, the flag should be removed.
Thanks,
Yang
| try { | ||
| Preconditions.checkIndex(1, 1, null); | ||
| } catch (IndexOutOfBoundsException e) { | ||
| // got it! | ||
| } |
There was a problem hiding this comment.
In all places where Precondition.checkIndex is expected to throw, an AssertionError should be generated if it doesn't throw:
try {
Preconditions.checkIndex(1, 1, null);
throw new AssertionError("Expected IndexOutOfBoundsException not thrown");
} catch (IndexOutOfBoundsException e) {
// got it!
}
There was a problem hiding this comment.
Yes, it does make sense
| @@ -78,7 +82,8 @@ static void check0() { | |||
|
|
|||
| try { | |||
| // read fields | |||
| Preconditions.checkIndex(limit + 1, limit, (s, integers) -> new MyException("Reason:" + s +"::" + integers)); | |||
| Preconditions.checkIndex(limit + 1, limit, (s, integers) -> new MyException("Reason:" + s + "::" + integers)); | |||
| throw new AssertionError("Expected IndexOutOfBoundsException not thrown"); | |||
There was a problem hiding this comment.
nit: well maybe here it should be:
throw new AssertionError("Expected MyException not thrown");
|
|
||
| static void check1(int i) { | ||
| try { | ||
| Preconditions.checkIndex(i, 9999, (s, integers) -> new RuntimeException("ex")); |
There was a problem hiding this comment.
I believe
throw new AssertionError("Expected IndexOutOfBoundsException not thrown");
should be added in check1...check4 as well...
There was a problem hiding this comment.
Hmm.. They would throw desired exceptions only if i==9999. Nevertheless, I will tweak this test later.
Thanks for taking this feedback into account and updating the test!
Note: I only reviewed the test.
|
@kelthuzadx This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 142 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
|
|
It was my hope this would eventually happen when we added |
|
Looks like now the test fails in the pre-submit tests? |
Hi Paul, Thank you for noticing this PR.
It seems that Object.checkIndex can not meet our needs because it implicitly passes null to Preconditions.checkIndex, but we want to customize exception messages, so we might add extra APIs in Objects while doing the replacement.
Best Regards, |
Hi Igor, Can you take a look at the latest version? Now it passes all pre-submit tests. Best Regards, |
It might be possible to directly use I would caution against adding a public API to support this. The work in Paul. |
|
Hi @veresov, may I ask your help to review this patch? Thanks a lot. |
|
Thank you @veresov! I'm glad to have more comments from hotspot-compiler group. Later, I'd like to integrate it if there are no more comments/objections. Thanks! |
|
@veresov I've rebased to the latest commit and resolved all conflicts. Please take a look at this. Thank you! |
|
|
|
Alright, tests pass now. I think we're good to go. |
|
/sponsor |
|
@veresov This change does not need sponsoring - the author is allowed to integrate it. |
|
I guess you need to do the "integrate" command again. |
Okay，thank you all for taking time to look at this /integrate |
|
Going to push as commit 5cee23a.
Your commit was automatically rebased without conflicts. |
|
@kelthuzadx Pushed as commit 5cee23a. |
|
This change removed a product flag so I wonder how it could be integrated without a CSR? |
And if the intention was to remove it, should it not have been marked as obsolete first? |
It's a diagnostic product flag, so it’ okay to remove it without issuing CSR. But I am not 100% sure. @dholmes-ora, do you have any comment about this? Thanks! |
|
Looks like you are right Yi:
https://wiki.openjdk.java.net/display/HotSpot/Hotspot+Command-line+Flags%3A+Kinds%2C+Lifecycle+and+the+CSR+Process
Seems like for diagnostic flags the creation of a CSR is up to the
developer.
Sorry for the confusion.
Yi Yang ***@***.***> schrieb am Sa., 12. Juni 2021, 08:54:
… This change removed a product flag so I wonder how it could be integrated
without a CSR?
It's a diagnostic product flag, so it’ okay to remove it without issuing
CSR. But I am not 100% sure.
@dholmes-ora <https://github.com/dholmes-ora>, do you have any comment
about this? Thanks!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3615 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYH5VPC6ZMY76YNJ4C4S4DTSMABPANCNFSM43L2BATA>
.
|
|
Hi Yi, you may need to add the option to the obsolete-flags-table though as described in arguments.cpp: jdk/src/hotspot/share/runtime/arguments.cpp Lines 489 to 490 in 5cee23a I think the point is to give a customer a grace period where the option is still accepted on the command line. I am not sure if that step is optional though, if one is reasonably sure that the option is unused. Maybe @dholmes-ora can chime in. Cheers, Thomas |
Hi Thomas, I think what you said is right. It does not take too much time to do this but it can give users a smooth transition for unavailable options! I will create a new PR to do this stuff if there are no objections. Thanks, |
|
It's up to you, we can of course print a warning that the flag has been removed. In my opinion, we shouldn't waste time on this, that was an obscure non-product flag. |
The JDK codebase re-created many variants of checkIndex(
grep -I -r 'cehckIndex' jdk/). A notable variant is java.nio.Buffer.checkIndex, which annotated with @IntrinsicCandidate and it only has a corresponding C1 intrinsic version.In fact, there is an utility method
jdk.internal.util.Preconditions.checkIndex(wrapped by java.lang.Objects.checkIndex) that behaves the same as these variants of checkIndex, we can replace these re-created variants of checkIndex by Objects.checkIndex, it would significantly reduce duplicated code and enjoys performance improvement because Preconditions.checkIndex is @IntrinsicCandidate and it has a corresponding intrinsic method in HotSpot.But, the problem is currently HotSpot only implements the C2 version of Preconditions.checkIndex. To reuse it global-widely in JDK code, I think we can firstly implement its C1 counterpart. There are also a few kinds of stuff we can do later:
Testing: cds, compiler and jdk
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3615/head:pull/3615$ git checkout pull/3615Update a local copy of the PR:
$ git checkout pull/3615$ git pull https://git.openjdk.java.net/jdk pull/3615/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3615View PR using the GUI difftool:
$ git pr show -t 3615Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3615.diff