Initialize dataAddr field only for non-zero length array #20869

VermaSh · 2024-12-23T19:10:20Z

Update array inline allocation sequence to initialize dataAddr field only for non-zero length arrays. Field should be left blank for zero length arrays. Additionally this also clears padding field after the size field.

VermaSh · 2024-12-23T19:17:29Z

Off-heap personal build, upstream personal build. My off-heap personal build is looking good so far, only 1 unrelated failure. I'll remove the WIP tag once both builds have passed without off-heap related failures.

cc: @r30shah / @zl-wang / @dmitripivkine

VermaSh · 2024-12-24T18:13:59Z

Launched new non-off-heap personal build to verify PR changes

VermaSh · 2025-01-03T19:19:24Z

Relaunched non off-heap build

VermaSh · 2025-01-07T05:45:29Z

I have verified the sequence for x and z through a unit test. Need to do the same for power and aarch64 before removing WIP tag.

VermaSh · 2025-01-07T22:26:11Z

Launched personal non off-heap build for power.

VermaSh · 2025-01-17T23:59:28Z

@r30shah I have updated the PR based on your suggestions. Can I please get another review?

VermaSh · 2025-01-22T16:10:19Z

@r30shah Since we only need to clear dataAddr field for off-heap I have moved padding related changes to a separate PR. I need to do more testing to verify that my changes don't break 32 bit builds. I'll also look into adding an API to determine whether the padding exists or not.

Can I please get a review for remaining changes?

r30shah · 2025-01-22T16:22:29Z

@r30shah Since we only need to clear dataAddr field for off-heap I have moved padding related changes to a separate #20998. I need to do more testing to verify that my changes don't break 32 bit builds. I'll also look into adding an API to determine whether the padding exists or not.

@VermaSh As overall changes are under review and most of the review is done, I would appreciate and prefer to go ahead with the changes in this PR (Just think about the effort on review and testing), unless you make it makes logical sense to do it when most of the review is done and we are about to merge this PR.

VermaSh · 2025-01-22T19:11:37Z

@r30shah Verified with my changes, we should be good for both clearing padding for 64 bit and 32 bit modes. BTW, I added an assert for getObjectAlignmentInBytes in the helper we use to clear array header. Let me know if it helps state the assumption in a readable manner or just adds more confusion.

r30shah · 2025-01-24T19:41:04Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+            node,
+            temp3Reg,
+            generateS390MemoryReference(targetReg, TR::Compiler->om.contiguousArrayHeaderSizeInBytes(), cg));
+         cursor = generateRXInstruction(cg,


Should we use query to get store opcode which returns appropriate opcode for platform (I do understand we may not have off heap for 31 bit, but should use query to use right opcde)

r30shah · 2025-01-24T19:41:27Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+         // Since 2nd dimension array is 0 length, we can use secondDimLenReg
+         // to clear dataAddr field. secondDimLenReg is expected to be NULL
+         // at this point.
+         cursor = generateRXInstruction(cg, TR::InstOpCode::STG, node, secondDimLenReg, generateS390MemoryReference(temp2Reg, fej9->getOffsetOfDiscontiguousDataAddrField(), cg));


Same question as before, should we use query ?

r30shah · 2025-01-24T19:45:17Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+
+      // Clear 4 byte padding after the size field in the array header.
+      TR_ASSERT_FATAL_WITH_NODE(node,
+         TR::Compiler->om.getObjectAlignmentInBytes() == 8,


This is rather long message to print for assert and I am not sure if it makes sense to print that out when someone hits the assert. It does not provide any more value to the failure than having a simple comment here that developer can take a look and see. Also what happens to this assert where using 31 bit ?

Object alignment is 8 bytes on 31 bit as well so this assert will be valid there as well (minimum object alignment). I think it will be better to check OMR_MINIMUM_OBJECT_SIZE rather than object alignment. Looking to see if we have an API for it or if I need to add one. This will help clean up the assert message as well.

May be I should ask you this differently. Think about the what is it that you want to prevent with having this Fatal Assert here ?

This was my train of thought behind adding the assert:

We want to avoid corrupting an other object while clearing padding for contiguous and discontiguous header layout. The padding field exists for contiguous header layout in full refs and for discontiguous header layout in compressed refs. But we don't know if the array we are dealing with has the field, we won't know until we do a runtime check. To avoid the extra instruction, we assume that the array has the field and clear memory at corresponding padding offset.

This is where the minimum object size comes into play. For compressed refs, contiguous header is 8 bytes and discontiguous header is 16 bytes, with the padding field at +12 offset. Since we unconditionally clear memory at +12 offset, we could potentially corrupt the object itself (if the array is actually contiguous) or adjacent objects.

We don't need to worry about corrupting the object itself because we are in allocation phase so there is nothing to corrupt at that memory offset. And as long as OMR_MINIMUM_OBJECT_SIZE is 16 bytes, we don't need to worry about corrupting any adjacent objects either.

Does that sound good? Should I change the assert and/or add additional details?

Here's the chart I am using for different array header layouts

Source: https://github.com/eclipse-openj9/openj9/blob/master/runtime/oti/j9nonbuilder.h

Thanks a lot for the explanation. What I was asking, is that, we do not need to write this explanation in the ASSERT message when for some reason this condition gets violated. I agree with your reasoning for having the Fatal Assert (To catch the case if things get changed, we catch the issue earlier in testing to prevent accidentally clearing other object).

I will let you figure out what query would be the correct one here , but recommend you to have a concise trace message in the trace so JIT developer/ service team can go through the code and understand why that assert was thrown.

Ah yes, I agree, I'll push updated assert message shortly.

r30shah · 2025-01-24T19:47:23Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+               iCursor = generateRRFInstruction(cg, TR::InstOpCode::LOCGR, node, dataSizeReg, offsetReg, getMaskForBranchCondition(TR::InstOpCode::COND_BE), true, iCursor);
+
+               // Write element address to dataAddr field
+               iCursor = generateRXInstruction(cg,


Same question as before, should we use query to get store opcode ?

r30shah · 2025-01-24T19:47:41Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+                  generateS390MemoryReference(resReg, TR::Compiler->om.contiguousArrayHeaderSizeInBytes(), cg),
+                  iCursor);
+               // Write first data element address to dataAddr field
+               iCursor = generateRXInstruction(cg,


same question as before.

VermaSh · 2025-01-27T21:15:01Z

@r30shah I have addressed all expect the object alignment suggestion. I need some more time to fix that.

VermaSh · 2025-02-07T16:24:47Z

Created a PR for minimum object size API, will update this PR as soon as that is merged. Setting this to WIP till then.

Update array inline allocation sequence to initialize dataAddr field only for non-zero size arrays. Field should be left blank for zero size arrays. Signed-off-by: Shubham Verma <[email protected]>

VermaSh · 2025-02-12T18:00:03Z

@r30shah I found J9_GC_MINIMUM_OBJECT_SIZE that we can use to assert minimum object size in bytes. Can I please get another review?

r30shah

LGTM

r30shah · 2025-02-14T14:43:36Z

jenkins test sanity zlinux jdk11,jdk21

r30shah · 2025-02-15T09:54:08Z

Merging as all tests have passed.

VermaSh force-pushed the inline_array_allocation_z branch from ff0ba22 to 5938046 Compare December 23, 2024 19:13

VermaSh marked this pull request as draft December 23, 2024 19:17

VermaSh changed the title ~~Intialize dataAddr field only for non-zero length array~~ WIP: Intialize dataAddr field only for non-zero length array Dec 23, 2024

VermaSh force-pushed the inline_array_allocation_z branch from 5938046 to 54bf0a5 Compare December 24, 2024 18:11

VermaSh force-pushed the inline_array_allocation_z branch 2 times, most recently from da4a877 to 9b31e29 Compare December 27, 2024 16:50

VermaSh force-pushed the inline_array_allocation_z branch 8 times, most recently from 1317bac to c3c566c Compare January 3, 2025 19:10

VermaSh force-pushed the inline_array_allocation_z branch 5 times, most recently from 648f364 to 068425c Compare January 7, 2025 05:42

VermaSh force-pushed the inline_array_allocation_z branch from 068425c to 6d13fde Compare January 7, 2025 22:15

VermaSh force-pushed the inline_array_allocation_z branch 3 times, most recently from 71bbabb to 8f03946 Compare January 8, 2025 18:20

VermaSh changed the title ~~WIP: Intialize dataAddr field only for non-zero length array~~ Intialize dataAddr field only for non-zero length array Jan 8, 2025

VermaSh marked this pull request as ready for review January 8, 2025 18:21

VermaSh force-pushed the inline_array_allocation_z branch 2 times, most recently from 961e9c7 to 152a6d8 Compare January 17, 2025 23:57

VermaSh force-pushed the inline_array_allocation_z branch 2 times, most recently from bbd1f85 to 2d64d57 Compare January 22, 2025 16:03

VermaSh force-pushed the inline_array_allocation_z branch 2 times, most recently from 2aa605a to 5166143 Compare January 22, 2025 18:57

r30shah requested changes Jan 24, 2025

View reviewed changes

VermaSh force-pushed the inline_array_allocation_z branch from 5166143 to d671f61 Compare January 27, 2025 21:13

VermaSh force-pushed the inline_array_allocation_z branch 2 times, most recently from e0be939 to 262b9ce Compare January 29, 2025 18:42

VermaSh requested a review from dsouzai as a code owner January 29, 2025 18:42

VermaSh force-pushed the inline_array_allocation_z branch 3 times, most recently from 2bf9312 to b4d1a22 Compare January 30, 2025 19:28

VermaSh marked this pull request as draft February 7, 2025 16:24

VermaSh changed the title ~~Intialize dataAddr field only for non-zero length array~~ Initialize dataAddr field only for non-zero length array Feb 7, 2025

z: NULL initialize dataAddr field for 0 size arrays

4c4826f

Update array inline allocation sequence to initialize dataAddr field only for non-zero size arrays. Field should be left blank for zero size arrays. Signed-off-by: Shubham Verma <[email protected]>

VermaSh force-pushed the inline_array_allocation_z branch from b4d1a22 to 4c4826f Compare February 12, 2025 17:59

VermaSh marked this pull request as ready for review February 12, 2025 17:59

r30shah approved these changes Feb 14, 2025

View reviewed changes

r30shah merged commit c9b8510 into eclipse-openj9:master Feb 15, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize dataAddr field only for non-zero length array #20869

Initialize dataAddr field only for non-zero length array #20869

VermaSh commented Dec 23, 2024 •

edited

Loading

VermaSh commented Dec 23, 2024 •

edited

Loading

VermaSh commented Dec 24, 2024

VermaSh commented Jan 3, 2025

VermaSh commented Jan 7, 2025

VermaSh commented Jan 7, 2025

VermaSh commented Jan 17, 2025

VermaSh commented Jan 22, 2025

r30shah commented Jan 22, 2025

VermaSh commented Jan 22, 2025 •

edited

Loading

r30shah Jan 24, 2025

r30shah Jan 24, 2025

r30shah Jan 24, 2025

VermaSh Jan 27, 2025

r30shah Jan 27, 2025

VermaSh Jan 27, 2025

VermaSh Jan 27, 2025 •

edited

Loading

r30shah Jan 27, 2025

VermaSh Jan 28, 2025

r30shah Jan 24, 2025

r30shah Jan 24, 2025

VermaSh commented Jan 27, 2025 •

edited

Loading

VermaSh commented Feb 7, 2025

VermaSh commented Feb 12, 2025

r30shah left a comment

r30shah commented Feb 14, 2025

r30shah commented Feb 15, 2025

Initialize dataAddr field only for non-zero length array #20869

Initialize dataAddr field only for non-zero length array #20869

Conversation

VermaSh commented Dec 23, 2024 • edited Loading

VermaSh commented Dec 23, 2024 • edited Loading

VermaSh commented Dec 24, 2024

VermaSh commented Jan 3, 2025

VermaSh commented Jan 7, 2025

VermaSh commented Jan 7, 2025

VermaSh commented Jan 17, 2025

VermaSh commented Jan 22, 2025

r30shah commented Jan 22, 2025

VermaSh commented Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VermaSh Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VermaSh commented Jan 27, 2025 • edited Loading

VermaSh commented Feb 7, 2025

VermaSh commented Feb 12, 2025

r30shah left a comment

Choose a reason for hiding this comment

r30shah commented Feb 14, 2025

r30shah commented Feb 15, 2025

VermaSh commented Dec 23, 2024 •

edited

Loading

VermaSh commented Dec 23, 2024 •

edited

Loading

VermaSh commented Jan 22, 2025 •

edited

Loading

VermaSh Jan 27, 2025 •

edited

Loading

VermaSh commented Jan 27, 2025 •

edited

Loading