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

8273301: [lworld] Bootstrap of instance-capturing lambda fails for reference favoring primitive types. #545

Closed
wants to merge 2 commits into from

Conversation

jespersm
Copy link
Contributor

@jespersm jespersm commented Sep 2, 2021

Fix the test by adjusting bootstrap parameters, like it was fixed in JDK-8271583.


Progress

  • Change must not contain extraneous whitespace

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8273301

Issue

  • JDK-8273301: [lworld] Bootstrap of instance-capturing lambda fails for reference favoring primitive types ⚠️ Title mismatch between PR and JBS. ⚠️ Issue is not open.

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 545

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

Using diff file

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

…avoring primitive types.

Adjust bootstrap parameters, like it was fixed in JDK-8271583.
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 2, 2021

👋 Welcome back jespersm! 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 openjdk bot added the rfr label Sep 2, 2021
@mlbridge
Copy link

mlbridge bot commented Sep 2, 2021

Webrevs

@jespersm jespersm changed the title 8273301: Bootstrap of instance-capturing lambda fails for reference favoring primitive types. 8273301: [lworld] Bootstrap of instance-capturing lambda fails for reference favoring primitive types. Sep 5, 2021
@sadayapalam
Copy link
Collaborator

sadayapalam commented Sep 14, 2021

Looks good to me - However, I would like to ask Mandy whether we are working around a problem in the runtime that should really be fixed there. @mlchung Could you please take a look at the code generated with and without fix and see if the change in javac emitted code is the proper way for this to be fixed ?

The failure mode is

Caused by: java.lang.invoke.LambdaConversionException: Invalid receiver type primitive X; not a subtype of implementation type primitive X

The failing code being:

public java.util.function.Supplier<X> makeCopier();
    descriptor: ()Ljava/util/function/Supplier;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokedynamic #7,  0              // InvokeDynamic #0:get:(LX;)Ljava/util/function/Supplier;
         6: areturn
      LineNumberTable:
        line 12: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       7     0  this   QX;

and the patch here alters the generated code to be:

 public java.util.function.Supplier<X> makeCopier();
    descriptor: ()Ljava/util/function/Supplier;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokedynamic #7,  0              // InvokeDynamic #0:get:(QX;)Ljava/util/function/Supplier;
         6: areturn
      LineNumberTable:
        line 12: 0
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
            0       7     0  this   QX;

@sadayapalam
Copy link
Collaborator

sadayapalam commented Sep 14, 2021

Still says title mismatch - Not sure why

@mlchung
Copy link
Member

mlchung commented Sep 16, 2021

A reference to a primitive object can be converted to a primitive value and vice versa. So I think the runtime should allow the receiver type of factoryType is a primitive reference type whereas the implementation type is a primitive value type of the same primitive class.

@mlchung
Copy link
Member

mlchung commented Sep 16, 2021

I'm not sure if the javac fix is needed. The factoryType describes the CallSite signature. @dansmithcode can you confirm if (LX;)Ljava/util/function/Supplier; is considered as a valid signature where X is primitive class? If valid, this issue does not require javac fix.

@dansmithcode
Copy link
Collaborator

dansmithcode commented Sep 16, 2021

The Java 17 version of LambdaMetafactory requires that the parameter types of factoryType exactly match the implementation types.

I'm not sure whether there have been modifications in Valhalla. We could conceivably make some changes if it's useful.

Eventually, we'd like to align the conversions supported by LambdaMetafactory for all parameters/returns with those supported by asType (JDK-8174983). But I'm not sure which conversions asType would support in the context of Valhalla...

@jespersm
Copy link
Contributor Author

jespersm commented Sep 16, 2021

The change to the runtime in JDK-8174983 will allow boxing and unboxing: In lworld terms, I suppose this would map to turning a QFoo (primitive class on-stack) into an LFoo (reference to primitive class), and vice versa (although I'm not sure how null references would be handled). There are obvious reasons why this is a useful feature for LambdaMetafactory.

However, I find it odd for javac not to generate identical code for a callsite depending on whether or not the enclosing class is "reference-favoring" or not. This should only influence uses of the type, not the type itself. Primitive class instance methods should still "operate on" instances (QFoo), not references to instance (LFoo).

So, I think that the javac should be consistent regarding the emitting of receiver-types, even if for the sake of the conceptual consistency itself.

@mlchung
Copy link
Member

mlchung commented Sep 17, 2021

I discussed with Dan offline to follow up my question in what cases javac should emit (LFoo;)LR; vs (QFoo;)LR;. For the lambda case, having the receiver type as QFoo; is a good thing to do as it's an instance method of the enclosing class.

For a method reference o::m where o is of primitive reference type, the factory type would be (LFoo;)LR;. I have a test case that verifies the emitted code is:

28: invokedynamic #38,  0             // InvokeDynamic #1:get:(LX;)Ljava/util/function/Supplier;

In short, it's good to fix javac to emit QFoo; as the receiver type if it's a primitive class.

@jespersm
Copy link
Contributor Author

jespersm commented Sep 24, 2021

Based on @mlchung 's comment from last week, it would be correct to integrate this.

/integrate

@openjdk
Copy link

openjdk bot commented Sep 24, 2021

@jespersm This PR has not yet been marked as ready for integration.

@jespersm
Copy link
Contributor Author

jespersm commented Sep 29, 2021

@mlchung : I've tested, and can confirm that both your fix on the lambda side of things and my fix on the compiler side (this PR) will make the attached test case pass.

However, from your comments above, quoting a discussion with Dan, I'm suggesting that javac is tightened up, as the smaller fix, so to speak, i.e. that we integrate this PR.

I then suggest that your fix should be brought under the JDK-8174983 umbrella, since the L->Q and Q->L conversions are conceptually similar, and thus useful for other users of LambdaMetafactory.

If you agree, @sadayapalam can review this bug as it.

@mlchung
Copy link
Member

mlchung commented Sep 29, 2021

@mlchung : I've tested, and can confirm that both your fix on the lambda side of things and my fix on the compiler side (this PR) will make the attached test case pass.

Thanks for confirming it. I also create JDK-8274399 to fix the runtime.

However, from your comments above, quoting a discussion with Dan, I'm suggesting that javac is tightened up, as the smaller fix, so to speak, i.e. that we integrate this PR.

Yes, we agree that it's good to fix javac as well. You need @sadayapalam to review your PR before integration. I recalled Srikanath was on vacation and I'm not sure whether he's back.

I then suggest that your fix should be brought under the JDK-8174983 umbrella, since the L->Q and Q->L conversions are conceptually similar, and thus useful for other users of LambdaMetafactory.

It's related to JDK-8174983. We can fix JDK-8274399 in lworld branch while JDK-8174983 can be fixed separately in the main line.

If you agree, @sadayapalam can review this bug as it.

Yes I agree.

@openjdk
Copy link

openjdk bot commented Jan 7, 2022

@jespersm 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 8273301-ref-lambda
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

@openjdk openjdk bot added merge-conflict and removed rfr labels Jan 7, 2022
@sadayapalam
Copy link
Collaborator

sadayapalam commented Jan 10, 2022

Jesper, I think this PR may be abandoned and the related JBS ticket closed as Not an Issue.
Via https://bugs.openjdk.java.net/browse/JDK-8279431 all support for ref-default classes has
been ripped out - these are subsumed by value classes - the so called B2 classes - which are
'L' types (reference types) really. Value classes are reference types that are final, whose instances are
immutable after construction, lack identity and are marked as ACC_VALUE in class files.

The whole inlining/flattening thing is an "optimization" - so these classes are reference classes for
all practical purposes. Against that backdrop I think this PR has lost its relavance,

@jespersm
Copy link
Contributor Author

jespersm commented Jan 19, 2022

Abandoning this, no longer relevant.

@jespersm jespersm closed this Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants