8369654: javac OutOfMemoryError for complex intersection type #28050
Conversation
|
👋 Welcome back vromero! A progress list of the required criteria for merging this PR into |
|
@vicente-romero-oracle 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: 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 12 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. ➡️ To integrate this PR with the above commit message to the |
|
@vicente-romero-oracle 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
|
| List<Type> mec = closureMin(cl); | ||
| List<Type> mec = null; | ||
| if (lubWillBeErased && !cl.isEmpty()) { | ||
| cl = List.of(cl.head); |
There was a problem hiding this comment.
we don't need to deal with the whole list of types from here as it will be erased
There was a problem hiding this comment.
This seems a bit unsound because we're just picking the first erased supertype. Maybe it's ok because all closures are sorted by subtyping (so all supertypes come after subtypes).
Another possible approach would be not to bother with lub, and to have a simpler routine in Code that:
- obtains all the erased supers of both types
- "minimize" them, to find the smallest set of common erased supertypes
- pick randomly one element in the minimized set
(because lub is a bit of an overkill is then we're going to erase the result anyway). Perhaps it would be good to write both versions, and then decide on which one seems minimally invasive. Of course it's a tricky trade off -- on the one hand by doubling down on lub, we can reuse some of the machinery there, but we also pull in more than we should.
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
Outdated
Show resolved
Hide resolved
|
I am not the expert on types, and leveraging the fact that we will erase the type later is clever. But, when I was trying to debug this, it seemed the reason is that we are repeatedly and recursively asking I also tried [1] diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
index 0c155caa56d..3da5c0703bd 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
@@ -3984,6 +3984,45 @@ public Type lub(List<Type> ts) {
* does not exist return the type of null (bottom).
*/
public Type lub(Type... ts) {
+ TypeListKey key = new TypeListKey(this, ts);
+ Type result = lubCache.get(key);
+
+ if (result == null) {
+ lubCache.put(key, result = computeLub(ts));
+ }
+
+ return result;
+ }
+ private record TypeListKey(Types thisTypes, Type... types) {
+ @Override
+ public int hashCode() {
+ int hashCode = 0;
+ for (Type t : types) {
+ hashCode = 127 * hashCode + thisTypes.hashCode(t);
+ }
+ return hashCode;
+ }
+ @Override
+ public boolean equals(Object obj) {
+ if (!(obj instanceof TypeListKey other)) {
+ return false;
+ }
+ if (thisTypes != other.thisTypes) {
+ return false;
+ }
+ if (types.length != other.types.length) {
+ return false;
+ }
+ for (int i = 0; i < types.length; i++) {
+ if (!thisTypes.isSameType(types[i], other.types[i])) {
+ return false;
+ }
+ }
+ return true;
+ }
+ }
+ private Map<TypeListKey, Type> lubCache = new HashMap<>();
+ private Type computeLub(Type... ts) {
final int UNKNOWN_BOUND = 0;
final int ARRAY_BOUND = 1;
final int CLASS_BOUND = 2; |
…java Co-authored-by: Andrey Turbanov <[email protected]>
correct, that's the reason, the recursion, but in general I don't think that you can cache the lub results based on the input. As the output will depend not only on the input. See the cache already existing for the Types::merge method. That method is invoked by Types::lub and it itself invokes lub or not. So if Types::merge finds that it itself invoked Types::lub for its current input, then it will refrain from calling Typess::lub again for the same input as this would be an infinite recursion situation. So depending on what is on this merge cache the output of Types::lub could be different for the same input |
|
some data points to have an idea of the number of times we are invoking lub for the test case included in the description above. Assuming that we run it with different definitions of ClientA and ClientB changing the number of super types so for for 2 supertypes we would have: and so on. I got the table below:
which is exponential on I think this is: e^x < f(x) < 3^(x+3) for x > 3 PS: System out of resources for |
|
Note for reviewers: I think that the current solution is incomplete and still incorrect. We need something more complex for the case when both types are non primitive arrays |
Also, for context -- yesterday @vicente-romero-oracle and I explored a possible solution that doesn't need go through lub -- after all here we're after finding a common erased supertype between the two types under test. While finding common erased super types is a part of lub (see JLS), lub also does a lot more stuff. In principle the code we need might be as simple as: But this simple solution seems to have issues, so additional investigation is required. @lahodaj am I correct in assuming that the types to be merged in Code are always either class types or array types? E.g. no type variables, intersection types, or other "weird" stuff? |
|
Yes, I think the types here should not be weird (even primitive types are probably handled by the |
| buf.append(erasure(sup)); | ||
| } | ||
|
|
||
| public List<Type> erasedSupertypes(Type t) { |
There was a problem hiding this comment.
no code changes just made it public plus indentation change
| } | ||
| return arraySuperType; | ||
| private Type arraySuperType; | ||
| public Type arraySuperType() { |
There was a problem hiding this comment.
no code changes just made it public plus indentation change
| } | ||
| } | ||
|
|
||
| private Type allArray(Type... ts) { |
There was a problem hiding this comment.
this code has been extracted from lub so there will be some duplicity here
|
I have updated the PR improving the array support, basically the previous approach was arriving to a wrong supertype for the case of two reference arrays. There is now some code that is common to a portion of lub which we could probably extract to a common method in Types and just reuse it in Code but not without some massage anyways that could slow the general lub a bit so probably not worthy as lub is a very established code that I don't think will change in the near future. Thanks for all the comments so far |
| } | ||
|
|
||
| private List<Type> getErasedSuperTypes(Type t) { | ||
| if (t.hasTag(TYPEVAR)) { |
There was a problem hiding this comment.
this can use Types.skipTypeVars?
liach
left a comment
liach
left a comment
There was a problem hiding this comment.
This iteration is much cleaner and more sensible than the initial version.
mcimadamore
left a comment
mcimadamore
left a comment
There was a problem hiding this comment.
Very good -- thanks!
thanks for the suggestions! |
|
/integrate |
|
Going to push as commit 9f97200.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@vicente-romero-oracle Pushed as commit 9f97200. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
5 participants
Javac is throwing an OOME for the code like:
the reason is that after JDK-8353565 we are using Types::lub when joining jump chains in Code. The result of the lub will be erased anyways so the proposal here is to cut in the result given by lub types that will be erased anyways. The reason for the slow lub invocation here is that in order to obtain the lub in this case, the lub method is invoked several times. Each invocation, minus the first one, with very complex types which makes them very slooooow.
TIA
Progress
Warning
8369654: javac OutOfMemoryError for complex intersection typeIssue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28050/head:pull/28050$ git checkout pull/28050Update a local copy of the PR:
$ git checkout pull/28050$ git pull https://git.openjdk.org/jdk.git pull/28050/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28050View PR using the GUI difftool:
$ git pr show -t 28050Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28050.diff
Using Webrev
Link to Webrev Comment