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

8244227: [lworld] Explore an implementation where the reference projection and value projection types are backed by a single class symbol #375

Closed
wants to merge 11 commits into from

Conversation

sadayapalam
Copy link
Collaborator

@sadayapalam sadayapalam commented Mar 31, 2021

This PR overhauls the existing implementation/representation of reference projection types and the inline types so that both share the same underlying symbol, much like how an infinite family of parameterized types that originate from the same generic type are modelled using the same underlying symbol.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8244227: [lworld] Explore an implementation where the reference projection and value projection types are backed by a single class symbol

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 375

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 31, 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 bot commented Mar 31, 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:

8244227: [lworld] Explore an implementation where the reference projection and value projection types are backed by a single class symbol

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 2 new commits pushed to the lworld branch:

  • f816f29: 8264085: [lworld] C2 compilation fails with assert "inline type should be loaded"
  • 937f9b3: 8264586: [lworld] C2 compilation fails due to infinite loop in PhaseIterGVN::optimize

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 bot commented Mar 31, 2021

}

public ClassType(Type outer, List<Type> typarams, TypeSymbol tsym,
TypeMetadata metadata) {
this(outer, typarams, tsym, metadata, false);
}

Copy link
Collaborator Author

@sadayapalam sadayapalam Mar 31, 2021

Choose a reason for hiding this comment

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

I have rewired most of the constructor invocations to the new one which handles a reference projection flag parameter. A few call sites are still left in for the old constructors, these seem harmless.

return (t.tsym != s.tsym || t.isReferenceProjection() == s.isReferenceProjection()) ?
asSuper(t, s.tsym) : null;
}

Copy link
Collaborator Author

@sadayapalam sadayapalam Mar 31, 2021

Choose a reason for hiding this comment

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

Not all calls to the original method asSuper(Type, Symbol) have been rerouted to here. A few calls it appears should not be. I will add more analysis on why that is so later tomorrow.

I haven't also fully studied whether asSub, asEnclosingSuper, asOuterSuper etc need similar variant, it seems not. This is to be studied and addressed in a follow up task.

@sadayapalam
Copy link
Collaborator Author

sadayapalam commented Mar 31, 2021

Here are some notes that should help in the review process. I have broken down the changes involved into six logical units each forming a separate commit. Of these, the very first ("In diagnostics and debug strings while mentioning projection types, use .ref/.val rather than $ref/$val") and perhaps even the second commit ("Get out of the business of having to maintain the doppleganger symbol in sync with original") can be skipped or cursorily glanced through.

Real action starts at the third commit ("Genesis, construction, preservation and propagation of reference projection type") and proceeds through the next three commits.

@sadayapalam
Copy link
Collaborator Author

sadayapalam commented Mar 31, 2021

Overall, I see two major challenges with the new approach (although it is simpler in many ways and has much to like about vis a vis what we have now):

  1. Types.asSuper(Type, Symbol) would answer non-null for Foo.ref, Foo.val.tsym being passed in because they both have the same sym and deep down in Types.asSuper we return nonnull if the symbol is equal. So, I have to invent a version of Types.asSuper(Type, Type) instead and reroute the calls to that. However this was tricky in that there are places we do want asSuper to answer non-null when the Type is reference projection and the Symbol is primitive class. For example given Foo<T> { void add(T) {} } a call to Types.memberType(Foo.ref, add(String)) internally results in a call to asSuper(Foo.ref<String>, Foo<T>) -
  2. There are places where we go from Symbol -> AST using the TreeMaker, - these Tree factory methods internally set the AST node type from the symbol. This results in "reference projectionness" being lost. The problem is to mechanically identify the places where such loss may occur (and recover, which is easy to do). ATM, I fixed the places where the tests failed thereby indicating type loss. But this approach is heavily dependent on coverage being good, and our tests are still not super good there being a work in progress (See that 2 is not a problem for parameterized types because after TransType, type arguments are gone AND more importantly the erasure of all parameterized types of the same generic class is the same, whereas we want Foo.ref and Foo.val to share a symbol but erase differently.)

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

This seems like a good cleanup overall. IMHO the issues can be bucketized as follows:

  • some more experiment is required to see if we can avoid changing asSuper too much
  • there is code which will be simplified if we come to a point where javac can let go of generating two classes instead of one.
  • there is a bigger issue (which we'll have to confront with, sooner or later, because of specialization) regarding type erasure kicking in too early, and subsequent pipeline steps only dealing with symbol (not type) info

Since the approach moves things in the way generic types are modeled, I'm not surprised to see some of the issues I had to face when writing a backend for the specializer compiler pop back here. I'd say, for now, let's try to deal with the first bullet (which is localized with these changes), and let's come back (in a separate PR) with some workaround for the last bullet (perhaps borrowing some old valhalla code?).

} else {
return combineMetadata(erased, t);
// erasure(projection(primitive)) = projection(erasure(primitive))
Type erased = eraseClassType(t, recurse);
Copy link
Collaborator

@mcimadamore mcimadamore Apr 1, 2021

Choose a reason for hiding this comment

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

Random non-code comment: this seems a case of erasure applied to early. The first time that happens in lworld, but in the old branch which featured generic specializations, at some point we decided to postpone type erasure, as we needed sharp generic types during code gen. This smells like more of the same - e.g. having to fight with erasure so that the info that we care about is preserved in some form.

Copy link
Collaborator

@mcimadamore mcimadamore Apr 1, 2021

Choose a reason for hiding this comment

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

There is also the issue that, no matter how delayed erasure is, at the moment we need to generate two different class artifacts, so, at some point the symbol duality is going to pop back up; that's understandable - hopefully we'll come up with a strategy which doesn't require javac to generate two artifacts - at which point javac can simply insert conversions when going from L to Q - and mostly be done with it (of course to generate such conversions it will need access to full type info - see above).

}
return javaFileObject;
}

// where
private static ClassSymbol getReferenceProjection(ClassSymbol c) {
Copy link
Collaborator

@mcimadamore mcimadamore Apr 1, 2021

Choose a reason for hiding this comment

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

Maybe one day we will be able to remove this...

startSbp,
sbp - startSbp));

name = names.fromUtf(signatureBuffer,
Copy link
Collaborator

@mcimadamore mcimadamore Apr 1, 2021

Choose a reason for hiding this comment

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

This is another bit which might get simpler if we end up generating only a single .class.

sadayapalam added 3 commits Apr 5, 2021
…s subtyping determination; Merge Two flavors of Types.asSuper into one; Make sure to switch to projection types for sucessful hierarchy lookup via asSuper
@openjdk openjdk bot removed ready rfr labels Apr 8, 2021
@sadayapalam
Copy link
Collaborator Author

sadayapalam commented Apr 8, 2021

Hello Maurzio, Please take another look. I have reverted all the earlier changes to asSuper per suggestion and transformed calls to asSuper into isSubtype calls where possible (This is not always possible - see the elaborate javadoc comment attached to Types.asSuper - in fact starting the review there would be a good thing)

There are still a few calls to asSuper that are not studied through the lens of the twin hazards - these are calls from Types itself and Infer.java - outside of these two files I have studied all call sites of asSuper and suitably amended them and added tests to guard against any regressions and prove the need for and correctness of the transformation. I decided to defer the study of these remaining call sites to the pre-existing ticket JDK-8244712 - Javac should switch to reference projection before walking type hierarchy. (This ticket would include analysis of not only the remaining asSuper calls, but also isSubtype calls)

Of the two commits (ignoring the merge related ones), one of them is simply a revert of earlier change for asSuper and can be ignored.

Let me know if you see any further issues that block integration. Thanks!

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

In general, it looks good. I have three main comments:

  • the refProjectionOrSelf trick is a good one, I believe
  • consider using reference projection always when doing asSuper (e.g. adding that projection inside asSuper, directly)
  • I believe the patch is replacing too many use sites with subtyping. There are several checks where we want to assert that T is an instance of C, where C can be AutoCloseable, Iterable, Serializable. It is not clear to me as to why we'd want these replaced with subtyping - in the sense that here we seem to be asking about a propoerty of the primitive class declaration rather than the particular projection we happen to have on hand.

Note that, as we're exploring new territory, it is very possible that I have overlooked some underlying issue here - but thanks for taking the time to guide me through all the problems!

@sadayapalam
Copy link
Collaborator Author

sadayapalam commented Apr 8, 2021

In general, it looks good. I have three main comments:

  • the refProjectionOrSelf trick is a good one, I believe
  • consider using reference projection always when doing asSuper (e.g. adding that projection inside asSuper, directly)

Will experiment with this and report back. This could minimize code changes.

  • I believe the patch is replacing too many use sites with subtyping. There are several checks where we want to assert that T is an instance of C, where C can be AutoCloseable, Iterable, Serializable. It is not clear to me as to why we'd want these replaced with subtyping - in the sense that here we seem to be asking about a propoerty of the primitive class declaration rather than the particular projection we happen to have on hand.

Possible. The rule of thumb I used for changing calls to asSuper into isSubtype was simply, whether (a) the call site is interested in the return value of asSuper as opoosed to just checking whether it is null or not and (b) whether generic class symbol could be passed in as the second argument to original asSuper call and if so retain it as asSuper itself.

So it is likely more sites are changed than strictly necessary. The way I see it some of the overeaget changes to isSubtype result in equivalent semantics with no regression. Still it is worth minimizing ripples and for that reason I will attempt to restore these.

Note that, as we're exploring new territory, it is very possible that I have overlooked some underlying issue here - but thanks for taking the time to guide me through all the problems!

I am grappling with similar challenges. I.e clearly characterizing which calls involve hazards of which kind. This involves reasoning about every site.

…when the goal is instanceof determination, the former is good enough.
@sadayapalam
Copy link
Collaborator Author

sadayapalam commented Apr 8, 2021

I pushed a commit accommodating two review comments:

  1. asSuper calls at various places employed for the purpose of instanceof determination can be left alone.
  2. Javadoc comment attached to asSuper is stronger than it ought to be and asserts something that is not quite true.

The third suggestion around asSuper internally coercing its argument into reference projection is something that will be taken up in a subsequent PR for JDK-8244712

@Maurizio, I'll proceed with integration if you give a quick glance through over the latest commit and approve. TIA

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks good - thanks for following up!

@sadayapalam
Copy link
Collaborator Author

sadayapalam commented Apr 9, 2021

/integrate

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

openjdk bot commented Apr 9, 2021

@sadayapalam Since your change was applied there have been 2 commits pushed to the lworld branch:

  • f816f29: 8264085: [lworld] C2 compilation fails with assert "inline type should be loaded"
  • 937f9b3: 8264586: [lworld] C2 compilation fails due to infinite loop in PhaseIterGVN::optimize

Your commit was automatically rebased without conflicts.

Pushed as commit c68ce89.

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

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