Methods and tests for jep334 Class #4157

theresa-m · 2018-12-31T16:33:00Z

Class.componentType
Class.describeConstable
Class.descriptorString
Class arrayType

Depends on (see #4195):

Fix Java 12 build failures: Fix Java 12 build failures #3980
Constants API stubs: Stubs for Constants API (jep334) #4030
Java12andUp test setup: Setup for Java12andUp tests (jep334) #4104

Signed-off-by: Theresa Mammarella Theresa.T.Mammarella@ibm.com

runtime/vm/BytecodeInterpreter.hpp

runtime/vm/FastJNI_java_lang_Class.cpp

runtime/vm/BytecodeInterpreter.hpp

runtime/vm/FastJNI_java_lang_Class.cpp

gacholio · 2019-01-07T18:41:41Z

@andrewcraik New native Class.arrayType should be recognized by the JIT if we this think it will be called a lot. The expected case is a few instructions, so making a call every time seems wasteful. The sequence should be:

result = clazz->arrayClass;
if (NULL == result) {
   // unexpected - put in snippet?
   result = clazz.arrayType();
}

theresa-m · 2019-01-08T15:35:27Z

fixing error in testng.xml

runtime/vm/BytecodeInterpreter.hpp

theresa-m · 2019-01-09T16:28:47Z

Found an issue with my Class.describeConstable implementation when I was testing VarHandleDesc. I've added more tests to Test_Class to reproduce the issue and fixed describeConstable

gacholio

I have reviewed only the code for the new native.

pshipton · 2019-02-13T22:35:21Z

Class implements TypeDescriptor.OfField<Class<?>>, Constable
http://cr.openjdk.java.net/~iris/se/12/build/latest/api/java.base/java/lang/Class.html

theresa-m · 2019-02-14T15:21:46Z

extra implements are included here #4030

DanHeidinga · 2019-02-14T15:38:40Z

jcl/src/java.base/share/classes/java/lang/Class.java

+	 * not represent an array.
+	 */
+	public Class<?> componentType() {
+		return this.getComponentType();


no need for the this.

DanHeidinga · 2019-02-14T16:24:02Z

test/functional/Java12andUp/src/org/openj9/test/java_lang/Test_Class.java

+	}
+
+	private void describeConstableTestGeneral(String testName, Class<?> testClass) {
+		Optional<ClassDesc> optionalDesc = testClass.describeConstable();


use .orElseThrow() and let the test code throw if the Optional is empty. The test harness knows how to fail tests for unexpected exceptions.

DanHeidinga · 2019-02-14T16:28:02Z

test/functional/Java12andUp/src/org/openj9/test/java_lang/Test_Class.java

+ * - componentType(): wrapper for Class.getComponentType(), not tested
+ */
+public class Test_Class {
+    public static Logger logger = Logger.getLogger(Test_Class.class);


tabs vs spaces

DanHeidinga · 2019-02-14T16:28:59Z

test/functional/Java12andUp/src/org/openj9/test/java_lang/Test_Class.java

+			Class<?> resolvedClass;
+			try {
+				resolvedClass = (Class<?>)desc.resolveConstantDesc(MethodHandles.lookup());
+			} catch(ReflectiveOperationException e) {


Let this code throw - the test framework knows how to fail tests for this

DanHeidinga · 2019-02-14T16:36:39Z

test/functional/Java12andUp/src/org/openj9/test/java_lang/Test_Class.java

+	public void testClassDescriptorString() {
+		/* primitive type test */
+		logger.debug("testClassDescriptorString: Primitive test result is: " + primitiveTest.descriptorString() + " expected: " + primitiveResult);
+		Assert.assertTrue(primitiveTest.descriptorString().equals(primitiveResult));


There are a small number of primitives - Z, B, C, S, I, F, D, J, & V. Seems like we should test them all:

Something like:

Object[][] primitiveData = { { boolean.class, "Z" }, { byte.class, "B" }, { char.class, "C" }, { short.class, "S" }, { int.class, "I" }, { float.class, "F" }, { double.class, "D" }, { long.class, "J" }, { void.class, "V" } }; for (Object[] params : primitiveData) { Assert.assertEquals(((Class<?>)params[0]).descriptorString(), params[1]); }

DanHeidinga · 2019-02-14T16:39:22Z

test/functional/Java12andUp/src/org/openj9/test/java_lang/Test_Class.java

+
+		/* ArrayType test */
+		logger.debug("testClassDescriptorString: Array test result is: " + arrayTest.descriptorString() + " expected: " + arrayResult);
+		Assert.assertTrue(arrayTest.descriptorString().equals(arrayResult));


Something similar to the suggestion above can be done for the primitive array cases.... actually, the Object subclasses can be rolled into the same kind of tests

DanHeidinga · 2019-02-14T17:12:10Z

test/functional/Java12andUp/src/org/openj9/test/java_lang/Test_Class.java

+		String arrayString = array.descriptorString();
+		logger.debug(testName + ": Descriptor of component is: " + result + " descriptor of array is: " + arrayString);
+		Assert.assertTrue(arrayString.equals("[" + result));
+	}


Can you also add a test case for the class of a lambda expression?

Runnable r = () -> System.out.println("a"); // This should succeed ClassDesc desc = r.getClass().describeConstanble(); desc.getDescriptorString() should also succeed but the value returned will be different in each run but resolveConstantDesc(MethodHandles.lookup()) should fail

theresa-m · 2019-02-19T15:45:58Z

I added a helper for the arrayType() method that throws an IllegalArgumentException when the instance class is void.class to replicate the ri's behavior.

DanHeidinga

Apart from the very minor naming convention, this looks good

DanHeidinga · 2019-02-21T16:30:16Z

jcl/src/java.base/share/classes/java/lang/Class.java

 	}

+	private native Class<?> arrayTypeHelper();


The more typical naming convention is adding impl to the method name

I just pushed the rename.

DanHeidinga · 2019-02-21T18:49:08Z

Jenkins compile win jdk11

DanHeidinga · 2019-02-21T18:49:20Z

Jenkins test sanity zlinux jdk12

theresa-m · 2019-02-21T19:17:57Z

sorry that push was a mistake :(

DanHeidinga · 2019-02-21T19:21:13Z

@theresa-m Is this branch back to being ready to PR test and merge?

theresa-m · 2019-02-21T19:25:12Z

Right now it's dependent on #4105 only for the reason that the rest of the Java12andUp xml files are there. I just assumed String would be merged first since it was approved at the time. I can also move that commit with the other test files here so the tests will run properly

theresa-m · 2019-02-21T19:27:58Z

okay I've just added the rest of the test files to this pr too. now it won't matter which one goes in first

tests will now be run properly for pr builds here

theresa-m · 2019-02-21T22:37:59Z

test/functional/Java12andUp/src/org/openj9/test/java_lang/Test_Class.java

+		String originalDescriptor = testClass.descriptorString();
+		String newDescriptor = resolvedClass.descriptorString();
+		logger.debug(testName + ": Descriptor of original class is: " + originalDescriptor + " descriptor of ClassDesc is: " + newDescriptor);
+		Assert.assertTrue(testClass.equals(resolvedClass));


one last improvement to this line. previously was comparing strings

Assert.assertTrue(originalDescriptor.equals(newDescriptor));

- Class.componentType - Class.describeConstable - Class.descriptorString - Class arrayType Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

DanHeidinga · 2019-02-22T19:21:58Z

Jenkins test sanity zlinux jdk12

theresa-m · 2019-02-22T21:01:05Z

failing tests are related to #4661

DanHeidinga · 2019-02-22T21:10:50Z

Same known failures - com.ibm.j9.jsr292.api.MethodHandleAPI_defineClass.test_defineClass_ClassBytes_CorruptedArray

gacholio reviewed Jan 1, 2019

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Show resolved Hide resolved

gacholio reviewed Jan 1, 2019

View reviewed changes

runtime/vm/FastJNI_java_lang_Class.cpp Show resolved Hide resolved

theresa-m force-pushed the jep334_class branch 2 times, most recently from 523fa4d to 0f9b798 Compare January 3, 2019 00:35

gacholio reviewed Jan 3, 2019

View reviewed changes

runtime/vm/FastJNI_java_lang_Class.cpp Outdated Show resolved Hide resolved

theresa-m force-pushed the jep334_class branch from 0f9b798 to 18686bf Compare January 3, 2019 15:51

gacholio reviewed Jan 4, 2019

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Show resolved Hide resolved

gacholio reviewed Jan 5, 2019

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Show resolved Hide resolved

theresa-m force-pushed the jep334_class branch from 18686bf to 8c6327c Compare January 7, 2019 14:51

gacholio reviewed Jan 7, 2019

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Outdated Show resolved Hide resolved

gacholio reviewed Jan 7, 2019

View reviewed changes

runtime/vm/FastJNI_java_lang_Class.cpp Outdated Show resolved Hide resolved

theresa-m force-pushed the jep334_class branch 3 times, most recently from ce37313 to 5b73f6e Compare January 8, 2019 15:35

This was referenced Jan 8, 2019

JEP 334 Constants API pr tracking #4195

Closed

Methods and tests for jep334 MethodType #4194

Merged

gacholio reviewed Jan 8, 2019

View reviewed changes

runtime/vm/BytecodeInterpreter.hpp Outdated Show resolved Hide resolved

theresa-m force-pushed the jep334_class branch 2 times, most recently from 9290593 to a84e8a0 Compare January 9, 2019 16:27

gacholio approved these changes Jan 9, 2019

View reviewed changes

theresa-m force-pushed the jep334_class branch from 85c48a1 to e754615 Compare January 23, 2019 19:37

This was referenced Jan 23, 2019

Method and tests for jep334 MethodHandle #4412

Merged

Jep334 VarHandle and VarHandleDesc #4449

Merged

pshipton added comp:vm jdk12 labels Jan 28, 2019

pshipton added this to the Release 0.13.0 (Java 12) milestone Jan 28, 2019

pshipton added this to In progress in JEP 334 via automation Jan 28, 2019

pshipton mentioned this pull request Feb 13, 2019

New methods in Class for jdk12 #4733

Closed

DanHeidinga reviewed Feb 14, 2019

View reviewed changes

theresa-m force-pushed the jep334_class branch from e754615 to 504d2c9 Compare February 19, 2019 15:41

DanHeidinga approved these changes Feb 21, 2019

View reviewed changes

DanHeidinga self-assigned this Feb 21, 2019

theresa-m force-pushed the jep334_class branch from 504d2c9 to 6f7659f Compare February 21, 2019 18:42

theresa-m force-pushed the jep334_class branch from e87f7f1 to 6f7659f Compare February 21, 2019 19:16

theresa-m force-pushed the jep334_class branch from f8fa669 to fc4a0c0 Compare February 21, 2019 22:37

theresa-m commented Feb 21, 2019

View reviewed changes

theresa-m force-pushed the jep334_class branch from fc4a0c0 to 722a0ce Compare February 22, 2019 19:16

Methods and tests for jep334 Class

e416518

- Class.componentType - Class.describeConstable - Class.descriptorString - Class arrayType Signed-off-by: Theresa Mammarella <Theresa.T.Mammarella@ibm.com>

theresa-m force-pushed the jep334_class branch from 722a0ce to e416518 Compare February 22, 2019 19:17

DanHeidinga merged commit 1440bcf into eclipse-openj9:master Feb 22, 2019

JEP 334 automation moved this from In progress to Done Feb 22, 2019

theresa-m deleted the jep334_class branch February 25, 2019 14:15

Charlmzz mentioned this pull request Jul 24, 2019

Add enumerateFields api to create a TypeLayout object #6570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Methods and tests for jep334 Class #4157

Methods and tests for jep334 Class #4157

theresa-m commented Dec 31, 2018 •

edited

gacholio commented Jan 7, 2019 •

edited

theresa-m commented Jan 8, 2019

theresa-m commented Jan 9, 2019

gacholio left a comment

pshipton commented Feb 13, 2019

theresa-m commented Feb 14, 2019

DanHeidinga Feb 14, 2019

DanHeidinga Feb 14, 2019

DanHeidinga Feb 14, 2019

DanHeidinga Feb 14, 2019

DanHeidinga Feb 14, 2019

DanHeidinga Feb 14, 2019

DanHeidinga Feb 14, 2019

theresa-m commented Feb 19, 2019

DanHeidinga left a comment

DanHeidinga Feb 21, 2019

theresa-m Feb 21, 2019

DanHeidinga commented Feb 21, 2019

DanHeidinga commented Feb 21, 2019

theresa-m commented Feb 21, 2019

DanHeidinga commented Feb 21, 2019

theresa-m commented Feb 21, 2019

theresa-m commented Feb 21, 2019 •

edited

theresa-m Feb 21, 2019

DanHeidinga commented Feb 22, 2019

theresa-m commented Feb 22, 2019

DanHeidinga commented Feb 22, 2019

Methods and tests for jep334 Class #4157

Methods and tests for jep334 Class #4157

Conversation

theresa-m commented Dec 31, 2018 • edited

gacholio commented Jan 7, 2019 • edited

theresa-m commented Jan 8, 2019

theresa-m commented Jan 9, 2019

gacholio left a comment

Choose a reason for hiding this comment

pshipton commented Feb 13, 2019

theresa-m commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theresa-m commented Feb 19, 2019

DanHeidinga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga commented Feb 21, 2019

DanHeidinga commented Feb 21, 2019

theresa-m commented Feb 21, 2019

DanHeidinga commented Feb 21, 2019

theresa-m commented Feb 21, 2019

theresa-m commented Feb 21, 2019 • edited

Choose a reason for hiding this comment

DanHeidinga commented Feb 22, 2019

theresa-m commented Feb 22, 2019

DanHeidinga commented Feb 22, 2019

theresa-m commented Dec 31, 2018 •

edited

gacholio commented Jan 7, 2019 •

edited

theresa-m commented Feb 21, 2019 •

edited