openj9 icon indicating copy to clipboard operation
openj9 copied to clipboard

Add primitive Value Type reference projection support (part 1)

Open hangshao0 opened this issue 2 years ago • 33 comments

  1. There are 2 ram classes generated from a primitive value type. One is the L type ram class and the other is the Q type ram class.
  2. The L and Q type ram class are companion of each other. They have a pointer to each other in the J9Class struct.
  3. Add new flag J9_FINDCLASS_FLAG_QTYPE to indicate if it is a Q type that is to be found or generated.
  4. Both Q type and L type ram classes are added to the class table. A new local type TYPE_QTYPE_RAM_CLASS in KeyHashTable.c is added.
  5. The same as existing classes, both L type and Q type ram classes will be keyed on the class name in the class hash table. However, if it is a Q type ram class, its hash value will be incremented by 1 (the value of TYPE_QTYPE_RAM_CLASS)
  6. Add J9_HASH_TABLE_QUERY_FLAG_QTYPE to indicate if the hash table query is for a Q type or not.

issue #14858

Signed-off-by: Hang Shao [email protected]

hangshao0 avatar May 16 '22 18:05 hangshao0

This change does not cover arrays. I will look at arrays in the next change.

hangshao0 avatar May 16 '22 19:05 hangshao0

@tajila Can you have the first look? I'm working on a customer issue.

gacholio avatar May 18 '22 04:05 gacholio

Sure Ill take a look

tajila avatar May 18 '22 13:05 tajila

How do we deal with statics? If one defines a primitive type with a static field, does the static exist in both the L and the Qtype?

Yes, the static exist in both L and Q type. I notice in the class file, statics are currently referenced the same way from both L and Q type. i.e.

in class:

primitive class PointPrimitive {
        public static int count = 100;
        ...

Both PointPrimitive.ref.count and PointPrimitive.count are referenced via Field PointPrimitive.count:I in the class file. I think sharing the statics between L type and Q type or putting them separately does not seem to make much functional difference for now.

I think that the Qtype can simply reference to the Ltype for things like methods, statics, constantPool, etc. Assuming the behaviour is that both the L and Q are essentially one class, and Im also assuming we create the L type first

I think we can share the statics, but not sure about the methods and constantPool. We have marcos like J9_CLASS_FROM_CP() and J9_CLASS_FROM_METHOD(), which will be broken if they are shared between both L and Q type.

hangshao0 avatar May 25 '22 19:05 hangshao0

Update the code to share ramStatics between L and Q type.

hangshao0 avatar Jun 01 '22 19:06 hangshao0

All existing review comments addressed.

hangshao0 avatar Jun 03 '22 18:06 hangshao0

I noticed the Valhalla build is currently broken by missing classes in the JCL:

import jdk.internal.vm.ContinuationScope;
                      ^
  symbol:   class ContinuationScope
  location: package jdk.internal.vm
/root/CCM/JdkNext/openj9vt/openj9-openjdk-jdk.valuetypes/build/linux-x86_64-server-release/support/j9jcl/java.base/share/classes/java/lang/StackWalker.java:237: error: cannot find symbol
        static StackWalker newInstance(Set<Option> options, ExtendedOption extendedOption, ContinuationScope contScope) {

Talked to @JasonFengJ9, there seem to be some JCL merge conflicts that some changes did not go into https://github.com/ibmruntimes/openj9-openjdk-jdk.valuetypes.

hangshao0 avatar Jun 14 '22 20:06 hangshao0

Updated openj9-openjdk-jdk.valuetypes @hangshao0 you might give another try.

JasonFengJ9 avatar Jun 15 '22 02:06 JasonFengJ9

you might give another try.

Things are working now. Thanks @JasonFengJ9

hangshao0 avatar Jun 15 '22 16:06 hangshao0

It seems to me this is being implemented before we understand the requirements.

Can you please describe exactly the function of the Q and L types? What is the result of Something.class? Are the statics supposed to be shared or should they only be referenced from one or the other of the classes?

I'm concerned that sharing statics may lead to Unsafe not working properly - the static address is stored as an offset from the ramStatics of the Field instance, so using the wrong class in the Unsafe call will result in a crash/corruption.

gacholio avatar Jun 16 '22 16:06 gacholio

Can you please describe exactly the function of the Q and L types?

I guess I'm really interested in the circumstances where the Q and L types would be used. This should go a long way to answering the questions about methods and statics.

gacholio avatar Jun 16 '22 17:06 gacholio

Can you please describe exactly the function of the Q and L types?

There are 2 types, reference type (L type) and value type (Q type) generated from the same VT class file. The L type is used if user want to allow NULL or atomic access. It won't be flattened in its container. Q type does not allow NULL, it does not guarantee atomic access and it can be flattened. From the JEP, the relationship between the Q type (Point) and L type (Point.ref) is similar to the traditional relationship between the types int and Integer. However, Point and Point.ref both correspond to the same class declaration; the values of both types are instances of a single Point class.

What is the result of Something.class?

From the current RI implementation, Something.class is the L type. There are 2 new fields in Class.java:

private transient Class<T> primaryType;
private transient Class<T> secondaryType;

They are set only when the class is value type. primaryType is set to the class representing the L type and secondaryType is set to the class representing the Q type.

Are the statics supposed to be shared or should they only be referenced from one or the other of the classes?

Currently the statics can only be referenced from the Q type, not the L type.

Not sue if you have anything to add @tajila

hangshao0 avatar Jun 16 '22 18:06 hangshao0

Currently the statics can only be referenced from the Q type, not the L type.

This is somewhat inconsistent with Something.class being the L type - I would expect everything to be accessed via the user-facing type. If that's not the case, there's going to be a lot of unnecessary indirection to keep flipping between the types (perhaps need to do j9class->heapClass->primaryType->j9class to get to the right place).

gacholio avatar Jun 16 '22 19:06 gacholio

There is inconsistence between the RI implementation and the JEP documentation, not sure which one they are going to update. The JEP says "a CONSTANT_Class using the plain name of a primitive class represents the class's reference type", RI JCL sets primaryType to the reference type, but the RI JVM treats CONSTANT_Class of the plain name as Q type instead of reference type, therefore statics are referenced from the Q type.

hangshao0 avatar Jun 16 '22 19:06 hangshao0

This plain name thing is silliness - there's a way to specify exactly what you want, why have this wishy-washy who-knows-what-it-is type as well?

gacholio avatar Jun 16 '22 19:06 gacholio

Are all classrefs plain types now or are they a mixture of Q, L and plain?

gacholio avatar Jun 16 '22 20:06 gacholio

They are mixture of Q, L and plain.

hangshao0 avatar Jun 16 '22 20:06 hangshao0

Are there java.lang.Class instances for both the Q and L types?

gacholio avatar Jun 17 '22 15:06 gacholio

Are there java.lang.Class instances for both the Q and L types?

Yes, one java.lang.Class instance for Q type and one java.lang.Class instance for L type.

hangshao0 avatar Jun 17 '22 15:06 hangshao0

What is the behaviour of getClass()? If used on a Q instance do you get the Q class back, or is the L type always the user-facing class? Thinking of the Field class - variables will have different offsets in the Q and L instances. Similarly for Method - are you expected to be able to use the Q and L classes to invoke (presumably identical) methods?

I don't see how a method could deal with both the Q and L instances - field resolution will pick one of the other offset and you're stuck with that regardless of which instance is passed to the method.

gacholio avatar Jun 17 '22 15:06 gacholio

What is the behaviour of getClass()? If used on a Q instance do you get the Q class back, or is the L type always the user-facing class?

getClass() returns the L type class on a Q instance. But there are public getters in Class.java that return the primaryType and secondaryType. so the Q type class can be exposed to the users as well.

Thinking of the Field class - variables will have different offsets in the Q and L instances. Similarly for Method - are you expected to be able to use the Q and L classes to invoke (presumably identical) methods?

According to a previous comment from Tobi, L type class cannot be instanciated, we could only have instance of the Q type class.

For a class like:

primitive class PointPrimitive {
        public static int count = 100;
        ...

Both PointPrimitive.ref.count (PointPrimitive.ref is the reference type in Java code) and PointPrimitive.count (PointPrimitive is the value type in Java code) are compiled to Field PointPrimitive.count:I by javac. If plain class name in Field PointPrimitive.count:I is treated as Q type, they are both accessed from the Q type. It is the same for static methods.

hangshao0 avatar Jun 17 '22 16:06 hangshao0

It's not at all confusing for the Q class to be the actual type and the L class to be what people see. :)

I guess we just need to know what happens if people use the Q class in various APIs like reflect.

As an aside does Class.newInstance() work on Q types (presumably via the L class)?

Are we linking the RAM classes so we can avoid transitioning through the objects to get from Q to L and back?

gacholio avatar Jun 17 '22 17:06 gacholio

As an aside does Class.newInstance() work on Q types (presumably via the L class)?

No, it does not work. It triggers java.lang.InstantiationException.

Are we linking the RAM classes so we can avoid transitioning through the objects to get from Q to L and back?

Yes, there is a new field J9Class* companionClass added in J9Class struct. L and Q type J9Classes point to each other via this field.

hangshao0 avatar Jun 17 '22 17:06 hangshao0

Wondering if it would make sense to add another field like primaryClass which would be the same on both Q and L, so a single fetch with no test and branch can be used to get the "correct" class (presumably, Q is almost always the interesting version).

gacholio avatar Jun 17 '22 17:06 gacholio

Given everything said above, I'd rather not share the statics - makes more sense to keep them on the Q class and make sure we're always using the Q when we access them.

gacholio avatar Jun 17 '22 17:06 gacholio

If we do want to add a new field, maybe companionClass should be renamed - we could explicitly have Q and L class pointers initialized appropriately (and for a normal class, they both point to itself). I'd rather avoid test and branch where possible.

gacholio avatar Jun 17 '22 17:06 gacholio

Do we want to set the statics (constant pool, itables, invoke cache, etc.) on the L type J9Class or leave them to NULL ? There are a lot of places they are used without NULL check as in https://github.com/eclipse-openj9/openj9/pull/15070#discussion_r899248833

hangshao0 avatar Jun 17 '22 17:06 hangshao0

Where possible, I'd like to see them left NULL. Hopefully most things will be covered by associated counters, so the NULL pointer may be read but will never actually be dereferenced.

gacholio avatar Jun 17 '22 17:06 gacholio

Noticed some counters are on the romClass (like romMethodCount for methods), which is not 0.

hangshao0 avatar Jun 17 '22 17:06 hangshao0

Then we need a different solution - duplicating the pointers will make it very easy for these things to be walked twice by the GC (which may or may not be an issue but it's a bad idea in general) or other things like DDR-based tools.

gacholio avatar Jun 17 '22 17:06 gacholio