From b212891e3a58fc4f5c229fb32def147c4ba0bd32 Mon Sep 17 00:00:00 2001 From: Devin Papineau Date: Tue, 8 Mar 2022 19:40:29 -0500 Subject: [PATCH 1/5] Disallow nop guards for interface call sites ...unless inliner has already proved that the receiver is an instance of a particular class that implements the expected interface. (However, it appears that it is currently difficult or maybe impossible to get such a bound despite tryToRefineReceiverClassBasedOnResolvedTypeArgInfo()). A passing vgnop-based interface guard can guarantee the receiver type is as expected by the inlined body, but only if we already know before the guard that the receiver implements the expected interface. Here, an interface type bound is insufficient to know that, because bytecode verification does not guarantee any particular receiver type at invokeinterface. As such, in the absence of a class (i.e. non-interface) type bound on the receiver, all targets must use profiled guard. Additionally, the profiled guard must ensure on the hot side that the receiver is an instance of the interface expected at this call site. There was one circumstance where it was possible to generate a profiled guard with method test for a method defined by a class that does not implement the interface. Inliner now uses a VFT test instead in this situation. Only hot compilations performed during startup are affected. It was also possible to generate an interface (nop) guard for a single implementer method defined by a class that does not implement the interface. Since this must now be a profiled guard, inliner will only accept single implementers defined by classes that do implement the interface. Others will be rejected in the hope that it will be possible to generate a VFT test based on profiling. Without these precautions, it would be possible for the JIT to inline a single implementing method with a nop guard, and for execution to flow into the inlined body with a receiver that is not an instance of the class expected by the inlined code. The inlined code could then access fields of the receiver as though it were of the expected type. --- runtime/compiler/optimizer/J9CallGraph.hpp | 3 +- runtime/compiler/optimizer/J9Inliner.cpp | 143 ++++++++++++++++++++- 2 files changed, 139 insertions(+), 7 deletions(-) diff --git a/runtime/compiler/optimizer/J9CallGraph.hpp b/runtime/compiler/optimizer/J9CallGraph.hpp index 6999fa87e27..04f2c2a2196 100644 --- a/runtime/compiler/optimizer/J9CallGraph.hpp +++ b/runtime/compiler/optimizer/J9CallGraph.hpp @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2021 IBM Corp. and others + * Copyright (c) 2000, 2022 IBM Corp. and others * * This program and the accompanying materials are made available under * the terms of the Eclipse Public License 2.0 which accompanies this @@ -87,6 +87,7 @@ class TR_J9InterfaceCallSite : public TR_ProfileableCallSite public: TR_CALLSITE_TR_ALLOC_AND_INHERIT_EMPTY_CONSTRUCTOR(TR_J9InterfaceCallSite, TR_ProfileableCallSite) virtual bool findCallSiteTarget (TR_CallStack *callStack, TR_InlinerBase* inliner); + bool findCallSiteTargetImpl (TR_CallStack *callStack, TR_InlinerBase* inliner); virtual TR_OpaqueClassBlock* getClassFromMethod (); virtual const char* name () { return "TR_J9InterfaceCallSite"; } protected: diff --git a/runtime/compiler/optimizer/J9Inliner.cpp b/runtime/compiler/optimizer/J9Inliner.cpp index 4195caa52a8..155ab85d48f 100644 --- a/runtime/compiler/optimizer/J9Inliner.cpp +++ b/runtime/compiler/optimizer/J9Inliner.cpp @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2021 IBM Corp. and others + * Copyright (c) 2000, 2022 IBM Corp. and others * * This program and the accompanying materials are made available under * the terms of the Eclipse Public License 2.0 which accompanies this @@ -673,6 +673,108 @@ static TR_ResolvedMethod * findSingleImplementer( */ bool TR_J9InterfaceCallSite::findCallSiteTarget (TR_CallStack *callStack, TR_InlinerBase* inliner) + { + bool result = findCallSiteTargetImpl(callStack, inliner); + + // A passing vgnop-based interface guard can guarantee the receiver type is + // as expected by the inlined body, but only if we already know before the + // guard that the receiver implements the expected interface. Here, an + // interface type bound is insufficient to know that, because bytecode + // verification does not guarantee any particular receiver type at + // invokeinterface. + // + // As such, in the absence of a class (i.e. non-interface) type bound on the + // receiver, all targets must use profiled guard. Additionally, the profiled + // guard must ensure on the hot side that the receiver is an instance of the + // interface expected at this call site. + // + // These requirements could be relaxed in the future by generating a type + // check against the interface type at the beginning of the inlined body + // (taking care to ensure that the exception successors are the same as + // those of the call, rather than those of the first block of the callee). + // + // ZEROCHK jitThrowIncompatibleClassChangeError + // instanceof + // + // loadaddr + // + // In particular, such a type check would allow the following cases: + // + // - A default method inlined using a nonoverridden guard. Currently, + // default methods are never inlined (at interface call sites) because + // TR_ResolvedJ9Method::getResolvedInterfaceMethod() does not return them. + // + // - A profiled guard with method test for a method defined by a class that + // does not implement the expected interface. Subtypes may still implement + // the interface. + // + // If/when we want to start inlining in one or both of these cases, the + // assertions below can be relaxed accordingly. However, testing instanceof + // against the interface will be expensive, so there would have to be a + // considerable benefit to the inlining to motivate such a change. + // + TR_OpaqueClassBlock *iface = getClassFromMethod(); + if (_receiverClass != NULL + && !TR::Compiler->cls.isInterfaceClass(comp(), _receiverClass)) + { + TR_ASSERT_FATAL( + fe()->isInstanceOf(_receiverClass, iface, true, true, true) == TR_yes, + "interface call site %p receiver type %p " + "does not implement the expected interface %p", + this, + _receiverClass, + iface); + + heuristicTrace( + inliner->tracer(), + "Interface call site %p has receiver class bound %p; nop guards ok", + this, + _receiverClass); + } + else + { + TR_Debug *debug = comp()->getDebug(); + for (int32_t i = 0; i < numTargets(); i++) + { + TR_CallTarget *tgt = getTarget(i); + TR_VirtualGuardKind kind = tgt->_guard->_kind; + TR_ASSERT_FATAL( + kind == TR_ProfiledGuard, + "interface call site %p requires profiled guard (kind %d), " + "but target %d [%p] uses %s (kind %d)", + this, + (int)TR_ProfiledGuard, + i, + tgt, + debug == NULL ? "" : debug->getVirtualGuardKindName(kind), + (int)kind); + + // Bound on the receiver types that pass the profiled guard + TR_OpaqueClassBlock *passClass = NULL; + TR_ResolvedMethod *callee = tgt->_calleeMethod; + if (tgt->_guard->_type == TR_VftTest) + passClass = tgt->_guard->_thisClass; + else + passClass = callee->containingClass(); + + TR_ASSERT_FATAL( + fe()->isInstanceOf(passClass, iface, true, true, true) == TR_yes, + "interface call site %p target %d [%p] (J9Method %p) " + "accepts receivers of type %p, " + "which does not implement the expected interface %p", + this, + i, + tgt, + callee->getPersistentIdentifier(), + passClass, + iface); + } + } + + return result; + } + +bool TR_J9InterfaceCallSite::findCallSiteTargetImpl(TR_CallStack *callStack, TR_InlinerBase* inliner) { static char *minimizedInlineJIT = feGetEnv("TR_JITInlineMinimized"); @@ -721,10 +823,30 @@ bool TR_J9InterfaceCallSite::findCallSiteTarget (TR_CallStack *callStack, TR_Inl if (calleeResolvedMethod && !calleeResolvedMethod->virtualMethodIsOverridden()) { - TR_VirtualGuardSelection * guard = new (comp()->trHeapMemory()) TR_VirtualGuardSelection(TR_InterfaceGuard, TR_MethodTest); - addTarget(comp()->trMemory(),inliner,guard,calleeResolvedMethod,_receiverClass,heapAlloc); - heuristicTrace(inliner->tracer(),"Call is an Interface with a Single Implementer guard %p\n", guard); - return true; + TR_VirtualGuardKind kind = TR_ProfiledGuard; + if (_receiverClass != NULL + && !TR::Compiler->cls.isInterfaceClass(comp(), _receiverClass)) + { + kind = TR_InterfaceGuard; + } + else + { + // Profiled guard with method test must test for a method defined by a + // class that implements the expected interface. See the comment in + // TR_J9InterfaceCallSite::findCallSiteTarget(). + TR_OpaqueClassBlock *defClass = calleeResolvedMethod->containingClass(); + TR_OpaqueClassBlock *iface = getClassFromMethod(); + if (fe()->isInstanceOf(defClass, iface, true, true, true) != TR_yes) + calleeResolvedMethod = NULL; // hope to get a VFT test from profiling + } + + if (calleeResolvedMethod != NULL) + { + TR_VirtualGuardSelection * guard = new (comp()->trHeapMemory()) TR_VirtualGuardSelection(kind, TR_MethodTest); + addTarget(comp()->trMemory(),inliner,guard,calleeResolvedMethod,_receiverClass,heapAlloc); + heuristicTrace(inliner->tracer(),"Call is an Interface with a Single Implementer guard %p\n", guard); + return true; + } } return findProfiledCallTargets(callStack, inliner); @@ -1040,7 +1162,16 @@ void TR_ProfileableCallSite::findSingleProfiledReceiver(ListIteratorcontainingClass(); + TR_OpaqueClassBlock *iface = getClassFromMethod(); + if (fe()->isInstanceOf(defClass, iface, true, true, true) != TR_yes) + preferMethodTest = false; + } TR_J9VMBase *fej9 = (TR_J9VMBase *)(comp()->fe()); // need to be able to store class chains for these methods From 16ebce00b02416a5239b62ba7ad66da59c192703 Mon Sep 17 00:00:00 2001 From: Devin Papineau Date: Mon, 6 Jun 2022 18:59:07 -0400 Subject: [PATCH 2/5] Correct interface call site assertion Apparently the class pointer in the profiled guard comes directly from the call site object, not from its virtual guard selection. --- runtime/compiler/optimizer/J9Inliner.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/compiler/optimizer/J9Inliner.cpp b/runtime/compiler/optimizer/J9Inliner.cpp index 155ab85d48f..5072630a5b9 100644 --- a/runtime/compiler/optimizer/J9Inliner.cpp +++ b/runtime/compiler/optimizer/J9Inliner.cpp @@ -753,7 +753,7 @@ bool TR_J9InterfaceCallSite::findCallSiteTarget (TR_CallStack *callStack, TR_Inl TR_OpaqueClassBlock *passClass = NULL; TR_ResolvedMethod *callee = tgt->_calleeMethod; if (tgt->_guard->_type == TR_VftTest) - passClass = tgt->_guard->_thisClass; + passClass = tgt->_receiverClass; else passClass = callee->containingClass(); From b6475510409208ee6885f87ba41633119744577e Mon Sep 17 00:00:00 2001 From: Devin Papineau Date: Mon, 6 Jun 2022 18:17:56 -0400 Subject: [PATCH 3/5] Heuristically consider VFT test for interface call profiled guards In particular, for the profiled guards generated instead of nop guards due to safety requirements in the case of a single implementer. VFT test excludes some receivers that could run the inlined method body, but it's a cheaper test at runtime. It's now preferred: - when the inlined method is defined by a final class, - when the inlined method is defined by a class that has not been extended (yet), - when profiling indicates that only a single receiver type occurs. --- runtime/compiler/optimizer/J9Inliner.cpp | 118 ++++++++++++++++++++++- 1 file changed, 113 insertions(+), 5 deletions(-) diff --git a/runtime/compiler/optimizer/J9Inliner.cpp b/runtime/compiler/optimizer/J9Inliner.cpp index 5072630a5b9..63dd336104c 100644 --- a/runtime/compiler/optimizer/J9Inliner.cpp +++ b/runtime/compiler/optimizer/J9Inliner.cpp @@ -824,26 +824,134 @@ bool TR_J9InterfaceCallSite::findCallSiteTargetImpl(TR_CallStack *callStack, TR_ if (calleeResolvedMethod && !calleeResolvedMethod->virtualMethodIsOverridden()) { TR_VirtualGuardKind kind = TR_ProfiledGuard; + TR_VirtualGuardTestType testType = TR_DummyTest; + TR_OpaqueClassBlock *thisClass = _receiverClass; if (_receiverClass != NULL && !TR::Compiler->cls.isInterfaceClass(comp(), _receiverClass)) { kind = TR_InterfaceGuard; + testType = TR_MethodTest; } else { - // Profiled guard with method test must test for a method defined by a - // class that implements the expected interface. See the comment in - // TR_J9InterfaceCallSite::findCallSiteTarget(). + // Whether to try to choose VFT test based on a non-extended defClass + // or based on profiling. This does not affect the final defClass + // heuristic because in that case we can be certain that the class + // won't be extended later. + bool useVftTestHeuristics = true; + TR::PersistentInfo *persistInfo = comp()->getPersistentInfo(); + if (persistInfo->getJitState() == STARTUP_STATE) + { + const static bool useVftTestHeuristicsDuringStartup = + feGetEnv("TR_useInterfaceVftTestHeuristicsDuringStartup") != NULL; + + useVftTestHeuristics = useVftTestHeuristicsDuringStartup; + } + + // Profiled guards must guarantee that passing receivers are instances + // of some class that implements the expected interface. See the + // comment in TR_J9InterfaceCallSite::findCallSiteTarget(). + // + // The choice of VFT test vs. method test is irrelevant here, as + // either one would accept instances of defClass. However, a VFT test + // obtained by consulting the profiled receiver types would work. + // TR_OpaqueClassBlock *defClass = calleeResolvedMethod->containingClass(); + thisClass = defClass; TR_OpaqueClassBlock *iface = getClassFromMethod(); if (fe()->isInstanceOf(defClass, iface, true, true, true) != TR_yes) + { calleeResolvedMethod = NULL; // hope to get a VFT test from profiling + } + // Heuristically choose between VFT test and method test. VFT test + // is cheaper, but method test can potentially allow the inlined + // body to be run for more receivers. + else if (TR::Compiler->cls.isClassFinal(comp(), defClass)) + { + testType = TR_VftTest; // method test will never help + } + else if (useVftTestHeuristics && !fe()->classHasBeenExtended(defClass)) + { + // Hope that defClass won't be extended in the future, or if it is, + // that its subtypes will override the inlined method anyway. + testType = TR_VftTest; + } + else + { + // There's already at least one subclass inheriting the single + // implementation. Choose method test because it covers the + // defining class and (so far) all of its subclasses. (If there + // were a subclass with its own override, then calleeResolvedMethod + // would be overridden.) + testType = TR_MethodTest; + + // Still consult the profiling though, since it might reveal that + // one type is overwhelmingly frequent at this call site. In that + // case, change back to VFT test. + TR_ValueProfileInfoManager *profMgr = + TR_ValueProfileInfoManager::get(comp()); + + TR_AddressInfo *valueInfo = NULL; + if (profMgr != NULL) + { + valueInfo = static_cast( + profMgr->getValueInfo(_bcInfo, comp(), AddressInfo)); + } + + if (useVftTestHeuristics + && valueInfo != NULL + && !comp()->getOption(TR_DisableProfiledInlining)) + { + TR_ScratchList byFreqDesc(comp()->trMemory()); + valueInfo->getSortedList(comp(), &byFreqDesc); + ListIterator it(&byFreqDesc); + uint32_t remainingTotalFreq = valueInfo->getTotalFrequency(); + TR_OpaqueClassBlock *topProfiledClass = NULL; + uint32_t topProfiledFreq = 0; + TR_ExtraAddressInfo *cur = it.getFirst(); + for (; cur != NULL; cur = it.getNext()) + { + auto *curClass = + reinterpret_cast(cur->_value); + + if (persistInfo->isObsoleteClass(curClass, comp()->fe()) + || fe()->isInstanceOf(curClass, iface, true, true, true) != TR_yes) + { + remainingTotalFreq -= cur->_frequency; + } + else if (topProfiledClass == NULL) + { + topProfiledClass = curClass; + topProfiledFreq = cur->_frequency; + } + } + + if (topProfiledClass != NULL + && remainingTotalFreq >= 32 + && topProfiledFreq == remainingTotalFreq) + { + testType = TR_VftTest; + thisClass = topProfiledClass; + } + } + } } if (calleeResolvedMethod != NULL) { - TR_VirtualGuardSelection * guard = new (comp()->trHeapMemory()) TR_VirtualGuardSelection(kind, TR_MethodTest); - addTarget(comp()->trMemory(),inliner,guard,calleeResolvedMethod,_receiverClass,heapAlloc); + TR_ASSERT_FATAL(testType != TR_DummyTest, "failed to select a guard test type"); + TR_VirtualGuardSelection *guard = + new (comp()->trHeapMemory()) TR_VirtualGuardSelection( + kind, testType, thisClass); + + addTarget( + comp()->trMemory(), + inliner, + guard, + calleeResolvedMethod, + thisClass, + heapAlloc); + heuristicTrace(inliner->tracer(),"Call is an Interface with a Single Implementer guard %p\n", guard); return true; } From a628c7796b8b9d9721ecdbd88c0ecfaa35690d87 Mon Sep 17 00:00:00 2001 From: Devin Papineau Date: Thu, 9 Jun 2022 19:42:48 -0400 Subject: [PATCH 4/5] Be more confident about single interface implementer profiled guards These guards used to be nop guards. They no longer are, but only because certain kinds of bytecode are technically allowed to be loaded. Bytecode that is problematic in this way should be exceedingly rare though, so treat these guards more like the nop guards we used to have by marking the taken side cold and by making sure to allow privatized argument rematerialization. --- runtime/compiler/optimizer/InlinerTempForJ9.cpp | 10 +++++++--- runtime/compiler/optimizer/J9Inliner.cpp | 14 ++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/runtime/compiler/optimizer/InlinerTempForJ9.cpp b/runtime/compiler/optimizer/InlinerTempForJ9.cpp index 8934b0c7365..3699e3629d1 100644 --- a/runtime/compiler/optimizer/InlinerTempForJ9.cpp +++ b/runtime/compiler/optimizer/InlinerTempForJ9.cpp @@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2021 IBM Corp. and others + * Copyright (c) 2000, 2022 IBM Corp. and others * * This program and the accompanying materials are made available under * the terms of the Eclipse Public License 2.0 which accompanies this @@ -2847,7 +2847,7 @@ TR_MultipleCallTargetInliner::eliminateTailRecursion( backEdge = TR::CFGEdge::createEdge(gotoBlock, branchDestination, trMemory()); callerCFG->addEdge(backEdge); callerCFG->removeEdge(origEdge); - if (guard->_kind == TR_ProfiledGuard) + if (guard->_kind == TR_ProfiledGuard && !guard->_forceTakenSideCold) { if (block->getFrequency() < 0) block2->setFrequency(block->getFrequency()); @@ -6784,7 +6784,11 @@ TR_J9InlinerPolicy::suitableForRemat(TR::Compilation *comp, TR::Node *callNode, bool suitableForRemat = true; TR_AddressInfo *valueInfo = static_cast(TR_ValueProfileInfoManager::getProfiledValueInfo(callNode, comp, AddressInfo)); - if (guard->isHighProbablityProfiledGuard()) + if (guard->_forceTakenSideCold) + { + // remat ok + } + else if (guard->isHighProbablityProfiledGuard()) { if (comp->getMethodHotness() <= warm && comp->getPersistentInfo()->getJitState() == STARTUP_STATE) { diff --git a/runtime/compiler/optimizer/J9Inliner.cpp b/runtime/compiler/optimizer/J9Inliner.cpp index 63dd336104c..787c23c397d 100644 --- a/runtime/compiler/optimizer/J9Inliner.cpp +++ b/runtime/compiler/optimizer/J9Inliner.cpp @@ -944,6 +944,20 @@ bool TR_J9InterfaceCallSite::findCallSiteTargetImpl(TR_CallStack *callStack, TR_ new (comp()->trHeapMemory()) TR_VirtualGuardSelection( kind, testType, thisClass); + if (kind == TR_ProfiledGuard) + { + // Almost all bytecode would pass type checking even including + // interface types. So even though this can't be a nop guard + // (because verification doesn't check interface types), treat it + // as much like a nop guard as possible. In particular, this will + // ensure that the block containing the cold call is actually + // marked cold, and ensure that priv. arg remat is allowed. + guard->_forceTakenSideCold = true; + + // It is still a high-probability profiled guard... + guard->setIsHighProbablityProfiledGuard(); + } + addTarget( comp()->trMemory(), inliner, From 6dbf777ff3e147208b76ce40cecb75c529a34c4c Mon Sep 17 00:00:00 2001 From: Devin Papineau Date: Tue, 20 Sep 2022 13:38:32 -0400 Subject: [PATCH 5/5] Base interface inlining determination directly on VM startup phase The JIT can re-enter STARTUP_STATE after leaving it, and it can remain in that state for much longer than one might naively expect, inhibiting these heuristics. --- runtime/compiler/optimizer/J9Inliner.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runtime/compiler/optimizer/J9Inliner.cpp b/runtime/compiler/optimizer/J9Inliner.cpp index 787c23c397d..3a743d30abd 100644 --- a/runtime/compiler/optimizer/J9Inliner.cpp +++ b/runtime/compiler/optimizer/J9Inliner.cpp @@ -840,7 +840,7 @@ bool TR_J9InterfaceCallSite::findCallSiteTargetImpl(TR_CallStack *callStack, TR_ // won't be extended later. bool useVftTestHeuristics = true; TR::PersistentInfo *persistInfo = comp()->getPersistentInfo(); - if (persistInfo->getJitState() == STARTUP_STATE) + if (TR::Compiler->vm.isVMInStartupPhase(comp()->fej9()->getJ9JITConfig())) { const static bool useVftTestHeuristicsDuringStartup = feGetEnv("TR_useInterfaceVftTestHeuristicsDuringStartup") != NULL;