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

8237073: [lworld] Need special handling of jlO constructor invocation #481

Closed
wants to merge 5 commits into from

Conversation

sadayapalam
Copy link
Collaborator

@sadayapalam sadayapalam commented Jul 15, 2021

Translate new Object() instantiations down to invovations of java.util.Objects.newIdentity() call with an informational message at compie time


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8237073: [lworld] Need special handling of jlO constructor invocation

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 481

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 15, 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 Jul 15, 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:

8237073: [lworld] Need special handling of jlO constructor invocation

Reviewed-by: thartmann, hseigel, fparain, mchung

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:

  • b6e7836: 8268945: [lworld] Re-implement JDK-8267151 after merge
  • 9b5ae0a: 8270896: [lworld] C2 compilation fails with "Meet Not Symmetric"

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 Jul 15, 2021

Webrevs

@mlchung
Copy link
Member

mlchung commented Jul 16, 2021

For src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java, I suggest to replace it with a specific class like this:

diff --git a/src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java b/src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java
index b3c149492ac..a0707150cfa 100644
--- a/src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java
+++ b/src/java.base/share/classes/java/lang/invoke/ClassSpecializer.java
@@ -152,12 +152,14 @@ abstract class ClassSpecializer<T,K,S extends ClassSpecializer<T,K,S>.SpeciesDat
         return new IllegalArgumentException(message, cause);
     }
 
-    private static final Function<Object, Object> CREATE_RESERVATION = new Function<>() {
-        @Override
-        public Object apply(Object key) {
-            return new Object();
-        }
-    };
+    static class CacheHolder {
+        static final Function<Object, Object> CREATE = new Function<>() {
+            @Override
+            public Object apply(Object key) {
+                return new CacheHolder();
+            }
+        };
+    }
 
     public final S findSpecies(K key) {
         // Note:  Species instantiation may throw VirtualMachineError because of
@@ -180,12 +182,12 @@ abstract class ClassSpecializer<T,K,S extends ClassSpecializer<T,K,S>.SpeciesDat
         // concrete class if ever.
         // The concrete class is published via SpeciesData instance
         // returned here only after the class and species data are linked together.
-        Object speciesDataOrReservation = cache.computeIfAbsent(key, CREATE_RESERVATION);
+        Object speciesDataOrReservation = cache.computeIfAbsent(key, CacheHolder.CREATE);
         // Separating the creation of a placeholder SpeciesData instance above
         // from the loading and linking a real one below ensures we can never
         // accidentally call computeIfAbsent recursively.
         S speciesData;
-        if (speciesDataOrReservation.getClass() == Object.class) {
+        if (speciesDataOrReservation.getClass() == CacheHolder.class) {
             synchronized (speciesDataOrReservation) {
                 Object existingSpeciesData = cache.get(key);
                 if (existingSpeciesData == speciesDataOrReservation) { // won the race

@sadayapalam
Copy link
Collaborator Author

sadayapalam commented Jul 16, 2021

Thanks Mandy, I will include your changes. I assume you have taken a look at the test/jdk/ changes too

@mlchung
Copy link
Member

mlchung commented Jul 16, 2021

I assume you have taken a look at the test/jdk/ changes too

They are dynalink tests that I'm not familiar with. I suspect it can be replaced with an instance of other class. I will take a look.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Compiler test changes look good to me.

@@ -183,7 +183,7 @@ public void afterTest() {
public void getPropertyTest(final boolean publicLookup) throws Throwable {
final MethodType mt = MethodType.methodType(Object.class, Object.class, String.class);
final CallSite cs = createCallSite(publicLookup, GET_PROPERTY, mt);
Assert.assertEquals(cs.getTarget().invoke(new Object(), "class"), Object.class);
Assert.assertEquals(cs.getTarget().invoke(new Object(), "class"), java.util.Objects.newIdentity().getClass());
Copy link
Member

@mlchung mlchung Jul 16, 2021

Choose a reason for hiding this comment

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

The change looks okay but this may be unclear to the reader why the returned class is not Object.class.

An alternative fix is to define an explicit TestObject class:

diff --git a/test/jdk/jdk/dynalink/BeanLinkerTest.java b/test/jdk/jdk/dynalink/BeanLinkerTest.java
index fafc1be447f..f22aab8f026 100644
--- a/test/jdk/jdk/dynalink/BeanLinkerTest.java
+++ b/test/jdk/jdk/dynalink/BeanLinkerTest.java
@@ -179,11 +179,13 @@ public class BeanLinkerTest {
         this.linker = null;
     }
 
+    static class TestObject {}
+
     @Test(dataProvider = "flags")
     public void getPropertyTest(final boolean publicLookup) throws Throwable {
         final MethodType mt = MethodType.methodType(Object.class, Object.class, String.class);
         final CallSite cs = createCallSite(publicLookup, GET_PROPERTY, mt);
-        Assert.assertEquals(cs.getTarget().invoke(new Object(), "class"), Object.class);
+        Assert.assertEquals(cs.getTarget().invoke(new TestObject(), "class"), TestObject.class);
         Assert.assertEquals(cs.getTarget().invoke(new Date(), "class"), Date.class);
     }
 
@@ -191,14 +193,14 @@ public class BeanLinkerTest {
     public void getPropertyNegativeTest(final boolean publicLookup) throws Throwable {
         final MethodType mt = MethodType.methodType(Object.class, Object.class, String.class);
         final CallSite cs = createCallSite(publicLookup, GET_PROPERTY, mt);
-        Assert.assertNull(cs.getTarget().invoke(new Object(), "DOES_NOT_EXIST"));
+        Assert.assertNull(cs.getTarget().invoke(new TestObject(), "DOES_NOT_EXIST"));
     }
 
     @Test(dataProvider = "flags")
     public void getPropertyTest2(final boolean publicLookup) throws Throwable {
         final MethodType mt = MethodType.methodType(Object.class, Object.class);
         final CallSite cs = createCallSite(publicLookup, GET_PROPERTY, "class", mt);
-        Assert.assertEquals(cs.getTarget().invoke(new Object()), Object.class);
+        Assert.assertEquals(cs.getTarget().invoke(new TestObject()), TestObject.class);
         Assert.assertEquals(cs.getTarget().invoke(new Date()), Date.class);
     }
 
@@ -208,7 +210,7 @@ public class BeanLinkerTest {
         final CallSite cs = createCallSite(publicLookup, GET_PROPERTY, "DOES_NOT_EXIST", mt);
 
         try {
-            cs.getTarget().invoke(new Object());
+            cs.getTarget().invoke(new TestObject());
             throw new RuntimeException("Expected NoSuchDynamicMethodException");
         } catch (final Throwable th) {
             Assert.assertTrue(th instanceof NoSuchDynamicMethodException);

test/jdk/jdk/dynalink/TrustedDynamicLinkerFactoryTest.java Outdated Show resolved Hide resolved
Copy link
Member

@hseigel hseigel left a comment

The hotspot test changes look good.
Thanks, Harold

@sadayapalam
Copy link
Collaborator Author

sadayapalam commented Jul 20, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Jul 20, 2021

Going to push as commit 4609bad.
Since your change was applied there have been 2 commits pushed to the lworld branch:

  • b6e7836: 8268945: [lworld] Re-implement JDK-8267151 after merge
  • 9b5ae0a: 8270896: [lworld] C2 compilation fails with "Meet Not Symmetric"

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Jul 20, 2021

@sadayapalam Pushed as commit 4609bad.

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

@sadayapalam sadayapalam deleted the JDK-8237073 branch Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants