Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Adjust arguments to transformed UnsafeAPI calls when offheap is enabled #20333

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

midronij
Copy link
Contributor

@midronij midronij commented Oct 11, 2024

During recognizedCallTransformer and unsafeFastPath, Unsafe.getAndAdd() or Unsafe.getAndSet() can be transformed into an atomicFetchAndAdd/atomicSwap helper call. For the UnsafeAPI methods, the destination address is passed in as two separate values: a base object address and an offset. When they are transformed into atomic helper calls, these values are added together and passed in as a single destination address value. However, this does not account for the differing shape of array objects under offheap/balanced GC policy (as opposed to the default, gencon).

Thus, this contribution includes the following changes

  • For both recognizedCallTransformer and unsafeFastPath, if the object is an array, the dataAddr pointer is loaded and used as the base address.
  • For recognizedCallTransformer, an array check is added to the series of runtime tests performed before calling the atomic helper, to ensure that dataAddr is only used if the object is indeed an array.

When Unsafe.getAndAdd()/getAndSet() is called on an array and offheap
changes are enabled, adjust arguments so that dataAddr is passed in as
the base address of the object.

Signed-off-by: midronij <[email protected]>
@midronij
Copy link
Contributor Author

@zl-wang here is the Unsafe.getAndAdd()/getAndSet() fix! Marked as a WIP since I am still finalizing the unsafeFastPath changes (the last two commits)

intrinsics during unsafeFastPath

During unsafeFastPath, some UnsafeAPI calls (such as Unsafe.getAndAdd())
are converted to atomic intrinsics (such as atomicFetchAndAdd). When
this occurs with offheap allocation enabled, and the object is an array,
load dataAddr.

Signed-off-by: midronij <[email protected]>
{
//generate array check treetop
TR::Node *constNode = TR::Node::lconst(unsafeCall, ~(J9_SUN_FIELD_OFFSET_MASK));
TR::Node *unsafeAddress = TR::Node::create(TR::aladd, 2, objectNode, TR::Node::create(TR::land, 2, offsetNode, constNode));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how can you do aladd before you know if it is an array or not? it is unclear to me what purpose this aladd is for.

TR::Node *unsafeAddress = TR::Node::create(TR::aladd, 2, objectNode, TR::Node::create(TR::land, 2, offsetNode, constNode));
TR::Node *vftLoad = TR::Node::createWithSymRef(TR::aloadi, 1, 1,
TR::Node::createWithSymRef(unsafeAddress,
comp()->il.opCodeForDirectLoad(unsafeAddress->getDataType()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

directload opcode is for static field i think ... this code looks contradictory to where the addressing is from.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is consistent with other areas where runtime array checks are performed, such as with Unsafe.get/put():

TR::Node *objLoad = TR::Node::createWithSymRef(unsafeAddress,
                                       comp()->il.opCodeForDirectLoad(unsafeAddress->getDataType()), 0,
                                       newSymbolReferenceForAddress);
TR::Node *vftLoad = TR::Node::createWithSymRef(TR::aloadi, 1, 1, objLoad,
                                       comp()->getSymRefTab()->findOrCreateVftSymbolRef());
.
.
.
TR::Node *testIsArrayFlag = comp()->fej9()->testIsClassArrayType(vftLoad);
TR::Node *isArrayNode = TR::Node::createif(TR::ificmpne, testIsArrayFlag, TR::Node::create(TR::iconst, 0), NULL);

(from genCodeForUnsafeGetPut())

comp()->il.opCodeForDirectLoad(unsafeAddress->getDataType()),
0,
objectNode->getSymbolReference()),
comp()->getSymRefTab()->findOrCreateVftSymbolRef());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't understand this either: you seemed assuming the previous directload result as an object. at the same time, you look up the opcode through a table (while you know the datatype already).

@ianguo
Copy link

ianguo commented Oct 23, 2024

During recognizedCallTransformer and unsafeFastPath, Unsafe.getAndAdd() or Unsafe.getAndSet() can be transformed into an atomicFetchAndAdd/atomicSwap helper call. For the UnsafeAPI methods, the destination address is passed in as two separate values: a base object address and an offset. When they are transformed into atomic helper calls, these values are added together and passed in as a single destination address value. However, this does not account for the differing shape of array objects under offheap/balanced GC policy (as opposed to the default, gencon).

Thus, this contribution includes the following changes

  • For both recognizedCallTransformer and unsafeFastPath, if the object is an array, the dataAddr pointer is loaded and used as the base address.
  • For recognizedCallTransformer, an array check is added to the series of runtime tests performed before calling the atomic helper, to ensure that dataAddr is only used if the object is indeed an array.

During the running period of our application, from the thread stack, it has been "staying" at AtomicReferenceArray#getAndSet. A phenomenon similar to livelock has occurred. As a result, the thread is blocked and the application is seriously unrecoverable. I am not sure if it is related to your problem? @midronij @zl-wang

thread stack:
"pool-EventCacheMessageHandler-thread-1" Id=21859 RUNNABLE
at sun.misc.Unsafe.getAndSetObject(Unsafe.java:1165)
at java.util.concurrent.atomic.AtomicReferenceArray.getAndSet(AtomicReferenceArray.java:164)
at com.fasterxml.jackson.core.util.BufferRecycler.allocCharBuffer(BufferRecycler.java:159)
at com.fasterxml.jackson.core.io.IOContext.allocTokenBuffer(IOContext.java:270)
at com.fasterxml.jackson.core.JsonFactory.createParser(JsonFactory.java:1164)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3629)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3597)

java version:
openjdk version "1.8.0_422"
IBM Semeru Runtime Open Edition (build 1.8.0_422-b05)
Eclipse OpenJ9 VM (build openj9-0.46.0, JRE 1.8.0 Linux amd64-64-Bit Compressed References 20240802_1004 (JIT enabled, AOT enabled)
OpenJ9 - 1a6f612
OMR - 840a9adba
JCL - a75ff73ce5 based on jdk8u422-b05)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants