Fold MethodHandle.asType in Lambdaform-generated methods#23481
Conversation
b2f3e0e to
a6b7aa1
Compare
e7b9e2f to
62f4605
Compare
| { x(TR::java_lang_invoke_MethodHandle_asType_instance, "asType", | ||
| "(Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/MethodHandle;") }, | ||
| { x(TR::java_lang_invoke_Invokers_checkGenericType, "checkGenericType", | ||
| "(Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/MethodHandle;") }, |
There was a problem hiding this comment.
This is not the right place to add this method. It is only for methods of class MethodHandle.
62f4605 to
28671e2
Compare
e63aa6b to
c07d3c9
Compare
|
thanks @nbhuiyan . I realized there is a much simpler way of checking the type compatibility. See PR description. |
| */ | ||
| TR::KnownObjectTable *knot = comp->getKnownObjectTable(); | ||
| if (!knot) | ||
| return false; |
There was a problem hiding this comment.
Return 0 here similar to the return 0 at the end, as the return type of the function is uintptr_t.
|
|
||
| /* We shouldn't need to check for anonymous classes or for same class loaders for the Hard Cache case. | ||
| * The java.lang.invoke infrastructure only sets this cache when it is safe to do so. | ||
| * TODO: if we find that the normal asTypeCache doesn't get many hits, add (careful) checks for the soft cache.å |
There was a problem hiding this comment.
Looks like some unintended character at the end of this line.
Also, I prefer not to add more TODO labels. The comment can be updated to describe the TODO as something to consider in the future.
| return cachedMH; | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
VMAccess ends here, with the function returning a raw pointer (if successful). The callers of this function then takes the resulting raw pointer, and then calls knot->getOrCreateIndex(result of this function), which is not GC safe. The VM access should be held all the way till the knot index is obtained, and the easiest way to do that would be to modify this function to return the knot index instead of the raw pointer, so that VM access is held throughout.
| */ | ||
| void process_java_lang_invoke_Invokers_checkVarHandleGenericType(TR::TreeTop *tt, TR::Node *node); | ||
|
|
||
| void process_java_lang_invoke_MethodHandle_asType(TR::TreeTop *tt, TR::Node *node); |
| #endif // TR_ALLOW_NON_CONST_KNOWN_OBJECTS | ||
| } | ||
|
|
||
| void TR_MethodHandleTransformer::process_java_lang_invoke_MethodHandle_asType(TR::TreeTop *tt, TR::Node *node) |
There was a problem hiding this comment.
This function should check if const resfs are enabled, without which it should abort.
| J9::ConstProvenanceGraph *cpg = comp()->constProvenanceGraph(); | ||
| // cpg->addEdge(cpg->knownObject(idx), clazz); | ||
| auto knot = comp()->getKnownObjectTable(); | ||
| bool transformed = false; |
There was a problem hiding this comment.
This will trigger unused variable warnings.
| logprintf(trace(), comp()->log(), "MethodHandle is obj%d\n", mhIndex); | ||
| logprintf(trace(), comp()->log(), "MethodType is obj%d\n", desiredMTIndex); | ||
| J9::ConstProvenanceGraph *cpg = comp()->constProvenanceGraph(); | ||
| // cpg->addEdge(cpg->knownObject(idx), clazz); |
There was a problem hiding this comment.
We should not be adding commented out code.
| uintptr_t convertedMH = fej9->getConvertedMethodhandle(comp(), mhIndex, desiredMTIndex); | ||
| if (0 != convertedMH) { | ||
| logprintf(trace(), comp()->log(), "Method types are the compatible%d\n"); | ||
| TR::KnownObjectTable::Index convertedMHIndex = knot->getOrCreateIndex(convertedMH); |
There was a problem hiding this comment.
Same issue as in InterpreterEmulator. It is not safe to call getOrCreateIndex with a raw pointer without VM access being held for the whole duration.
| void TR_MethodHandleTransformer::process_java_lang_invoke_MethodHandle_asType(TR::TreeTop *tt, TR::Node *node) | ||
| { | ||
| auto mhNode = node->getChild(0); | ||
| auto desiredMTNode = node->getChild(1); |
There was a problem hiding this comment.
Better to use node->getArgument instead of node->getChild when trying to access specific args of a call node, as otherwise we run into the risk of unintentionally accessing the receiver instead of the first arg. In this case, using getChild is not incorrect, but should ideally use getArgument.
| logprintf(trace(), comp()->log(), "Exact compatibilty check failed - checking subtype compatibility\n"); | ||
| uintptr_t convertedMH = fej9->getConvertedMethodhandle(comp(), mhIndex, desiredMTIndex); | ||
| if (0 != convertedMH) { | ||
| logprintf(trace(), comp()->log(), "Method types are the compatible%d\n"); |
There was a problem hiding this comment.
Typo, and a format specifier with no args to feed it.
| uintptr_t TR_J9VMBase::getConvertedMethodhandle(TR::Compilation *comp, TR::KnownObjectTable::Index mhIndex, | ||
| TR::KnownObjectTable::Index desiredTypeIndex) | ||
| { | ||
| /* Indidivual type compatibilty checks on each type in the MT are not needed. Since we only fold |
9d97321 to
cf8b089
Compare
|
thanks @nbhuiyan , this is ready for another review |
nbhuiyan
left a comment
There was a problem hiding this comment.
In addition to these review comments, please update JITServer MINOR_NUMBER in CommunicationStream.hpp
cf8b089 to
fad9036
Compare
|
Jenkins test sanity all jdk21 depends eclipse-omr/omr#8209 |
edbb7ea to
363a558
Compare
|
@0xdaryl the change I just pushed should fix the issue, can you run the tests again? |
|
Jenkins test sanity all jdk21 depends eclipse-omr/omr#8209 |
|
aarch64 failure is a jit server fail, looks like it is #14706 Z failure is an illegal instruction in the vector tests, looks unrelated X sanity.functional failure looks related to #22758 (comment) X sanity.openjdk is a java compile failure, unrelated #23943 |
|
all failures were found to be known in #23481 (comment) |
mpirvu
left a comment
There was a problem hiding this comment.
There are some conflicts that need to be resolved and it makes sense to rebase to test against the latest changes.
As far as I understand, this code will be exercised when const refs will be enabled. If that's the case, wouldn't be more prudent to enable const refs first and then run tests for this PR?
363a558 to
78bbf76
Compare
Adds recognized method for Invokers.checkGenericType (the callsite for MH.asType). Folds call to the asTypeCache if the MTs are exact same object. - Type checking each type in the MT in not needed. If the MT of the asTypeCache MH is the same as the MT of the target MH, we can just use the cache. If they are not the same, then we cannot fold as we have no way of obtaining the asType result at compile time. (we also should not even get here if they are not the same, since we shouldn't have a known-object for the target MT in that case). Signed-off-by: Matthew Hall <matthew.hall3@outlook.com>
78bbf76 to
81da4ec
Compare
The OMR branch that the tests used enables const refs (eclipse-omr/omr#8209), so the tests runs are valid. |
All constrefs-related opts have been merged into master, but not yet enabled. As far as I can tell, this change is independent of those opts being enabled. |
|
Jenkins test sanity xlinux jdk25 depends eclipse-omr/omr#8209 |
|
If the xlinux tests are clean, I will launch tests on the other platforms too. |
|
failures look related to #23814 |
|
Jenkins test sanity plinux,zlinux,alinux64,aix,win,xmac,amac jdk25 depends eclipse-omr/omr#8209 |
|
Jenkins test sanity.functional plinux,zlinux,alinux64,aix,win,xmac,amac jdk25 depends eclipse-omr/omr#8209 |
|
plinux fail is #24139 |
|
openjdk alinux, xmac and xlinux have a vectorapi assert captured here: #23814 |
|
Since the failures that were seen have been accounted for, this PR is ready to be merged. |
When calling
originalMH.invoke(args), MH.asType is called at runtime to ensure the arguments are compatible with the original MH. AMethodTypefor the arguments is constructed and checked against the original MethodHandle's MethodType. This is essentially subtype compatibility.When the types do not match exactly (but are compatible), the LambdaForm needs to be edited to perform typecasts before making the call to the target, resulting in a new MethodHandle. This can result in substantial runtime overhead.
Const refs allows us to create a reference to the resulting new MethodHandle when it is known to be constant at compile time, and to simply use that reference instead of having to call asType on each invocation of the MethodHandle.
This PR:
(for calls like
mh.invoke(args),mhis the original MethodHandle, and the result of theasTypecall is the target MethodHandle.)Type checking each type in the MT in not needed. If the MT of the asTypeCache MH is the same as the MT of the target MH, we can just use the cached MH. If they are not the same, then we cannot fold since we have no way of obtaining the asType result at compile time. (we also should not even get here if they are not the same, since we shouldn't have a known-object for the target MT in that case).