Refactor HeapArgInfo to Support Recursive Downcalls #22370

AditiS11 · 2025-08-06T17:00:43Z

Fix for Issue #21944
Fix for failing java/foreign/TestBufferStack.java test.

Replaced single HeapArgInfo with a thread-local Deque.
To track multiple argument sets for recursions.
For every downcall a new entry is pushed onto the stack.
The entry is removed from the stack after downcall completion.

tajila · 2025-08-06T17:33:55Z

@AditiS11 Please read through https://github.com/eclipse-omr/omr/blob/master/doc/CodingStandard.md

tajila · 2025-08-06T17:34:45Z

Also please refer to https://github.com/eclipse-openj9/openj9/blob/master/CONTRIBUTING.md for opening PRs

tajila · 2025-08-12T15:23:50Z

Please rename the title of the PR to something that describes the changes.

Also, please explain in the commit message what you are trying to achieve.

AditiS11 · 2025-08-18T15:45:06Z

sanity.functional and sanity.openjdk tests: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/29426/

tajila · 2025-08-18T16:32:14Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -117,13 +119,17 @@ void append(Object base, long offset) {
 			index += 1;
 		}

+		boolean isFull() {
+			return index >= bases.length;


this should be impossible if a new HeapArgInfo is created for each call

tajila · 2025-08-18T16:39:51Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

 		void clear() {
 			Arrays.fill(bases, null);
 			index = 0;
 		}
 	}

-	private final ThreadLocal<HeapArgInfo> heapArgInfo;
+	private final ThreadLocal<Deque<HeapArgInfo>> heapArgInfo;


this should be static

And the thread local should be initialized here

tajila · 2025-08-18T16:42:08Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -507,7 +518,7 @@ public InternalDowncallHandler(MethodType functionMethodType, FunctionDescriptor
 		/*[ENDIF] JAVA_SPEC_VERSION == 17 */

 		/*[IF JAVA_SPEC_VERSION >= 22]*/
-		heapArgInfo = new ThreadLocal<>();
+		heapArgInfo = ThreadLocal.withInitial(ArrayDeque::new);


this should be removed

theresa-m

Aditi, thanks for making this change.

The test in #21944 is labelled with "test excluded" which means it will not be run in the sanity.openjdk test suite. Can you also run the test on its own in a grinder to show its passing? Let me know if you have any questions on how to do this.

Once this change is in you will need to submit another pull request at https://github.com/adoptium/aqa-tests to re-enable TestBufferStack by removing this line before closing #21944.

Title nitpick: Instead of including implementation details here please include why HeapArgInfo is being refactored. I suggest something along the lines of "Refactor HeapArgInfo to Support Recursive Downcalls".

AditiS11 · 2025-08-23T11:42:54Z

sanity.functional and sanity.openjdk tests: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/29534/

java/foreign/TestBufferStack: https://hyc-runtimes-jenkins.swg-devops.com/view/Test_grinder/job/Grinder/54089/

theresa-m · 2025-08-25T14:08:13Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -27,6 +27,8 @@
 /*[ENDIF] JAVA_SPEC_VERSION >= 22 */
 import java.util.HashMap;
 import java.util.List;
+import java.util.Deque;


Please move these imports inside JAVA_SPEC_VERSION >= 22 and order them alphabetically.

theresa-m · 2025-08-25T14:08:56Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -123,7 +125,8 @@ void clear() {
 		}
 	}

-	private final ThreadLocal<HeapArgInfo> heapArgInfo;
+	private static final ThreadLocal<Deque<HeapArgInfo>> heapArgInfo = ThreadLocal.withInitial(ArrayDeque::new);
+	private static final ThreadLocal<Boolean> flagCreateHeapArgInfo = ThreadLocal.withInitial(()->Boolean.TRUE);


Please add a comment either here or in runNativeMethod describing the updated behavior of these two fields.

nit: flag in flagCreateHeapArgInfo seems redundant, I would suggest shortening this to createHeapArgInfo or newHeapArgInfo.

theresa-m · 2025-08-25T14:14:02Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -323,18 +327,21 @@ private final long memSegmtOfPtrToLongArg(MemorySegment argValue, LinkerOptions
 			 * is captured so as to reset the internal index and avoid retaining the references
 			 * to the unreachable objects.
 			 */
-			if (info != null) {
-				info.clear();
+			if(!flagCreateHeapArgInfo.get() && !infoStack.isEmpty()) {


The second check !infoStack.isEmpty() is not needed. If flagCreateHeapArgInfo is false there should be a stack entry. Also add a space before the first parenthesis.

theresa-m · 2025-08-25T14:15:09Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

 			}
 			/*[ENDIF] JAVA_SPEC_VERSION >= 22 */
 			throw e;
 		}

 		long address = argValue.address();
 		/*[IF JAVA_SPEC_VERSION >= 22]*/
-		if (info == null) {
+
+		if ((info == null) || flagCreateHeapArgInfo.get()) {


The (info == null) check is not necessary since flagCreateHeapArgInfo is true by default.

theresa-m · 2025-08-25T14:17:33Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

-			if (info != null) {
-				info.clear();
+			Deque<HeapArgInfo> infoStack = heapArgInfo.get();
+			if(!flagCreateHeapArgInfo.get() && !infoStack.isEmpty()) {


Same applies here, the second check can be removed and please add a space after if.

theresa-m · 2025-08-25T14:18:47Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

+		if (flagCreateHeapArgInfo.get()) {
+			info = null;
+			pointerArgs = false;
+		}


Put else on the same line.

theresa-m · 2025-08-25T14:19:20Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -936,8 +951,8 @@ Object runNativeMethod(Addressable downcallAddr, SegmentAllocator segmtAllocator
 			 * so as to reset the internal index and avoid retaining the references to the
 			 * unreachable objects.
 			 */
-			if (info != null) {
-				info.clear();
+			if (!infoStack.isEmpty() && pointerArgs) {


Don't need to check infoStack.isEmpty.

theresa-m · 2025-08-25T15:52:18Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -123,7 +125,15 @@ void clear() {
 		}
 	}

-	private final ThreadLocal<HeapArgInfo> heapArgInfo;
+	/* heapArgInfo holds a stack of HeapArgInfo structures.
+	 * createHeapArgInfo is a flag indicating whether a new entry should be created.


Please also describe when a new entry should be created.

theresa-m · 2025-08-25T15:52:25Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -123,7 +125,15 @@ void clear() {
 		}
 	}

-	private final ThreadLocal<HeapArgInfo> heapArgInfo;
+	/* heapArgInfo holds a stack of HeapArgInfo structures.


Please also describe what the stack is for.

theresa-m · 2025-08-25T17:27:31Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -25,6 +25,8 @@
 /*[IF JAVA_SPEC_VERSION >= 22]*/
 import java.util.Arrays;
 /*[ENDIF] JAVA_SPEC_VERSION >= 22 */
+import java.util.ArrayDeque;


I thought I saw this fixed before but these imports should be moved within JAVA_SPEC_VERSION >= 22.

theresa-m · 2025-08-25T17:29:39Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

+	/* heapArgInfo holds a per-thread stack of HeapArgInfo structures.
+	 * Each entry tracks Java references to heap-allocated memory segments
+	 *
+	 * createHeapArgInfo controls when a new entry should be created.


I'm looking for this comment to explain more of the why and less of the what. As per the coding standards:

Comments are used to explain the intent of the code, not how it works. Well-written code shouldn't require comments to explain how it works. Comments should explain why it is necessary.

So for heapArgInfo you can just explain that its needed for recursive downcalls and for createHeapArgInfo that it is needed since there may be multiple pointer arguments for a downcall.

theresa-m

Looks good to me. Can you please review @tajila ?

theresa-m · 2025-08-26T14:51:20Z

@AditiS11 I just noticed that the description is no longer correct, can you please update?

Each downcall pushes a new HeapArgInfo into Deque.
Clears the most recent entry after downcall completion.

tajila · 2025-08-26T15:22:06Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

+		HeapArgInfo info;
+		/* Check if current downcall has pointer arguments. */
+		boolean pointerArgs = true;
+		if (createHeapArgInfo.get()) {


According to

* createHeapArgInfo controls when a new entry should be created. * If true, a new HeapArgInfo is pushed for the current downcall. * A single downcall may have multiple pointer arguments,

This would indicate that if if (createHeapArgInfo.get()) { then pointerArgs = true; ?

pointerArgs is used to determine whether a HeapArgInfo entry exists for this downcall so it can be removed at the end of runNativeMethod after the downcall has completed.

By defualt createHeapArgInfo is set to true. If the downcall has pointer arguments then memSegmtOfPtrToLongArg is called before runNative where the flag will be set to false and a HeapArgInfo is pushed to the stack.

If createHeapArgInfo is still true in runNative then it means the downcall has no pointer args hence pointerArgs is set to false.

Thanks for clarifying

tajila · 2025-08-26T15:53:25Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

+	 * A single downcall may have multiple pointer arguments,
+	 * and all of them share the same entry.
+	 */
+	private static final ThreadLocal<Deque<HeapArgInfo>> heapArgInfo = ThreadLocal.withInitial(ArrayDeque::new);


I think this can be simplified:

Always create a HeapArgInfo in the entry to the MethodHandle (you'll need to find the entry point since the core handle is wrapped) and push it on to the local stack. And always pop it after the call irrespective of whether pointers are used or not. This removes the need for createHeapArgInfo.

In the constructor for HeapArgInfo do not allocate the bases and offsets upfront, instead check and allocate when append() is called.

All the if (info == null) { checks can be removed. However, the if (info != null) { still need to be kept.

(you'll need to find the entry point since the core handle is wrapped)

For this part, might be easiest to warp the bounded handle in another handle that just create the heapArgInfo and passes the args along. This can be done with MethodHandles.foldArguments(boundHandle, createHeapArginfoHandle). You'll need to bind this (lookup.bind(this,) to it so you can access the threadlocals etc.

The createHeapArginfoHandle should match the args of the boundHandle so that the initial arguments are just passed through.

tajila · 2025-08-28T12:08:42Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

-			return isFfiProtoOn ? getNativeMHWithInvokeCache(boundMH) : boundMH;
-			/*[ELSE] JAVA_SPEC_VERSION >= 22 */
-			return boundMH;
+			MethodHandle pushHeapArgInfoMH = MethodHandles.lookup().findStatic(InternalDowncallHandler.class, "pushHeapArgInfo", MethodType.methodType(void.class, InternalDowncallHandler.class)).bindTo(this);


can you create this in the same place as the others around line 530?

EDIT: the handle that is, you can still do foldArgs here.

tajila · 2025-08-28T12:10:04Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -914,8 +935,8 @@ Object runNativeMethod(Addressable downcallAddr, SegmentAllocator segmtAllocator
 					returnStateMemBase,
 					/*[ENDIF] JAVA_SPEC_VERSION >= 24 */
 					/*[IF JAVA_SPEC_VERSION >= 22]*/
-					(info != null) ? info.bases : null,
-					(info != null) ? info.offsets : null,
+					(info != null) ? info.getBases() : null,


Is. it possible that info == null here? if not you can just pass bases and offsets without the null check

It can't be null. Removed the null check.

theresa-m · 2025-08-28T12:36:09Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java


 		HeapArgInfo(int size) {
-			bases = new Object[size];
-			offsets = new long[size];
+			bases = null;


Please be consistent with use of this. The index field can also be private.

theresa-m · 2025-08-28T12:40:05Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -324,18 +346,14 @@ private final long memSegmtOfPtrToLongArg(MemorySegment argValue, LinkerOptions
 			 * to the unreachable objects.
 			 */
 			if (info != null) {


There is no need for this line since each downcall will have a HeapArgInfo entry. Same for the other exception cases.

tajila · 2025-08-29T13:43:02Z

@theresa-m Please take another look

theresa-m · 2025-08-29T14:03:24Z

jcl/src/java.base/share/classes/openj9/internal/foreign/abi/InternalDowncallHandler.java

@@ -27,6 +27,8 @@
 /*[ENDIF] JAVA_SPEC_VERSION >= 22 */
 import java.util.HashMap;
 import java.util.List;
+import java.util.Deque;


The imports moved out of JAVA_SPEC_VERSION >= 22. Otherwise looks good.

Fix for Issue eclipse-openj9#21944 Fix for failing java/foreign/TestBufferStack.java test. Replaced single HeapArgInfo with a thread-local Deque. To track multiple argument sets for recursions. For every downcall a new entry is pushed onto the stack. The entry is removed from the stack after downcall completion.

tajila · 2025-09-02T16:21:30Z

jenkins test sanity,extended.functional plinux jdk25

AditiS11 marked this pull request as draft August 8, 2025 14:54

AditiS11 force-pushed the Issue-21944 branch 2 times, most recently from 7d00d90 to ee8865d Compare August 9, 2025 15:40

AditiS11 force-pushed the Issue-21944 branch 2 times, most recently from 6e71ba7 to bf1c4b8 Compare August 16, 2025 19:15

AditiS11 changed the title ~~Fix for Issue #21944~~ Refactor HeapArgInfo to Support Queue-Based Tracking for Java 22+ Aug 18, 2025

AditiS11 changed the title ~~Refactor HeapArgInfo to Support Queue-Based Tracking for Java 22+~~ Refactor HeapArgInfo to Support Queue-Based Tracking Aug 18, 2025

AditiS11 force-pushed the Issue-21944 branch 2 times, most recently from 333aee1 to 43cf086 Compare August 18, 2025 15:30

AditiS11 marked this pull request as ready for review August 18, 2025 15:45

theresa-m self-requested a review August 18, 2025 15:49

tajila reviewed Aug 18, 2025

View reviewed changes

theresa-m reviewed Aug 18, 2025

View reviewed changes

r30shah mentioned this pull request Aug 20, 2025

JDK25 jdk_foreign_0_FAILED java/foreign/TestBufferStack.java ArrayIndexOutOfBoundsException: Array index out of range, followed by Segmentation error vmState=0x00000000 #21944

Open

zl-wang mentioned this pull request Aug 20, 2025

jdk25 openjdk java/foreign/TestBufferStackStress timeout #22224

Open

AditiS11 force-pushed the Issue-21944 branch from ed1cff3 to 4bad089 Compare August 23, 2025 01:08

AditiS11 marked this pull request as draft August 23, 2025 01:09

AditiS11 force-pushed the Issue-21944 branch 3 times, most recently from f11b808 to 728686d Compare August 23, 2025 11:38

AditiS11 changed the title ~~Refactor HeapArgInfo to Support Queue-Based Tracking~~ Refactor HeapArgInfo to Support Recursive Downcalls Aug 23, 2025

AditiS11 marked this pull request as ready for review August 23, 2025 11:40

theresa-m suggested changes Aug 25, 2025

View reviewed changes

AditiS11 force-pushed the Issue-21944 branch 2 times, most recently from a2cebac to 0b42010 Compare August 25, 2025 15:42

theresa-m suggested changes Aug 25, 2025

View reviewed changes

AditiS11 force-pushed the Issue-21944 branch from 0b42010 to 93f92ec Compare August 25, 2025 16:44

theresa-m suggested changes Aug 25, 2025

View reviewed changes

AditiS11 force-pushed the Issue-21944 branch 2 times, most recently from 08ba733 to 22869c2 Compare August 26, 2025 12:47

theresa-m approved these changes Aug 26, 2025

View reviewed changes

tajila reviewed Aug 26, 2025

View reviewed changes

AditiS11 force-pushed the Issue-21944 branch from 22869c2 to 27fae34 Compare August 26, 2025 15:49

tajila reviewed Aug 26, 2025

View reviewed changes

AditiS11 force-pushed the Issue-21944 branch from 27fae34 to 8bf3224 Compare August 28, 2025 11:43

tajila reviewed Aug 28, 2025

View reviewed changes

theresa-m suggested changes Aug 28, 2025

View reviewed changes

AditiS11 force-pushed the Issue-21944 branch 2 times, most recently from d067dfa to 7970d41 Compare August 29, 2025 05:21

tajila approved these changes Aug 29, 2025

View reviewed changes

theresa-m reviewed Sep 2, 2025

View reviewed changes

AditiS11 force-pushed the Issue-21944 branch from 7970d41 to 62cad9e Compare September 2, 2025 15:04

theresa-m approved these changes Sep 2, 2025

View reviewed changes

Refactor HeapArgInfo to Support Recursive Downcalls #22370

Are you sure you want to change the base?

Refactor HeapArgInfo to Support Recursive Downcalls #22370

Uh oh!

Conversation

AditiS11 commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tajila commented Aug 6, 2025

Uh oh!

tajila commented Aug 6, 2025

Uh oh!

tajila commented Aug 12, 2025

Uh oh!

AditiS11 commented Aug 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theresa-m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AditiS11 commented Aug 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theresa-m Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theresa-m left a comment

Choose a reason for hiding this comment

Uh oh!

theresa-m commented Aug 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tajila Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tajila Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tajila Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

AditiS11 commented Aug 6, 2025 •

edited

Loading

theresa-m left a comment •

edited

Loading

theresa-m Aug 25, 2025 •

edited

Loading

tajila Aug 26, 2025 •

edited

Loading

tajila Aug 27, 2025 •

edited

Loading

tajila Aug 28, 2025 •

edited

Loading

theresa-m Aug 28, 2025 •

edited

Loading