DDR Changes for Off-Heap by LinHu2016 · Pull Request #20111 · eclipse-openj9/openj9

LinHu2016 · 2024-09-04T17:12:25Z

This change would not have any dependence on off-heap runtime changes,
and should handle old core file (before any off-heap changes introduced)
, intermediate core file (-XXgc:disableIndexableDualHeaderShape) and new
core file (-XXgc:enableVirtualLargeObjectHeap or
-XXgc:disableVirtualLargeObjectHeap) smoothly.

Use gcExtensions.isVirtualLargeObjectHeapEnabled for identifying
if off heap is enabled or not.
Verify the DataAddr via hasCorrectDataAddrPointer() if
isVirtualLargeObjectHeapEnabled.
Use javaVM.isIndexableDataAddrPresent for identifying if there is
extra DataAddr field in the header of IndexableObject.
Output DataAddr after size for IndexableObject if
isIndexableDataAddrPresent.
Handle new structures
J9IndexableObjectWithDataAddressContiguous/Discontiguous
Use arrayletObjectModel->_arrayletRangeBase/
_arrayletRangeTop to verify if the address in heap.

Signed-off-by: lhu linhu@ca.ibm.com

dmitripivkine · 2024-09-04T18:18:04Z

-					}
-					if (false == isCorrectDataAddrPointer) {
+
+					if (false == J9IndexableObjectHelper.hasCorrectDataAddrPointer(array)) {


no need to do false ==, just !

amicic · 2024-09-04T18:25:27Z

+
+			if (dataSizeInBytes.isZero()) {
+				VoidPointer discontiguousDataAddr = VoidPointer.cast(arrayPtr.addOffset(J9IndexableObjectHelper.discontiguousHeaderSize()));
+				hasCorrectDataAddrPointer = (dataAddr.isNull() || dataAddr.equals(discontiguousDataAddr));


We could be more strict if enableVirtualLargeObjectHeap is true, so that we disallow null.
Null is only possible in interim builds that don't have offheap code or in new builds with offheap disabled.

actually, my comment was not correct. at least for interpreter, we will have it null.

amicic · 2024-09-04T18:35:16Z

@@ -1194,19 +1194,8 @@ private int checkJ9Object(J9ObjectPointer object, GCHeapRegionDescriptor regionD
 			if (J9BuildFlags.J9VM_ENV_DATA64 && isIndexableDataAddressFlagSet() && ObjectModel.isIndexable(object)) {
 				if (!_javaVM.isIndexableDataAddrPresent().isZero()) {


try to check if offheap is enabled here, and proceed with per object check only if it's enabled

if it's disabled we don't really need to do any check against dataAddr (even random value is ok, even though, it might be set properly by interpreter or often, but not alway zeroed by JIT).

amicic · 2024-09-04T19:00:32Z

+
+		UDATA heapAddr = UDATA.cast(address);
+		UDATA heapBase = UDATA.cast(extensions.cardTable()._heapBase());
+		UDATA heapTop = UDATA.cast(extensions.cardTable()._heapAlloc());


we should not be fetching base/alloc from card table. probalby those fileds exist in ext.heap

amicic · 2024-09-04T19:34:14Z

+				dataAddr = null;
+			}
+			out.format("    U_64 DataAddr = %s, %n", (dataAddr == null)? "NULL" : dataAddr.getHexAddress());
+		}


move after the size printing

No space before %n; missing space before ?.

This has not been addressed.

LinHu2016 · 2024-09-09T20:07:46Z

Hi @keithc-ca Could you please review the changes, Thanks

keithc-ca · 2024-09-09T18:46:42Z

+				dataAddr = null;
+			}
+			out.format("    U_64 DataAddr = %s, %n", (dataAddr == null)? "NULL" : dataAddr.getHexAddress());
+		}


No space before %n; missing space before ?.

keithc-ca

Several of my previous review comments have not been addressed: please correct that.

keithc-ca · 2024-09-10T17:58:55Z

+				dataAddr = null;
+			}
+			out.format("    U_64 DataAddr = %s, %n", (dataAddr == null)? "NULL" : dataAddr.getHexAddress());
+		}


This has not been addressed.

keithc-ca · 2024-09-10T18:01:37Z

Please update the commit message and the description here to use the full names of the relevant options (e.g. refer to -XXgc:disableIndexableDualHeaderShape instead of XXgc:disableIndexableDualHeaderShape).

keithc-ca

Please also update the commit message:

correct the spelling of "dependence"
start sentences with a capital
insert a space before parenthetical comments
avoid lines longer than 72 characters

amicic · 2024-09-12T14:23:33Z

Functionally, looks good. I'll approve, although expect that a couple of comments from @keithc-ca (like updating PR comment) to be addressed.

This change would not have any dependence on off-heap runtime changes, and should handle old core file (before any off-heap changes introduced) , intermediate core file (-XXgc:disableIndexableDualHeaderShape) and new core file (-XXgc:enableVirtualLargeObjectHeap or -XXgc:disableVirtualLargeObjectHeap) smoothly. - Use gcExtensions.isVirtualLargeObjectHeapEnabled for identifying if off heap is enabled or not. - Verify the DataAddr via hasCorrectDataAddrPointer() if isVirtualLargeObjectHeapEnabled. - Use javaVM.isIndexableDataAddrPresent for identifying if there is extra DataAddr field in the header of IndexableObject. - Output DataAddr after size for IndexableObject if isIndexableDataAddrPresent. - Handle new structures J9IndexableObjectWithDataAddressContiguous/Discontiguous - Use arrayletObjectModel->_arrayletRangeBase/ _arrayletRangeTop to verify if the address in heap. Signed-off-by: lhu <linhu@ca.ibm.com>

keithc-ca · 2024-09-12T18:04:48Z

Jenkins test sanity,extended amac jdk21

LinHu2016 force-pushed the off-heap-ddr-part branch from f2de1ae to c62209d Compare September 4, 2024 17:29

dmitripivkine reviewed Sep 4, 2024

View reviewed changes

amicic reviewed Sep 4, 2024

View reviewed changes

LinHu2016 force-pushed the off-heap-ddr-part branch 9 times, most recently from d82fdea to 47a96f2 Compare September 9, 2024 15:08

keithc-ca self-requested a review September 9, 2024 16:12

LinHu2016 force-pushed the off-heap-ddr-part branch 2 times, most recently from dce38d6 to 441b94e Compare September 9, 2024 20:05

keithc-ca requested changes Sep 9, 2024

View reviewed changes

LinHu2016 force-pushed the off-heap-ddr-part branch from 441b94e to 6205144 Compare September 10, 2024 13:41

keithc-ca requested changes Sep 10, 2024

View reviewed changes

keithc-ca reviewed Sep 10, 2024

View reviewed changes

Comment thread debugtools/DDR_VM/src/com/ibm/j9ddr/AuxFieldInfo29.dat Outdated

LinHu2016 force-pushed the off-heap-ddr-part branch 2 times, most recently from 35025a6 to b8a96b4 Compare September 11, 2024 12:51

LinHu2016 requested a review from keithc-ca September 11, 2024 15:09

keithc-ca requested changes Sep 11, 2024

View reviewed changes

Comment thread ...m/j9ddr/vm29/tools/ddrinteractive/structureformat/extensions/J9ObjectStructureFormatter.java Outdated

amicic approved these changes Sep 12, 2024

View reviewed changes

LinHu2016 force-pushed the off-heap-ddr-part branch from b8a96b4 to 128d447 Compare September 12, 2024 17:24

LinHu2016 requested a review from keithc-ca September 12, 2024 17:24

dmitripivkine approved these changes Sep 12, 2024

View reviewed changes

keithc-ca approved these changes Sep 12, 2024

View reviewed changes

keithc-ca merged commit a600d90 into eclipse-openj9:master Sep 12, 2024

amicic added the comp:gc label Mar 17, 2025

keithc-ca mentioned this pull request Jun 2, 2026

DDR: cache VM.isIndexableDataAddrPresent #24042

Merged

		@@ -1194,19 +1194,8 @@ private int checkJ9Object(J9ObjectPointer object, GCHeapRegionDescriptor regionD
		if (J9BuildFlags.J9VM_ENV_DATA64 && isIndexableDataAddressFlagSet() && ObjectModel.isIndexable(object)) {
		if (!_javaVM.isIndexableDataAddrPresent().isZero()) {

Uh oh!

Conversation

LinHu2016 commented Sep 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amicic Sep 10, 2024 • 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!

LinHu2016 commented Sep 9, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keithc-ca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithc-ca commented Sep 10, 2024

Uh oh!

Uh oh!

keithc-ca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amicic commented Sep 12, 2024

Uh oh!

keithc-ca commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LinHu2016 commented Sep 4, 2024 •

edited

Loading

amicic Sep 10, 2024 •

edited

Loading