8304283: Modernize the switch statements in jdk.internal.foreign #13047

minborg · 2023-03-15T18:10:29Z

This PR proposes changing old-type switch statements to newer forms of switch.

Progress

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

Issue

JDK-8304283: Modernize the switch statements in jdk.internal.foreign

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to 779d1ee2

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13047

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

Using diff file

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

bridgekeeper · 2023-03-15T18:11:31Z

👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-15T18:13:54Z

@minborg The following label will be automatically applied to this pull request:

core-libs

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.

mlbridge · 2023-03-15T18:17:00Z

Webrevs

mcimadamore · 2023-03-15T18:33:34Z

src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java

-    );
+    static boolean isUnbox(Binding binding) {
+        return switch (binding) {
+            case Binding.VMStore unused -> true;


I wonder... here it might be better to capture the box/unbox nature of a binding in a virtual method in the binding class? E.g. isBox(), isUnbox() ?

I prefer this approach, to be honest, since the logic for all the different binding operators is in one place. It gives an overview of which operators are expected in an unboxing recipe. Making them virtual methods would put quite a bit of visual distance between the different true/false values.

I've been through the Binding file quite a bit, and the amount of scrolling/searching needed to find a particular implementation is kind of annoying.

To pull on that string some more. I think we should move the implementations of various Binding::verify methods here, or perhaps into a separate BindingVerifier class.

Ditto for the Binding::interpret implementations. They could be moved to BindingInterpreter. The Binding class would just be left as a simple bag of records + documentation for each operator.

The main motivation for this would be that, right now, if you're looking at e.g. interpretation in Binding, there's a lot of noise that you have to filter through. I think it makes more sense to group these things into classes (for specialization/verification/interpretation), where all the code in a class is logically related.

mcimadamore · 2023-03-15T18:34:50Z

src/java.base/share/classes/jdk/internal/foreign/abi/riscv64/linux/TypeClass.java

-                return flatten(sequenceLayout.elementLayout()).mul(elementCount);
-            } else {
-                throw new IllegalStateException("Cannot get here: " + layout);
+                case null, default -> throw new IllegalStateException("Cannot get here: " + layout);


Since the default throws, and the switch w/o a case null also throws, do we need the case null here?

No. Null is not needed. I will remove it.

mcimadamore

Overall looks good. I've added a couple of optional comments for your consideration.

openjdk · 2023-03-15T18:38:19Z

@minborg This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8304283: Modernize the switch statements in jdk.internal.foreign

Reviewed-by: mcimadamore

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

7277bb1: 8293324: ciField.hpp has two methods to return field's offset
b7945bc: 8303154: Investigate and improve instruction cache flushing during compilation
eefbaa2: 8283400: [macos] a11y : Screen magnifier does not reflect JRadioButton value change
42dd907: 8302906: AArch64: Add SVE backend support for vector unsigned comparison
2b81fae: 8303022: "assert(allocates2(pc)) failed: not in CodeBuffer memory" When linking downcall handle
be08a25: 8304264: Debug messages always show up for NativeGSS
1ae69e3: 8304287: Problemlist java/net/SocketOption/OptionsTest.java
116627d: 8304267: JDK-8303415 missed change in Zero Interpreter
824a5e4: 8284047: Harmonize/Standardize the SSLSocket/SSLEngine/SSLSocketSSLEngine test templates
7ad48ea: 8300317: vmTestbase/nsk/stress/strace/strace* tests fail with "ERROR: wrong lengths of stack traces"

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

JornVernee · 2023-03-15T19:33:19Z

src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java

-    );
+    static boolean isUnbox(Binding binding) {
+        return switch (binding) {
+            case Binding.VMStore unused -> true;


I prefer this approach, to be honest, since the logic for all the different binding operators is in one place. It gives an overview of which operators are expected in an unboxing recipe. Making them virtual methods would put quite a bit of visual distance between the different true/false values.

I've been through the Binding file quite a bit, and the amount of scrolling/searching needed to find a particular implementation is kind of annoying.

JornVernee · 2023-03-15T19:38:53Z

src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java

+    static boolean isUnbox(Binding binding) {
+        return switch (binding) {
+            case Binding.VMStore unused -> true;
+            case Binding.VMLoad unused -> false;
+            case Binding.BufferStore unused -> false;
+            case Binding.BufferLoad unused -> true;
+            case Binding.Copy unused -> true;
+            case Binding.Allocate unused -> false;
+            case Binding.BoxAddress unused -> false;
+            case Binding.UnboxAddress unused -> true;
+            case Binding.Dup unused -> true;
+            case Binding.Cast unused-> true;
+        };
+    }


I'd go a bit further here and visually organize the cases as well. Also, using a static import for Binding.*:

Suggested change

static boolean isUnbox(Binding binding) {

return switch (binding) {

case Binding.VMStore unused -> true;

case Binding.VMLoad unused -> false;

case Binding.BufferStore unused -> false;

case Binding.BufferLoad unused -> true;

case Binding.Copy unused -> true;

case Binding.Allocate unused -> false;

case Binding.BoxAddress unused -> false;

case Binding.UnboxAddress unused -> true;

case Binding.Dup unused -> true;

case Binding.Cast unused-> true;

};

}

static boolean isUnbox(Binding binding) {

return switch (binding) {

case VMStore unused -> true;

case BufferLoad unused -> true;

case Copy unused -> true;

case UnboxAddress unused -> true;

case Dup unused -> true;

case Cast unused -> true;

case VMLoad unused -> false;

case BufferStore unused -> false;

case Allocate unused -> false;

case BoxAddress unused -> false;

};

}

It's a bit unfortunate that we need variable names as well.

While I agree that some "visitor" methods would be better dealt with using patterns, I remain unconvinced about the box/unbox classification being modeled as an "operation". That's because it's a static property of the binding - e.g. given a binding you know whether it can be used for box/unbox/both/none. If this was an enum, I would encode it as a boolean field and never look back. Also note how the javadoc for Binding itself makes quite a lot of comments on box/unbox nature of bindings, and how they can have different semantics depending on the direction. To me it feels like that distinction is a first class citizen in Binding.

Perhaps, pulling together the various strings, it would maybe possible to define very simple records for the various bindings, with no verification and interpretation code (e.g. leave that to some pattern-based visitor somewhere else). But I think I would still expect a binding class to know whether it's used for unbox or not w/o having to run a complex operation all the time (given that the property is a constant of the binding class). The fact that we're using records for bindings and we can't have an abstract class also biases the decision a bit (otherwise we could simply use a bunch of constructor parameters to encode the box/unbox properties).

That said, this is all subjective. I don't have super strong opinion on this. The code above looks a tad odd to me, but I can live with it.

I've spoken to the Amber persons and soon we will be able to do:

static boolean isBox(Binding binding) { return switch (binding) { case VMLoad _, Copy _, Allocate _, BoxAddress _, BufferStore _, Dup _, Cast _-> true; case VMStore _, BufferLoad _, UnboxAddress _ -> false; }; }

JornVernee · 2023-03-15T19:51:15Z

src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java

-    );
+    static boolean isUnbox(Binding binding) {
+        return switch (binding) {
+            case Binding.VMStore unused -> true;


To pull on that string some more. I think we should move the implementations of various Binding::verify methods here, or perhaps into a separate BindingVerifier class.

Ditto for the Binding::interpret implementations. They could be moved to BindingInterpreter. The Binding class would just be left as a simple bag of records + documentation for each operator.

The main motivation for this would be that, right now, if you're looking at e.g. interpretation in Binding, there's a lot of noise that you have to filter through. I think it makes more sense to group these things into classes (for specialization/verification/interpretation), where all the code in a class is logically related.

minborg · 2023-03-16T12:09:51Z

/integrate

openjdk · 2023-03-16T12:11:18Z

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

7277bb1: 8293324: ciField.hpp has two methods to return field's offset
b7945bc: 8303154: Investigate and improve instruction cache flushing during compilation
eefbaa2: 8283400: [macos] a11y : Screen magnifier does not reflect JRadioButton value change
42dd907: 8302906: AArch64: Add SVE backend support for vector unsigned comparison
2b81fae: 8303022: "assert(allocates2(pc)) failed: not in CodeBuffer memory" When linking downcall handle
be08a25: 8304264: Debug messages always show up for NativeGSS
1ae69e3: 8304287: Problemlist java/net/SocketOption/OptionsTest.java
116627d: 8304267: JDK-8303415 missed change in Zero Interpreter
824a5e4: 8284047: Harmonize/Standardize the SSLSocket/SSLEngine/SSLSocketSSLEngine test templates
7ad48ea: 8300317: vmTestbase/nsk/stress/strace/strace* tests fail with "ERROR: wrong lengths of stack traces"

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-16T12:11:28Z

@minborg Pushed as commit dfc7214.

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

Modernize the switch statements in jdk.internal.foreign

ea882e2

openjdk bot added the rfr Pull request is ready for review label Mar 15, 2023

Update copyright years

779d1ee

openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 15, 2023

mcimadamore reviewed Mar 15, 2023

View reviewed changes

mcimadamore approved these changes Mar 15, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 15, 2023

JornVernee reviewed Mar 15, 2023

View reviewed changes

minborg added 3 commits March 16, 2023 09:16

Reintroduce missing comment

e9b157e

Update after comments

f20f351

Pretty case statements

4c9fea7

openjdk bot added the integrated Pull request has been integrated label Mar 16, 2023

openjdk bot closed this Mar 16, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8304283: Modernize the switch statements in jdk.internal.foreign #13047

8304283: Modernize the switch statements in jdk.internal.foreign #13047

minborg commented Mar 15, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

mlbridge bot commented Mar 15, 2023 •

edited

Loading

mcimadamore Mar 15, 2023

JornVernee Mar 15, 2023

JornVernee Mar 15, 2023 •

edited

Loading

mcimadamore Mar 15, 2023

minborg Mar 16, 2023

mcimadamore left a comment

openjdk bot commented Mar 15, 2023 •

edited

Loading

JornVernee Mar 15, 2023

JornVernee Mar 15, 2023

mcimadamore Mar 16, 2023

minborg Mar 16, 2023

JornVernee Mar 15, 2023 •

edited

Loading

minborg commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

8304283: Modernize the switch statements in jdk.internal.foreign #13047

8304283: Modernize the switch statements in jdk.internal.foreign #13047

Conversation

minborg commented Mar 15, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 15, 2023

openjdk bot commented Mar 15, 2023

mlbridge bot commented Mar 15, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JornVernee Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcimadamore left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JornVernee Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

minborg commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

minborg commented Mar 15, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 15, 2023 •

edited

Loading

JornVernee Mar 15, 2023 •

edited

Loading

openjdk bot commented Mar 15, 2023 •

edited

Loading

JornVernee Mar 15, 2023 •

edited

Loading