Implement MethodHandleNatives native code #10690

fengxue-IS · 2020-09-24T17:25:53Z

Related: #7352

Co-authored-by: Babneet Singh sbabneet@ca.ibm.com
Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

fengxue-IS · 2020-09-24T18:41:37Z

I've noticed that Java_java_lang_invoke_MethodHandleNatives_checkClassBytes have been added as part of Java 15, will have to look into if that should be merged into MethodHandleNatives.cpp

DanHeidinga

I reviewed about 1/2 this before stopping as I need more context on what the operations are expected to do.

A good next step would be to document each of the functions so it's clear what the expected behaviour is

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

fengxue-IS · 2020-11-12T19:54:15Z

Thanks for the review @DanHeidinga, I will add documentation comments for the function to provide details on the expected behavior

fengxue-IS

I have updated the code to include a function header that describe the expected behavior of each function and added comments to large code block to make it more readable. Also fixed some formatting issues.

While I was updating the comments, I realized there are more potential places where GC could occur and invalidate the native object reference. I will push the fix for these (either refetch the data or push objects to special frame) once local testing passed

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

DanHeidinga

Taking a break for lunch. Will pick up the review from sigForPrimitiveOrVoid later

runtime/jcl/uma/java_lang_invoke_MethodHandleNatives_exports.xml

runtime/oti/jclprots.h

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

DanHeidinga

@fengxue-IS there's a lot of feedback here already so I'll stop reviewing at this point.

Please look at all the allocations and check for allocations failures. Figuring out how to report that back to the callers will help clarify your apis.

It's also good to minimize the lifetimes of every fetched object so they are less likely to be lost by a GC. It'll also make the code easier to reason about

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

fengxue-IS · 2020-11-27T21:58:10Z

Thanks @DanHeidinga for the indepth review, I will look into the issues pointed out and also go through all the allocation codes to make sure to check for any potential failures.

fengxue-IS · 2020-12-10T22:15:35Z

Added checks for allocation failure and handle exception from helper functions
Added support for CallerSensitive annotation
Moved createFieldObject to reflecthelp.c and added to the J9ReflectionFunctions table
Modified variable declaration to minimize refetch required after potential GC points
Use existing objects from reflect object rather than generating new objects
Fixed formating
Created TODO for JSR 292 MethodHandleNatives APIs #11443 to track todo items to be completed in future PR

@DanHeidinga can you take another look please

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

- use reflect object's name/type/signature field instead of constructing new object - fill in name/signature for method/constructor rather than lazily computing during resolve - idFromReflectObject can't fail as long as the object is not null Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

- expand reflectFunctions table - reflectFunctions->createFieldObject Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

- replace copyStringToUTF8Helper with copyStringToUTF8WithMemAlloc - add MN_CALLER_SENSITIVE check - add OOM checks and exit with goto - throw and handle nativeOOMError - fix all missing Error checking and refetch after any GC point - fix tracepoint modifiers - fix use of bool in c code - fix declaration skipped by goto Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

- use Class.classNameString if possible - check if arrayClass to set proper classNameString format Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

fengxue-IS · 2021-01-12T17:38:18Z

Fix all issues in review comments, local build passed
@DanHeidinga can you please take a look?

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

- create common helper for setCallSiteTargetNormal & setCallSiteTargetVolatile - add static keyword for helper functions - fix array store index Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

fengxue-IS · 2021-01-27T21:20:13Z

Addressed review comments, local compile/basic testing passed.
@DanHeidinga do you see any other issues I should fix?

DanHeidinga · 2021-01-29T19:13:20Z

runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp

+		UDATA offset = (UDATA)vmFuncs->instanceFieldOffset(currentThread, J9VM_J9CLASS_FROM_HEAPCLASS(currentThread, callsiteObject), (U_8*)"target", strlen("target"), (U_8*)"Ljava/lang/invoke/MethodHandle;", strlen("Ljava/lang/invoke/MethodHandle;"), NULL, NULL, 0);
+		MM_ObjectAccessBarrierAPI objectAccessBarrier = MM_ObjectAccessBarrierAPI(currentThread);


This seems like a good candidate to put these in the VMConstantpool

DanHeidinga · 2021-01-29T19:30:48Z

I don't see any holes where objects will be lost now so I'm OK with proceeding with this change.

Having said that, I'm not 100% onboard with this approach. The heavy string allocation and copying should be looked at again to see if there's a better way to solve this. Given the other up-calls to the VM, I'd be tempted to implement this in Java and cache the signature per MT to make this cleaner.

All that can wait for a later PR though.

DanHeidinga · 2021-01-29T19:31:27Z

Jenkins test sanity xlinux jdk16

DanHeidinga · 2021-01-29T19:31:45Z

Jenkins test sanity xlinux jdk8,jdk11

fengxue-IS · 2021-02-03T19:57:56Z

Having said that, I'm not 100% onboard with this approach. The heavy string allocation and copying should be looked at again to see if there's a better way to solve this. Given the other up-calls to the VM, I'd be tempted to implement this in Java and cache the signature per MT to make this cleaner.

Thanks for the suggestions Dan, I have added this to the TODO list (#11443) to be investigated later

fengxue-IS force-pushed the jsr292_mh_native branch from 5b0e0d3 to ac41c0b Compare September 24, 2020 18:40

fengxue-IS force-pushed the jsr292_mh_native branch from ac41c0b to 152ecf0 Compare October 1, 2020 18:22

DanHeidinga added the comp:vm label Nov 12, 2020

DanHeidinga reviewed Nov 12, 2020

View reviewed changes

fengxue-IS force-pushed the jsr292_mh_native branch from e6878d7 to e6640a0 Compare November 12, 2020 19:51

babsingh mentioned this pull request Nov 26, 2020

Use aliases for MethodType fields in vmconstantpool.xml #11287

Closed

fengxue-IS commented Nov 27, 2020

View reviewed changes

fengxue-IS force-pushed the jsr292_mh_native branch from 7d72fc2 to 5e4fda0 Compare November 27, 2020 06:02

DanHeidinga reviewed Nov 27, 2020

View reviewed changes

DanHeidinga self-assigned this Nov 27, 2020

DanHeidinga added the project:MH Used to track Method Handles related work label Nov 27, 2020

babsingh mentioned this pull request Dec 9, 2020

[WIP] Use OJDK code to resolve OJDK MH CP Entry #11371

Closed

fengxue-IS force-pushed the jsr292_mh_native branch 2 times, most recently from 687afaa to 51f67c5 Compare December 10, 2020 21:55

fengxue-IS mentioned this pull request Dec 10, 2020

TODO for JSR 292 MethodHandleNatives APIs #11443

Open

4 tasks

fengxue-IS marked this pull request as ready for review December 10, 2020 22:17

babsingh mentioned this pull request Dec 11, 2020

Test Results for OpenJ9 JDK15/JDK16 with OpenJDK MethodHandles #11458

Closed

DanHeidinga reviewed Dec 17, 2020

View reviewed changes

This was referenced Dec 17, 2020

Update linkToInterface for OpenJDK MethodHandles #11367

Merged

Account HCR for OpenJDK MethodHandles #11528

Closed

fengxue-IS and others added 4 commits January 12, 2021 12:29

Implement MethodHandleNatives native code

24f324b

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

Add documentation for MethodHandleNatives native functions

4f80dd3

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

Fix indent in MethodHandleNatives_exports.xml

e1ac202

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

fengxue-IS and others added 9 commits January 12, 2021 12:29

move createFieldObject to reflecthelp.c

85aac41

- expand reflectFunctions table - reflectFunctions->createFieldObject Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

ifdef JCL function headers

662cc6e

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

Update comments for MN.expand and MN.resolve

6f40496

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

Use rom constants for (int/float/long/double)

512cf55

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

remove NULL check before j9mem_free_memory

eb86b17

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

combine getClassSignature and sigForPrimitiveOrVoid

57bbf4f

- use Class.classNameString if possible - check if arrayClass to set proper classNameString format Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

Store memberName to sp to avoid GC issue

6f74d5f

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

Update Copyright date

a7bf780

Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

fengxue-IS force-pushed the jsr292_mh_native branch from 15eaf9e to a7bf780 Compare January 12, 2021 17:36

DanHeidinga reviewed Jan 16, 2021

View reviewed changes

fengxue-IS mentioned this pull request Jan 20, 2021

Add Helper to check if a method is LambdaForm generated #11715

Merged

Address review comments

4f43221

- create common helper for setCallSiteTargetNormal & setCallSiteTargetVolatile - add static keyword for helper functions - fix array store index Co-authored-by: Babneet Singh <sbabneet@ca.ibm.com> Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

DanHeidinga reviewed Jan 29, 2021

View reviewed changes

DanHeidinga merged commit 5e0957a into eclipse-openj9:master Jan 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement MethodHandleNatives native code #10690

Implement MethodHandleNatives native code #10690

fengxue-IS commented Sep 24, 2020

fengxue-IS commented Sep 24, 2020

DanHeidinga left a comment

fengxue-IS commented Nov 12, 2020

fengxue-IS left a comment

DanHeidinga left a comment

DanHeidinga left a comment

fengxue-IS commented Nov 27, 2020

fengxue-IS commented Dec 10, 2020

fengxue-IS commented Jan 12, 2021

fengxue-IS commented Jan 27, 2021

DanHeidinga Jan 29, 2021

DanHeidinga commented Jan 29, 2021

DanHeidinga commented Jan 29, 2021

DanHeidinga commented Jan 29, 2021

fengxue-IS commented Feb 3, 2021

		UDATA offset = (UDATA)vmFuncs->instanceFieldOffset(currentThread, J9VM_J9CLASS_FROM_HEAPCLASS(currentThread, callsiteObject), (U_8)"target", strlen("target"), (U_8)"Ljava/lang/invoke/MethodHandle;", strlen("Ljava/lang/invoke/MethodHandle;"), NULL, NULL, 0);
		MM_ObjectAccessBarrierAPI objectAccessBarrier = MM_ObjectAccessBarrierAPI(currentThread);

Implement MethodHandleNatives native code #10690

Implement MethodHandleNatives native code #10690

Conversation

fengxue-IS commented Sep 24, 2020

fengxue-IS commented Sep 24, 2020

DanHeidinga left a comment

Choose a reason for hiding this comment

fengxue-IS commented Nov 12, 2020

fengxue-IS left a comment

Choose a reason for hiding this comment

DanHeidinga left a comment

Choose a reason for hiding this comment

DanHeidinga left a comment

Choose a reason for hiding this comment

fengxue-IS commented Nov 27, 2020

fengxue-IS commented Dec 10, 2020

fengxue-IS commented Jan 12, 2021

fengxue-IS commented Jan 27, 2021

DanHeidinga Jan 29, 2021

Choose a reason for hiding this comment

DanHeidinga commented Jan 29, 2021

DanHeidinga commented Jan 29, 2021

DanHeidinga commented Jan 29, 2021

fengxue-IS commented Feb 3, 2021