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

Support JEP-370 (Part 1) #8330

Merged
merged 9 commits into from
Feb 8, 2020
Merged

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented Jan 17, 2020

Related: #8292.

1. Remove empty stubs and use OpenJDK classes

VarHandles.makeMemoryAddressViewHandle is needed to support JEP-370.

To consume OpenJDK's VarHandles without any amends, we need to use the following
OpenJDK classes:

  1. AddressVarHandleGenerator
  2. VarHandleByteArrayBase
  3. VarHandleMemoryAddressBase

JEP-370 is a Java 14 feature. So, the empty OpenJ9 stubs for the above
classes have been removed or disabled in Java 14.

2. Use OpenJDK's VarForm and support a new VarHandle constructor

OpenJDK's VarForm is needed to support JEP-370. So, the empty OpenJ9
stub for VarForm is disabled in Java 14.

A new VarHandle constructor is also needed to support JEP-370.

VarHandle(VarForm varForm) {
	/* TODO: Translate {VarForm varForm} -> {Class<?> fieldType,
	 * Class<?>[] coordinateTypes, MethodHandle[] handleTable,
	 * int modifiers}
	 */
}

3. Support VarHandles.makeFieldHandle

VarHandles.makeFieldHandle relies upon following methods:

  • MethodHandleNatives.objectFieldOffset
  • MethodHandleNatives.staticFieldBase
  • MethodHandleNatives.staticFieldOffset

Stubs for the above three methods are added in OpenJ9's MethodHandleNatives.

OpenJ9's FieldVarHandle doesn't depend on VarHandles.makeFieldHandle.
OpenJ9 has its own implementation to support FieldVarHandle. So, the
above methods do not need to be implemented.

4. Support abstract method accessModeTypeUncached in OpenJ9's VarHandle

Java classes generated from X-VarHandle*.template implement the
VarHandle.accessModeTypeUncached abstract method.

This method has been introduced in OpenJ9's VarHandle to support JEP-370.

OpenJ9's implementation of ArrayVarHandle, FieldVarHandle and ViewVarHandle
do not utilize VarHandle.accessModeTypeUncached. So, they implement this
method as an empty stub.

5. Update MemberName to support JEP-370

MemberName(Method method) constructor is needed to support OpenJ9.

boolean isStatic() method is added to build VarHandles.makeFieldHandle. It does
not need an implementation since OpenJ9 implements its own FieldVarHandle and
does not rely upon VarHandles.makeFieldHandle.

6. Extend LambdaForm.BasicType to support JEP-370

In OpenJDK's AddressVarHandleGenerator, returnInsn and loadInsn rely upon
LambdaForm.BasicType.

The following values are added in LambdaForm.BasicType enum: I_TYPE, J_TYPE,
F_TYPE, D_TYPE and V_TYPE, in order to support the above functions.

BasicType.basicType(Class<?> cls) will need to be implemented in order to
support JEP-370.

7. Update AccessMode and SignatureType in VarHandle to support JEP-370

AddressVarHandleGenerator.addAccessModeTypeMethod accesses a field named at
in VarHandle.AccessMode enum. This field is currently named as signatureType.
It is renamed from signatureType to at in order to support JEP-370.

SignatureType enum values are renamed to work with AddressVarHandleGenerator
and VarForm:

  1. getter -> GET
  2. setter -> SET
  3. compareAndSet -> COMPARE_AND_SET
  4. compareAndExchange -> COMPARE_AND_EXCHANGE
  5. getAndSet -> GET_AND_UPDATE
  6. invalid removed since it is unused.

Also, enum SignatureType is renamed to AccessType in order to work with VarForm
and AddressVarHandleGenerator.

8. Add AccessMode.methodNameToAccessMode to support VarForm

AccessMode.methodNameToAccessMode is of type Map<String, AccessMode>.

It is introduced to support VarForm.linkFromStatic. It needs to be initialized
properly. Currently, it is initialized to null.

9. Add VarHandle.AIOOBE_SUPPLIER to support X-VarHandle.java.template

X-VarHandle.java.template exists in the java.lang.invoke package directory.

VarHandle.AIOOBE_SUPPLIER is needed to build the Java classes generated by the
above template.

OpenJ9 does not utilize the Java classes generated from the above template
since it has its own implementation.

So, VarHandle.AIOOBE_SUPPLIER is initialized to null.

Signed-off-by: Babneet Singh sbabneet@ca.ibm.com

@babsingh babsingh changed the title Support JEP-370 Support JEP-370 (Part 1) Jan 17, 2020
@DanHeidinga
Copy link
Member

6. Remove ASSERT from Unsafe.defineAnonymousClass

OpenJDK's AddressVarHandleGenerator.defineClass invokes
Unsafe.defineAnonymousClass with non-null constPatches.

The assertion is removed because it prevents non-null constPatches.

AddressVarHandleGenerator.defineClass works after the assertion is removed.

constPatches is unused in Unsafe.defineAnonymousClass. So, there should be no
restrictions on it.

Does it pass an empty array for the constPatches or actual values that need to be patched? @fengxue-IS has been looking at supporting cp patching but we don't have an implementation yet. Can you confirm whether the constPatches array is empty or not?

@DanHeidinga
Copy link
Member

Are these changes sufficient to run the new varhandles? Is this PR providing the framework / outline on which additional changes will be required?

@babsingh
Copy link
Contributor Author

babsingh commented Jan 17, 2020

Are these changes sufficient to run the new varhandles? Is this PR providing the framework / outline on which additional changes will be required?

No, these changes should build the new varhandles. We should also be able to run the new varhandles and corresponding tests but the stubs would throw OpenJDKCompileStubThrowError. The stubs need to be implemented in order for these changes to be fully functional.

These changes are dependent on ibmruntimes/openj9-openjdk-jdk14#6.

For the first commit Remove empty stubs and use OpenJDK classes, instead of completely removing the empty stubs, I am thinking of disabling the stubs from Java 14 and onwards. Completely removing the stubs would require the above openj9-openjdk-jdk14 changes to be backported, which should be avoided since JEP-370 is only introduced in Java 14 and backporting them will only introduce unusable code in Java13-.

@babsingh babsingh force-pushed the jep370 branch 3 times, most recently from 3657f3f to c89c2ff Compare January 17, 2020 19:12
@DanHeidinga
Copy link
Member

Adding notes based on our conversation Friday:

  • This is the direction I want us to go in as it moves towards consuming the OpenJDK MH/VH implementation.
  • Given the timeline before we split for JDK 14, there may not be time to complete this but we should give it a go for a week or so to see if we can get it working. Otherwise, we'll need to save enough time to implement your first suggested design.
  • This depends on @fengxue-IS's work on supporting constantpool patching being ready as a basis for this work

@babsingh
Copy link
Contributor Author

babsingh commented Jan 30, 2020

These changes are ready to be reviewed. The space vs tab issue was resolved. Remove ASSERT from Unsafe.defineAnonymousClass commit was removed since it will be accounted by #8460 (@fengxue-IS CP String Patching changes).

@babsingh
Copy link
Contributor Author

babsingh commented Feb 4, 2020

Depends on ibmruntimes/openj9-openjdk-jdk14#6.

VarHandles.makeMemoryAddressViewHandle is needed to support JEP-370.

To consume OpenJDK's VarHandles without any amends, we need to use the
following OpenJDK classes:
1) AddressVarHandleGenerator
2) VarHandleByteArrayBase
3) VarHandleMemoryAddressBase

JEP-370 is a Java 14 feature. So, the empty OpenJ9 stubs for the above
classes have been removed or disabled in Java 14.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
OpenJDK's VarForm is needed to support JEP-370. So, the empty OpenJ9
stub for VarForm is disabled in Java 14.

A new VarHandle constructor is also needed to support JEP-370.

VarHandle(VarForm varForm) {
	/* TODO: Translate {VarForm varForm} -> {Class<?> fieldType,
	 * Class<?>[] coordinateTypes, MethodHandle[] handleTable,
	 * int modifiers}
	 */
}

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
VarHandles.makeFieldHandle relies upon following methods:
- MethodHandleNatives.objectFieldOffset
- MethodHandleNatives.staticFieldBase
- MethodHandleNatives.staticFieldOffset

Stubs for the above three methods are added in OpenJ9's
MethodHandleNatives.

OpenJ9's FieldVarHandle doesn't depend on VarHandles.makeFieldHandle.
OpenJ9 has its own implementation to support FieldVarHandle. So, the
above methods do not need to be implemented.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Java classes generated from X-VarHandle*.template implement the
VarHandle.accessModeTypeUncached abstract method.

This method has been introduced in OpenJ9's VarHandle to support
JEP-370.

OpenJ9's implementation of ArrayVarHandle, FieldVarHandle and
ViewVarHandle do not utilize VarHandle.accessModeTypeUncached. So, they
implement this method as an empty stub.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
MemberName(Method method) constructor is needed to support OpenJ9.

boolean isStatic() method is added to build VarHandles.makeFieldHandle.
It does not need an implementation since OpenJ9 implements its own
FieldVarHandle and does not rely upon VarHandles.makeFieldHandle.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
In OpenJDK's AddressVarHandleGenerator, returnInsn and loadInsn methods
rely upon LambdaForm.BasicType.

The following values are added in LambdaForm.BasicType enum: I_TYPE,
J_TYPE, F_TYPE, D_TYPE and V_TYPE, in order to support the above
functions.

BasicType.basicType(Class<?> cls) will need to be implemented in order
to support JEP-370.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
AddressVarHandleGenerator.addAccessModeTypeMethod accesses a field named
"at" in VarHandle.AccessMode enum. This field is currently named as
"signatureType". It is renamed from "signatureType" to "at" in order to
support JEP-370.

SignatureType enum values are renamed to work with
AddressVarHandleGenerator and VarForm:
1) getter -> GET
2) setter -> SET
3) compareAndSet -> COMPARE_AND_SET
4) compareAndExchange -> COMPARE_AND_EXCHANGE
5) getAndSet -> GET_AND_UPDATE
6) invalid removed since it is unused.

Also, enum SignatureType is renamed to AccessType in order to work with
VarForm and AddressVarHandleGenerator.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
AccessMode.methodNameToAccessMode is of type Map<String, AccessMode>.

It is introduced to support VarForm.linkFromStatic. It needs to be
initialized properly. Currently, it is initialized to null.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
X-VarHandle.java.template exists in the java.lang.invoke package directory.

VarHandle.AIOOBE_SUPPLIER is needed to build the Java classes generated by the
above template.

OpenJ9 does not utilize the Java classes generated from the above template
since it has its own implementation.

So, VarHandle.AIOOBE_SUPPLIER is initialized to null.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk14 depends ibmruntimes/openj9-openjdk-jdk14#6

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk11

@babsingh
Copy link
Contributor Author

babsingh commented Feb 5, 2020

Build_JDK14_s390x_linux_Personal failure:

10:32:18  /home/jenkins/workspace/Build_JDK14_s390x_linux_Personal/src/java.base/share/classes/java/lang/invoke/VarHandles.java:54: error: package VarHandleReferences does not exist
10:32:18                         : new VarHandleReferences.FieldInstanceReadWrite(refc, foffset, type);
10:32:18                                                  ^

ibmruntimes/openj9-openjdk-jdk14#6 should generate VarHandleReferences.java and other missing Java dependencies. The JDK built successfully in s390_linux personal/local builds.

@babsingh
Copy link
Contributor Author

babsingh commented Feb 5, 2020

Build_JDK14_s390x_linux_Personal failure:

10:32:18  /home/jenkins/workspace/Build_JDK14_s390x_linux_Personal/src/java.base/share/classes/java/lang/invoke/VarHandles.java:54: error: package VarHandleReferences does not exist
10:32:18                         : new VarHandleReferences.FieldInstanceReadWrite(refc, foffset, type);
10:32:18                                                  ^

ibmruntimes/openj9-openjdk-jdk14#6 should generate VarHandleReferences.java and other missing Java dependencies. The JDK built successfully in s390_linux personal/local builds.

@AdamBrousseau Unable to reproduce this failure locally. depends ibmruntimes/openj9-openjdk-jdk14#6 seems to work in the log output. Not sure why the jenkins build job is failing.

@AdamBrousseau
Copy link
Contributor

LFTM

00:26:59.866  Look for Dependent Changes
00:26:59.871  PullRequest source repo appears to be OpenJ9
00:26:59.882  Dependent OpenJDK change found: #6
00:26:59.885  SDK: 14
00:26:59.887  REPO: openj9-openjdk-jdk14
00:27:00.518  + git fetch --progress origin +refs/pull/6/merge:refs/remotes/origin/pr/6/merge
...
00:27:00.660  From https://github.com/ibmruntimes/openj9-openjdk-jdk14
00:27:00.660   * [new ref]                 refs/pull/6/merge -> origin/pr/6/merge
00:27:01.282  + git checkout refs/remotes/origin/pr/6/merge
00:27:01.653  HEAD is now at de9e1892cc1... Merge 237d18de5aaabae38496ae177e7bb8db71492aa8 into e2df48e721fddd033b3eee1a68182fa007980de8
...
00:27:02.350  [2020-02-05 15:18:43] Get OpenJ9 sources
...
00:30:15.669  + git fetch --progress origin +refs/pull/8330/merge:refs/remotes/origin/pr/8330/merge
...
00:30:16.039  From https://github.com/eclipse/openj9
00:30:16.039   * [new ref]             refs/pull/8330/merge -> origin/pr/8330/merge
[Pipeline] sh
00:30:16.732  + git checkout refs/remotes/origin/pr/8330/merge
00:30:16.812  Note: checking out 'refs/remotes/origin/pr/8330/merge'.
...
00:30:16.812  HEAD is now at 5050be1ed... Merge c547036ce3671a60a9e15ea11bdd013c15bbaa1a into 81782de72a4ac69feae20371af7bc15eaff3b587

Can you reproduce in an internal personal build?
Do you suspect a machine issue?
Do you need machine access?

@babsingh
Copy link
Contributor Author

babsingh commented Feb 5, 2020

Can you reproduce in an internal personal build?

no, it passes in an internal personal build.

Do you suspect a machine issue?

can't tell from the log.

Do you need machine access?

yes if the issue is recurring.

A complete identical job in ibmruntimes/openj9-openjdk-jdk14#6 passed. I am guessing that a restart should resolve the issue.

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk14 depends ibmruntimes/openj9-openjdk-jdk14#6

@DanHeidinga
Copy link
Member

Restarted the build based on @AdamBrousseau's comment that a restart should resolve the failure.

@babsingh what order do these need to be delivered in? Or does delivering one (oj9 or extensions) break the build till the other is available?

@DanHeidinga DanHeidinga self-assigned this Feb 6, 2020
@pshipton
Copy link
Member

pshipton commented Feb 6, 2020

See the comments in ibmruntimes/openj9-openjdk-jdk14#6 (comment)

Jenkins test sanity zlinux jdk14 depends ibmruntimes/openj9-openjdk-jdk14#6

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants