Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8245584: [lworld] LW3 core reflection update #53

Closed
wants to merge 5 commits into from

Conversation

@mlchung
Copy link
Member

@mlchung mlchung commented May 21, 2020

This patch updates the core reflection for the new language model for inline classes.

  • Class::isInlineClass returns true for inline classes
  • Class::valueType returns the value projection of an inline class
  • Class::referenceType returns the reference projection if it's an inline class,
    or this class if it's not an inline class

If this class has no val type or ref type, Class::valueType and Class::referenceType
return an empty Optional. We can re-examine if we should modernize Class API shape
where we can rather than holding to the tradition.

I updated TestIntrinsics.java to use the new APIs for now to keep it compiled.
This test will be updated when the intrinsification of the old API Class::asPrimaryType
and Class:asIndirectType is removed.


Progress

  • Change must not contain extraneous whitespace

Issue

Reviewers

  • Frederic Parain (fparain - Committer) ⚠️ Review applies to 3efccac

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/53/head:pull/53
$ git checkout pull/53

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 21, 2020

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request.

@openjdk
Copy link

@openjdk openjdk bot commented May 21, 2020

@mlchung This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8245584: [lworld] LW3 core reflection update

Reviewed-by: fparain
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

There are currently no new commits on the lworld branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate e895128f832a783b33937e7e317c191b813aa3db.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 21, 2020

Webrevs

Copy link
Collaborator

@fparain fparain left a comment

Hi Mandy,

The HotSpot changes look good in general, with a lot of nice cleanups.
You'll find two comments, one minor about hard-coded characters, and a second more significant about the Class::refType field, for which you might want a review from the compiler team.

I find it disturbing to have VM code depending on value projection/reference projection fields when these are language concepts that are not part of the VM model. but this has nothing to do with your changes, it might be due to the model transition, or something we missed in the VM model.

Thank you,

Fred

src/hotspot/share/oops/instanceKlass.cpp Outdated Show resolved Hide resolved
src/hotspot/share/classfile/javaClasses.cpp Show resolved Hide resolved
@rose00
Copy link
Collaborator

@rose00 rose00 commented May 23, 2020

It's up to the source compiler to issue or not issue a V.ref class file. Perhaps the user has manually created one, or perhaps it is automatically generated, or perhaps the inline type is a HC and therefore cannot have been sealed to a companion type.

The JVM has to take what's there and surface it; if it's not there the JVM has to carry on. Since the core reflection APIs are mostly about what's in the classfile, they don't try to reconstruct what the source language was, but rather report any and all classfiles present.

But we do want to reflect the relation between V.ref and V.val, if both exist and are properly marked. We need a clearly stated rule for what "properly marked" means, and those rules are not nailed down yet, since we are hammering out a story where V.ref might be either an interface or an abstract class.

I suggest that the final form of the reflection code for V.ref/V.val should be written in Java, and clearly and simply implement whatever agreement we make in the end. Java code should manage the refType and valType fields, if possible. Bootstrapping issues may force them to be managed by the JVM, as in this patch, but even in that case the code should be set apart (probably as a subroutine or two) and documented clearly as relating to the above-mentioned agreement.

One comment, really for the future, regarding the shape of the Java API here: It uses Optional and omits the "get" prefix on accessors. This is the New Style, as opposed to the Classic Style using null (for "empty" results) and a "get" prefix ("getComponentType") to get a related type. We may choose to to use the New Style for new reflection API points, and if so let's not choose N different New Styles, but one New Style. Personally I like removing "get"; I accept Optional instead of null; and I also suggest that arrays (if any) be replaced by (immutable) Lists in the New Style.

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 25, 2020

Mailing list message from Remi Forax on valhalla-dev:

[CC to Brian and amber-dev] because this is relevant to the API around record too.

----- Mail original -----

De: "John R Rose" <jrose at openjdk.java.net>
?: "valhalla-dev" <valhalla-dev at openjdk.java.net>
Envoy?: Samedi 23 Mai 2020 20:51:14
Objet: Re: [lworld] RFR: 8245584: [lworld] LW3 core reflection update

One comment, really for the future, regarding the shape of the Java API here:
It uses Optional and omits the "get"
prefix on accessors. This is the New Style, as opposed to the Classic Style
using null (for "empty" results) and a
"get" prefix ("getComponentType") to get a related type. We may choose to to
use the New Style for new reflection API
points, and if so let's not choose N different New Styles, but one New Style.
Personally I like removing "get"; I
accept Optional instead of null; and I also suggest that arrays (if any) be
replaced by (immutable) Lists in the New Style.

Please no, no New Style if the Old Style is good enough,
as a user i don't want to care if something was created a long ago or recently, if the API uses "get", or not, consistency is more important,
because it creates the feeling that you understand how the API works.

I think NetworkInterface [1] is the perfect example of how to evolve an API,
because it contains a mix between old, very old and new style.

1/ you have Enumeration used instead of Iterator because it's coherent with the java.net package that was created for Java 1, even if NetworkInterface was introduced in Java 4.
even methods introduced in Java 6 uses Enumeration.
2/ recently some methods that returns a Stream were added.

I see two rules, one is when you grow an API follow the style of the API and if you have a good reason to introduce an API points with a new style, then provide both variants the old way and the new way.

regards,
R?mi

[1] https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/net/NetworkInterface.html

@openjdk
Copy link

@openjdk openjdk bot commented May 26, 2020

@mlchung this pull request can not be integrated into lworld due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout lw3-reflection
git fetch https://git.openjdk.java.net/valhalla lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 26, 2020

Mailing list message from Mandy Chung on valhalla-dev:

On 5/25/20 12:40 PM, Remi Forax wrote:

[CC to Brian and amber-dev] because this is relevant to the API around record too.

----- Mail original -----

De: "John R Rose" <jrose at openjdk.java.net>
?: "valhalla-dev" <valhalla-dev at openjdk.java.net>
Envoy?: Samedi 23 Mai 2020 20:51:14
Objet: Re: [lworld] RFR: 8245584: [lworld] LW3 core reflection update
One comment, really for the future, regarding the shape of the Java API here:
It uses Optional and omits the "get"
prefix on accessors. This is the New Style, as opposed to the Classic Style
using null (for "empty" results) and a
"get" prefix ("getComponentType") to get a related type. We may choose to to
use the New Style for new reflection API
points, and if so let's not choose N different New Styles, but one New Style.
Personally I like removing "get"; I
accept Optional instead of null; and I also suggest that arrays (if any) be
replaced by (immutable) Lists in the New Style.
Please no, no New Style if the Old Style is good enough,
as a user i don't want to care if something was created a long ago or recently, if the API uses "get", or not, consistency is more important,
because it creates the feeling that you understand how the API works.

I think NetworkInterface [1] is the perfect example of how to evolve an API,
because it contains a mix between old, very old and new style.

1/ you have Enumeration used instead of Iterator because it's coherent with the java.net package that was created for Java 1, even if NetworkInterface was introduced in Java 4.
even methods introduced in Java 6 uses Enumeration.
2/ recently some methods that returns a Stream were added.

I see two rules, one is when you grow an API follow the style of the API and if you have a good reason to introduce an API points with a new style, then provide both variants the old way and the new way.

IMO, adding both old way and the new way bloats the API unnecessarily.

Note that the java.lang.Class API implements TypeDescriptor.ofField
which already introduces the new style APIs, Class::describeConstable
and Class::arrayType etc.

Mandy

regards,
R?mi

[1] https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/net/NetworkInterface.html

@mlchung
Copy link
Member Author

@mlchung mlchung commented May 26, 2020

/integrate

@openjdk openjdk bot closed this May 26, 2020
@openjdk openjdk bot added integrated and removed ready labels May 26, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 26, 2020

@mlchung
Pushed as commit 9a83d9b.

@openjdk openjdk bot removed the rfr label May 26, 2020
@mlchung mlchung deleted the lw3-reflection branch Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants