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

8266466: [lworld] Enhance javac to consume unified primitive class files generated under the option -XDunifiedValRefClass #421

Closed
wants to merge 4 commits into from

Conversation

@sadayapalam
Copy link
Collaborator

@sadayapalam sadayapalam commented May 21, 2021

Post the integration of https://bugs.openjdk.java.net/browse/JDK-8244227,
the constructor of ClassType, takes a boolean isReferenceProjection as an additional
parameter. Till now, it was feasible to upfront decide at the time of constructing
a class type whether the proposed type is supposed to be a reference projection or not.

This is no longer possible under the unified class file generation scheme we are
moving to. When we see LFoo; in field/method signatures/descriptors, we don't know
whether the type is Foo.ref of a primitive class Foo or a plain old reference type.

Likewise when we see QFoo; we have an ambiguity whether this is the primitive class
type Foo or is Foo.val type of a ref-default primitive class Foo. Till a class is
fully built ("completed"), we don't know whether it is a primitive class and if so,
what its ref-val defaultness is.

Completing class Foo every time we see a type descriptor LFoo; or QFoo; is wasteful
of resources. Nor is it possible as it is, since it would call for the ClassReader's
code to be reentered even as some other class is being read in: So the proposed patch
implements an incremental piece meal augmentation of attributes.

I suggest the review be started with Type.java in ClassType#Flavor enumeration to gather
a high level picture of the abstractions put in place to characterize a class type in
an incremental basis. After Type.java, Attr.java, ClassReader.java and Symbol.java in
that order provide a good order for review.

Please note, while this patch puts together the infrastructure and layes the groundwork
for modelling ref-default classes and thereby enable VBC migration work, such work will
arrive separately at a later time.

As of now javac knows only about primitive class and their reference projections and
plain old reference types. There is no notion of ref-val defaultness yet, Nor a way
to declare them.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8266466: [lworld] Enhance javac to consume unified primitive class files generated under the option -XDunifiedValRefClass

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/valhalla pull/421/head:pull/421
$ git checkout pull/421

Update a local copy of the PR:
$ git checkout pull/421
$ git pull https://git.openjdk.java.net/valhalla pull/421/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 421

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/valhalla/pull/421.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 21, 2021

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

@openjdk
Copy link

@openjdk openjdk bot commented May 21, 2021

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

8266466: [lworld] Enhance javac to consume unified primitive class files generated under the option -XDunifiedValRefClass

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 1 new commit pushed to the lworld branch:

  • ae4ce74: 8267503: [lworld] [lw3] NPE message thrown by checkcast is incorrect

Please see this link for an up-to-date comparison between the source branch of this pull request and the lworld 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 lworld branch, type /integrate in a new comment.

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 21, 2021

Webrevs

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Overall, a clever strategy.

I'm a bit doubtful that we have seen the end of the string here. In particular, when it comes to LFoo; ambiguities (is it a regular reference Foo or a projection Foo.ref of some primitive type?) I think the old and trusted signature attribute also has a role to play.

The signature attribute will in fact require updates also to support universal generics.

} else {
requireProjection = false;
// We are seeing QFoo; or LFoo; The name itself does not shine any light on default val-refness
flavor = prefix == 'L' ? Flavor.L_TypeOf_X : Flavor.L_TypeOf_X;
Copy link
Collaborator

@mcimadamore mcimadamore May 21, 2021

Choose a reason for hiding this comment

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

Is there a typo here - shouldn't be:

                        flavor = prefix == 'L' ? Flavor.L_TypeOf_X : Flavor.Q_TypeOf_X;

Copy link
Collaborator Author

@sadayapalam sadayapalam May 21, 2021

Choose a reason for hiding this comment

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

Oops. Yes it should be as you say. I am puzzled because this was copy pasted code. Don't know how the error got introduced! Good catch! The other two places do the right thing. Hmm.

* The 'flavor' of a ClassType indicates its reference/primitive projectionness
* viewed against the default nature of the associated class.
*/
public enum Flavor {
Copy link
Collaborator

@mcimadamore mcimadamore May 21, 2021

Choose a reason for hiding this comment

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

Perhaps you could add a method which "upgrades" a flavor to a new one, given new info.

e.g.

L_TypeOf_X.update(sym.isPrimitiveClass()) ->  L_TypeOf_Q/L_TypeOf_L (depending on isPrimitiveClass)

This would allow you to rule out (e.g. throw) bad updates. E.g. X flavors can be updated, all the other "leaf" flavors can only be updated to self.

I think a method such as this would remove a lot of conditional logic in the clients.

Copy link
Collaborator Author

@sadayapalam sadayapalam May 21, 2021

Choose a reason for hiding this comment

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

I'll take a look ...

Copy link
Collaborator Author

@sadayapalam sadayapalam May 24, 2021

Choose a reason for hiding this comment

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

Perhaps you could add a method which "upgrades" a flavor to a new one, given new info.

e.g.

L_TypeOf_X.update(sym.isPrimitiveClass()) ->  L_TypeOf_Q/L_TypeOf_L (depending on isPrimitiveClass)

This would allow you to rule out (e.g. throw) bad updates. E.g. X flavors can be updated, all the other "leaf" flavors can only be updated to self.

I think a method such as this would remove a lot of conditional logic in the clients.

Thanks, this turned out to be a nice suggestion and helped simplify the code at 3 or 4 places.

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented May 21, 2021

Overall, a clever strategy.

I'm a bit doubtful that we have seen the end of the string here. In particular, when it comes to LFoo; ambiguities (is it a regular reference Foo or a projection Foo.ref of some primitive type?) I think the old and trusted signature attribute also has a role to play.

The signature attribute will in fact require updates also to support universal generics.

You are right that this is not the end of the story. The implementation deals with just a single bit ACC_PRIMITIVE and weaves that into the type system - so as of this patch, we can have plain old L_TypeOf_L, L_Typeof_Q and Q_TypeOf_Q as fully "evolved forms" and X_Typeof_X, Q_Typeof_X and L_Typeof_X as the "larval forms".

BUT, the ambiguity you right about i.e "In particular, when it comes to LFoo; ... (is it a regular reference Foo or a projection Foo.ref of some primitive type?)" is handled already in this patch. The single bit ACC_PRIMITIVE is enough for this.

See com.sun.tools.javac.code.Type.ClassType#isReferenceProjection which disambiguates L_TypeOf_X into one of L_TypeOf_Q or L_TypeOf_L.

I plan to take up work on JDK-8244231 right after this, at which point, a larval form may see other evolutions.

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented May 24, 2021

/integrate

@openjdk openjdk bot closed this May 24, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 24, 2021

@sadayapalam Since your change was applied there has been 1 commit pushed to the lworld branch:

  • ae4ce74: 8267503: [lworld] [lw3] NPE message thrown by checkcast is incorrect

Your commit was automatically rebased without conflicts.

Pushed as commit f9806a4.

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

@sadayapalam sadayapalam deleted the JDK-8266466 branch May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants