Adjust arguments to Unsafe array operations while offheap is enabled

[midronij]

In this contribution, the following adjustments have been made to the arguments that are passed in to Unsafe.get(), Unsafe.put(), and Unsafe.compareAndSwap() when offheap/balanced GC policy is in use:

• The dataAddr pointer is loaded and passed in as the base address of the object being operated on
• The array header size is subtracted from the offset
  no longer necessary due to this change: Array base offset zero #19326

This is to account for the differing array shape under offheap/balanced GC policy as opposed to the default (gencon).

In addition, as a performance improvement for both the offheap and gencon cases, when the object type is known at compile time, we can eliminate unnecessary lowtag, class, and array checks that are currently performed at runtime for the following cases:

• Object is known to be an array at compile time
• Object type is known to be non-array, non-class, non-java/lang/Object at compile time

Depends on #18303

[midronij]

@zl-wang @VermaSh @vijaysun-omr @0xdaryl here is the fix for the UnsafeAPI issue!

[VermaSh]

@midronij Thank you for working on this!
Did you test with latest Off heap changes [1-2]?

Also, can you please add a commit to remove checks which were added to disable unsafe array operations for off heap. You can find those checks in [1-2].

[1] openj9/offHeapMemoryCommitAndDecommitFinal_combined_without_new_macro
[2] omr/offHeapMemoryCommitAndDecommitFinal_combined_without_new_macro

[VermaSh]

Does this fix unsafe.setMemory as well?

[midronij]

@VermaSh I've added reversions of the following commits that are no longer necessary with the changes included in this PR:
VermaSh@58efdd7
VermaSh@5734c30

With these additions as well at the latest offheap changes, everything still works as expected: the IL trees include the array check and adjusted base address pointer/offset, and the UnsafeArrayGetTests are passing on all platforms.

However, this contribution does not deal with Unsafe.setMemory(). As a next step, I can add fixes for that as well as any other remaining Unsafe array operations to this PR.

[VermaSh]

Just a few minor suggestions. Also, here's the GC/VM PR these changes depend on #14667

[zl-wang]

i will read more to comment further next week

[midronij]

@zl-wang I have finished addressing your review suggestions. As well, I have provided some examples of what the post-inlining IL trees will look like with my changes here. Please let me know if there is anything else I should address with these additions.

Assuming everything looks good, I believe we are waiting on the following before we can merge this PR:

• Introduce Off-Heap Technology for Large Arrays in Balanced Region-Based Garbage Collector #14667 needs to get merged, since I believe this is where the J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION macro and the TR::Compiler->om.isOffHeapAllocationEnabled() check get implemented.
• As of right now, there appears to be a problem with one of the JIT helper methods, getIntFromObject(), which calls Unsafe.getInt(), that causes a NullPointerException when trying to build the git tests on vmfarm, and intermittently causes segmentation faults when running javac locally. I spoke to @VermaSh about this, and it seems that this was a previously existing issue which we are still waiting on a fix for.
• If we want to include them in this PR, then Unsafe.getAndAdd(), Unsafe.getAndSet(), Unsafe.setMemory(), and Unsafe.copyMemory() also need to have their arguments adjusted to account for offheap array allocation. This is what I'm working on right now.

If there is anything that I've missed or that I'm mistaken about, please let me know!

[zl-wang]

Works for the other Unsafe methods (if they are inlined in gencon ... this is certainly the condition) had better put up in a separate PR.

I supposed merging this PR has no effect on running none-offheap policies. i.e. it is relatively safe to merge. every existing logics (for none-offheap) are still honoured.

Comments on the first example IL:
BBStart <block_12> ificmpeq (equal to 0x10000) can be replaced with ificmpne (not equal to 0). better code can be generated for the latter case, since constant 0x10000 is big for Power ISA;

[zl-wang]

for the other two examples, i am wondering why we still generated the lowTag and class.class tests? are they necessary? at least, you should check against the ILs generated for gencon. maybe, they are needed for some reasons not welknown.

[zl-wang]

the better question is: if there is an example without any checks? there must be

[midronij]

the better question is: if there is an example without any checks? there must be

For Unsafe.get() and Unsafe.put(), as far as I can tell, when using gencon GC policy (even prior to my changes), there is no case where no checks are performed. Normally, there is a lowtag check and a class check. If the J9Class of java/lang/Class cannot be loaded, then there is an array check and a lowtag check. So basically the lowtag check and one other is always generated no matter what. There's a little more detail on which checks are performed and when here:

openj9/runtime/compiler/optimizer/InlinerTempForJ9.cpp

Lines 1032 to 1102 in d3388e1

	/*
	Converting Unsafe.get/put* routines into inlined code involves two cases:
	1) if the size of the element to put/get is 4 bytes or more
	2) if the size of the element to put/get is less than 4 bytes (boolean, byte, char, short)
	In (1), there are two alternatives on how to read from/write to the object: direct and
	indirect write/read. The selection of alternatives is done by looking at three conditions:
	a) whether the object is NULL
	b) whether the object is array
	c) whether the object is of type java.lang.Class
	The pseudocode of the generated inline code for case (1) under normal compilation is :
	if (object == NULL)
	use direct access
	else if (offset is not low tagged)
	use direct access
	else if (object is type of java/lang/Class)
	use indirect access
	else
	use direct access
	If we can not get the J9Class of java/lang/Class, we generate following sequence of tests
	if (object == NULL)
	use direct access
	else if (object it Array type)
	use direct access
	else if (offset is low tagged)
	use indirect access
	else
	use direct access
	In (2), there are three alternatives on how to read from/write the object. direct,
	direct with conversion, indirect. The same three conditions are used to decide which one
	to use based on the following pseudocode:
	if (object is NULL)
	use direct access with conversion
	else if (object is array)
	use direct access with conversion
	else if (object is of type Class)
	use indirect access
	else
	use direct access
	- genClassCheckForUnsafeGetPut builds the treetop for condition (c) above.
	- genDirectAccessCodeForUnsafeGetPut completes the building of treetop for both "direct access" and
	"direct access with conversion"
	- genIndirectAccessCodeForUnsafeGetPut builds the treetop for indirect access
	- addNullCheckForUnsafeGetPut builds node for NULLness check (condition (a) above) and
	builds a diamond CFG based on that. The CFG will be completed in later stages.
	- createAnchorNodesForUnsafeGetPut creates compressed references in case they are needed
	- genCodeForUnsafeGetPut completes the CFG/code by adding the array check, Class check,
	and the direct access code.
	Note that in case (2), i.e., when the conversion is needed, we generate code like the
	following for the "direct access with conversion" for Unsafe.getByte
	b2i
	ibload
	aiadd
	while the direct access code looks like
	iiload
	aiadd
	We will replace b2i and ibload by c2iu and icload for Unsafe.getChar, by
	s2i and isload for Unsafe.getShort, and by bu2i and ibload for Unsafe.getBoolean
	For Unsafe.putByte and Unsafe.putBoolean, we generate
	ibstore
	i2b
	<some load node>
	We replace i2b and ibstore by i2c and icstore for Unsafe.getChar, and by i2s and isstore for
	Unsafe.getShort.
	*/

I'm not sure if there's a particular reason for that, or if it's simply because prior to my changes, we never actually checked if the object class was known at compile time.

[midronij]

@zl-wang @VermaSh I've added the commit that gets rid of the uncessary lowtag/class/array checks when the object type is known at compile time. Once the necessary GC/VM changes are in this should be good to go

[hzongaro]

I still need to review the changes to the code that calls genCodeForUnsafeGetPut, but I wanted to pass along my review comments thus far.

[hzongaro]

Most of my comments are pretty minor. Even the one about duplicated blocks might not need to be addressed, or if it does, it might be that it could be handled in a follow on change. . . .

[hzongaro]

I think the changes look good. Just a couple of minor comments on some comments in the code.

Also, the Line endings check failed. May I ask you to take a look at that?

Finally, I had a few comments on the commit comments.

• Looking at the description of commit e7441f2, it states that it's reverting commit 58efdd7 - but looking at the two, it looks like the new commit is only reverting part of the x86 changes of that old commit. May I ask you to update the commit comment to describe the change more explicitly?
• For commit 5608dae, per the OpenJ9 Commit Guidelines, may I ask you to add a first line to the commit comment summarizing the commit?
• In the comment for commit 3a6a63f, a few of the lines are longer than 72 characters.

[hzongaro]

One more late comment: there is a block of comments before createUnsafePutWithOffset that describes the different ways that IL might be generated for Unsafe get and put operations. May I ask you to update that to account for all the work you've done to add support for off-heap allocation?

[midronij]

@hzongaro thank you for all of your feedback! I've made updates to address your most recent wave of suggested changes to both the comments in the code and the commit descriptions. Regarding the following:

Looking at the description of commit e7441f2, it states that it's reverting commit 58efdd7 - but looking at the two, it looks like the new commit is only reverting part of the x86 changes of that old commit. May I ask you to update the commit comment to describe the change more explicitly?

Upon further inspection, I found that this commit was actually cancelled out by the very next one, 16af941, so I simply dropped both of them.

If everything looks good, then I think we can move on to testing and merging! @zl-wang when you have a moment could you please get the CI tests started?

[hzongaro]

Looks good. Thanks!

[zl-wang]

Jenkins test sanity xlinux,aix jdk8,jdk21

[pshipton]

Reverted as the suspected cause of #19947

I did a little internal testing on xmac with the change reverted and didn't see the failure. I'll go ahead with the revert and the builds tonight will confirm the failures are no longer seen.