Lower segment-view VarHandle accesses in UnsafeFastPath in JDK 21+#24199
Conversation
UnsafeFastPath already converts jdk.internal.misc.Unsafe.get/put calls reached from the array, instance-field and static-field VarHandle operation methods into direct loads and stores. The MemorySegment view VarHandle operation classes: VarHandleSegmentAsBytes, AsChars, AsShorts, AsInts, AsLongs, AsFloats and AsDoubles, have a similar shape but were not recognized, so an access made through a segment-view VarHandle remained as an Unsafe call instead of folding to a direct access. This commit adds an umbrella recognizer, java_lang_invoke_VarHandleSegmentAsX_method, which is then used in UnsafeFastPath. A few notes: * The access base is null for a native segment and a primitive array for a heap segment. Both reduce to a single aladd(base, offset) load/store only while arrays are contiguous and on-heap, so the method is lowered only when arraylets and off-heap allocation are ruled out, the access is treated as a packed array element, and the base is not marked non-null (it may actually be null). * The Unsafe.get/putXUnaligned wrappers, used by the view classes for the unaligned element shapes, are reduced to a single load/store for packed-element callers on targets that allow misaligned accesses. This reduces the runtime overhead of MemorySegment accesses that reach the VarHandle path rather than the direct-lowering fast path mechanism that will be added in the near future. Signed-off-by: Nazim Bhuiyan <nubhuiyan@ibm.com>
nbhuiyan
force-pushed
the
memsegment-jun26-p2
branch
from
7cb043e to
1fc59c0
Compare
|
@vijaysun-omr @mpirvu Requesting review. This is the second of three FFM-related opt work, and this PR is building on top of the work contributed in #24190. |
| if (TR::Compiler->om.canGenerateArraylets()) | ||
| return false; | ||
| #if defined(J9VM_GC_SPARSE_HEAP_ALLOCATION) | ||
| if (TR::Compiler->om.isOffHeapAllocationEnabled()) |
There was a problem hiding this comment.
from reading the comment, the two cases (either native or byte-array object) can be distinguished by the fact that base == null, such that off-heap can handle (fastPath) them in a similar way as in gencon, except loading up dataAddr for byte-array off-heap. I am pretty sure we already did this for off-heap static vs. instance-field.
There was a problem hiding this comment.
I'm not familiar with MemorySegments and its nuances, but we fastPath unsafe accesses by inserting runtime check for base == null and isArray to insert load dataAddr if needed. Will look up the code and helpers for that as they might be useful here too.
There was a problem hiding this comment.
for this one, you even don't need to test isArray, since base!=NULL means isArray already
There was a problem hiding this comment.
Thanks for your insights @rmnattas @zl-wang. I will investigate and implement the off heap support in the near future, and that can be part of a different PR. I might have to reach out to you for additional details for this.
@vijaysun-omr, I think we could add the off-heap support in a separate PR in the future, as this PR forms part of a bigger piece of work that has been thoroughly tested end-to-end, and any major change to this PR will delay the final (and the most impactful) piece of MemorySegment optimization.
There was a problem hiding this comment.
Thanks, I will approve now and you can add the off heap support separately.
There was a problem hiding this comment.
There's the createUnsafeGetWithOffset helper but its much more inclusive than what it seem you need here, (it checks type signature and interface possibility).
Maybe you can point me to the code that does the transformation as some helpers already support OffHeap.
|
I'm going to start testing anyway, even though the off heap question needs to be answered. I would be fine if that was handled via a separate PR in the near future (or equally, via this PR). |
|
jenkins test sanity.functional all jdk25 |
|
I doubt the failures here are "real" since they were also seen in #24110 where I launched testing around the same time, but you can check and post. |
mpirvu
left a comment
mpirvu
left a comment
There was a problem hiding this comment.
The code looks good to me as far as I understand.
The AIX failure was due to infra:
19:04:26 Checking out Revision aa00f33e6a257eb8096d9dd2171aed951ca608f0 (detached)
18:59:34 > /opt/freeware/bin/git rev-parse aa00f33e6a257eb8096d9dd2171aed951ca608f0^{commit} # timeout=10
18:59:34 > /opt/freeware/bin/git config core.sparsecheckout # timeout=10
18:59:34 > /opt/freeware/bin/git checkout -f aa00f33e6a257eb8096d9dd2171aed951ca608f0 # timeout=30
19:34:27 ERROR: Checkout failed
19:34:27 hudson.plugins.git.GitException: Command "/opt/freeware/bin/git checkout -f aa00f33e6a257eb8096d9dd2171aed951ca608f0" returned status code 143:
19:34:27 stdout:
19:34:27 stderr:
19:34:27 at PluginClassLoader for git-client//org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:2847)
19:34:27 at PluginClassLoader for git-client//org.jenkinsci.plugins.gitclient.CliGitAPIImpl$9.execute(CliGitAPIImpl.java:3181)
|
jenkins test sanity.functional aix jdk25 |
|
x86-64 macOS failure is due to #24214 |
|
I have approved from my side, but I see that Marius started the AIX job and that is still pending. One of us can notice when that is done and merge it. |
|
The AIX job completed successfully. |
5 participants
UnsafeFastPath already converts jdk.internal.misc.Unsafe.get/put calls reached from the array, instance-field and static-field VarHandle operation methods into direct loads and stores. The MemorySegment view VarHandle operation classes: VarHandleSegmentAsBytes, AsChars, AsShorts, AsInts, AsLongs, AsFloats and AsDoubles, have a similar shape but were not recognized, so an access made through a segment-view VarHandle remained as an Unsafe call instead of folding to a direct access.
This commit adds an umbrella recognizer,
java_lang_invoke_VarHandleSegmentAsX_method, which is then used in UnsafeFastPath. A few notes:
This reduces the runtime overhead of MemorySegment accesses that reach the VarHandle path rather than the direct-lowering fast path mechanism that will be added in the near future.