From 5a349581ec246f77d17d599bf8f56c4796b248ff Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 21 Jul 2026 16:17:00 -0700 Subject: [PATCH] Make MethodDataObject::FillEntryDataForAncestor lazy per-level PR #130151 optimized FillEntryDataForAncestor to pre-resolve virtual slots whose final vtable entry is NULL (never overridden or called), which cannot be MethodImpl targets, so they are safe to fill even when a MethodImpl exists in the hierarchy. However, producing the null-slot bitmap was done on the initial MethodTable (m_pDeclMT), and to fill those slots across the hierarchy it added an eager loop that walked the entire parent chain on the first PopulateNextLevel call, defeating the class's lazy per-level design and adding tail latency. Since MethodDataObject retains a pointer to the initial MethodTable, the null slots for the entries introduced at any level can be computed on demand from m_pDeclMT's canonical MethodTable. Compute unprocessedNonOverriddenSlots per level instead: for each introduced virtual slot, when a MethodImpl exists at or below the current level (m_containsMethodImpl is monotonic across the upward walk), fill it only if its final vtable entry is NULL. This removes the eager full-chain walk, the null-slot bitvector, and its heap allocation, and restores one IntroducedMethodIterator per level while producing identical Decl/Impl data. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d6eeba00-418e-42e7-8e32-762bc1cedeb6 --- src/coreclr/vm/methodtable.cpp | 203 ++++++--------------------------- 1 file changed, 36 insertions(+), 167 deletions(-) diff --git a/src/coreclr/vm/methodtable.cpp b/src/coreclr/vm/methodtable.cpp index 652729a8755543..7059366bdd58ce 100644 --- a/src/coreclr/vm/methodtable.cpp +++ b/src/coreclr/vm/methodtable.cpp @@ -771,36 +771,6 @@ void MethodTable::InitializeExtraInterfaceInfo(PVOID pInfo) #define SELECT_TADDR_BIT(_index) (1U << (_index)) #endif -static inline unsigned GetBitVectorByteCount(unsigned bitCount) -{ - LIMITED_METHOD_CONTRACT; - return (bitCount + 7u) / 8u; -} - -static inline unsigned GetBitVectorBitCount(unsigned byteCount) -{ - LIMITED_METHOD_CONTRACT; - return byteCount * 8u; -} - -static inline bool IsBitVectorEntrySet(const uint8_t* pNullSlotBitvector, unsigned index) -{ - LIMITED_METHOD_CONTRACT; - return (pNullSlotBitvector[index / 8u] & static_cast(1u << (index % 8u))) != 0; -} - -static inline void SetBitVectorEntry(uint8_t* pNullSlotBitvector, unsigned index) -{ - LIMITED_METHOD_CONTRACT; - pNullSlotBitvector[index / 8u] |= static_cast(1u << (index % 8u)); -} - -static inline void ClearBitVectorEntry(uint8_t* pNullSlotBitvector, unsigned index) -{ - LIMITED_METHOD_CONTRACT; - pNullSlotBitvector[index / 8u] &= static_cast(~(1u << (index % 8u))); -} - //========================================================================================== // For the given interface in the map (specified via map index) mark the interface as declared explicitly on // this class. This is not legal for dynamically added interfaces (as used by RCWs). @@ -6744,83 +6714,33 @@ void MethodTable::MethodDataObject::FillEntryDataForAncestor(MethodTable * pMT) // we fill have been introduced/overridden by a subclass and so take // precedence over any inherited methodImpl. - // Before we fill the entry data, find if the current ancestor has any methodImpls + // Before we fill the entry data, find if the current ancestor has any methodImpls. + // Because we walk from the most-derived type upwards, m_containsMethodImpl is monotonic + // and, at any level, records whether a methodImpl exists between m_pDeclMT and the current + // level (inclusive). Those are exactly the methodImpls that can redirect the slots introduced + // at the current level, so it is the correct signal for the conservative behavior below. if (pMT->GetClass()->ContainsMethodImpls()) m_containsMethodImpl = TRUE; - if (m_containsMethodImpl && pMT != m_pDeclMT) - return; + // When there is a methodImpl in the inheritance chain we conservatively avoid pre-resolving + // virtual slots, since methodImpls may produce complex virtual slot behavior. The one exception + // is virtual slots whose final vtable entry is NULL: such a method has never been overridden by a + // subclass or called, so it cannot be the target of a methodImpl and can always be pre-resolved. + // + // Pre-resolving these NULL slots is important for performance, since otherwise the MethodImpl + // fallback case ends up using MethodTable::GetMethodDescForSlot_NoThrow to get the MethodDesc for + // the slot, and that is O(N) for the number of methods defined in the type hierarchy. The usage of + // MethodDataObject tends to be O(V) for the number of virtual method slots on the type, so we get + // O(V*N) processing time when this optimization fails. + // + // The MethodDataObject always retains a pointer to the most-derived type (m_pDeclMT), so we can + // determine NULL-ness of a slot from its canonical MethodTable at any level in the hierarchy, and + // therefore only need to walk a single IntroducedMethodIterator per level. + bool onlyFillNullSlots = m_containsMethodImpl; + MethodTable *pDeclCanonMT = onlyFillNullSlots ? m_pDeclMT->GetCanonicalMethodTable() : NULL; - NewArrayHolder pSlotFlags; - uint8_t nonAllocatedBitvector[16]; unsigned nVirtuals = pMT->GetNumVirtuals(); - unsigned unprocessedNonOverriddenSlots = 0; - - uint8_t* pNullSlotBitvector = NULL; - - if (pMT == m_pDeclMT) - { - // We have a concept of methods which have never been overridden by a subclass or called, and in that case we don't actually - // need to fill in the MethodTable's vtable entry with a stub. We can detect that here on the Canonical methodtable, - // and setup the Decl/Impl MethodDescs for a slot even if there are MethodImpls in the inheritance chain, since the - // vtable entry will only be NULL if the method could not have been involved with a MethodImpl. This optimization - // is only really important for the scenario where we have these NULL entries, since the MethodImpl fallback - // case will end up using MethodTable::GetMethodDescForSlot_NoThrow to get the MethodDesc for the slot, and that is O(N) - // for the number of Method defined in the type hierarchy, and the usage of MethodDataObject tends to be O(V) for the number - // of virtual method slots on the type, so we get O(V*N) processing time when the not MethodImpl optimization case - // fails, which needs addressing. - - // This optimization is only an improvement, if there is a type which is containsMethodImpl in the hierarchy. - bool containsMethodImplInHierarchy = m_containsMethodImpl; - MethodTable *pMTWalk = pMT; - while (!containsMethodImplInHierarchy && pMTWalk != NULL) - { - containsMethodImplInHierarchy = pMTWalk->GetClass()->ContainsMethodImpls(); - pMTWalk = pMTWalk->GetParentMethodTable(); - } - - if (containsMethodImplInHierarchy) - { - MethodTable *pCanonMT = pMT->GetCanonicalMethodTable(); - - if (nVirtuals <= GetBitVectorBitCount(sizeof(nonAllocatedBitvector))) - { - pNullSlotBitvector = nonAllocatedBitvector; - memset(pNullSlotBitvector, 0, sizeof(nonAllocatedBitvector)); - } - else - { - // Use a non-throwing allocation to keep this method within its NOTHROW contract. - // If the allocation fails, pNullSlotBitvector remains NULL and we simply fall back to the - // conservative (correct, but potentially slower) behavior below. - pSlotFlags = new (nothrow) uint8_t[GetBitVectorByteCount(nVirtuals)]; - pNullSlotBitvector = pSlotFlags; - if (pNullSlotBitvector != NULL) - memset(pNullSlotBitvector, 0, GetBitVectorByteCount(nVirtuals) * sizeof(*pNullSlotBitvector)); - } - - if (pNullSlotBitvector != NULL) - { - for (unsigned slot = 0; slot < nVirtuals; slot++) - { - PCODE pCode = pCanonMT->GetSlotForVirtualVolatileLoadWithoutBarrier(slot); - - if (pCode == (PCODE)NULL) - { - SetBitVectorEntry(pNullSlotBitvector, slot); - unprocessedNonOverriddenSlots += 1; - } - } - - if (unprocessedNonOverriddenSlots == 0) - { - pNullSlotBitvector = NULL; - } - } - } - } - unsigned nVTableLikeSlots = pMT->GetCanonicalMethodTable()->GetNumVtableSlots(); MethodTable::IntroducedMethodIterator it(pMT, FALSE); @@ -6834,38 +6754,25 @@ void MethodTable::MethodDataObject::FillEntryDataForAncestor(MethodTable * pMT) // We want to fill all methods introduced by the actual type we're gathering // data for, and the virtual methods of the parent and above unless there are MethodImpls - // in the hierarchy, in which cases we can only fill in entries which are known not to + // in the hierarchy, in which cases we can only fill in entries which are known not to // participate in complex virtual behavior. - if (pMT == m_pDeclMT) + if (slot < nVirtuals) { - if (slot < nVirtuals) + if (onlyFillNullSlots) { - if (pNullSlotBitvector == NULL || !IsBitVectorEntrySet(pNullSlotBitvector, slot)) - { - // We reach here if we've disabled the null-slot optimization (due to an allocation failure), - // or if the slot is not a null slot. This indicates that there is an implementation for the slot, and so - // the GetMethodDescForSlot_NoThrow API will operate in O(1) time (or we're in the allocation-failure case - // and the performance implications are unimportant). - if (m_containsMethodImpl) - { - // If there is a MethodImpl in the hierarchy, then we will need to skip the optimization of pre-resolving - // MethodDesc resolution for this slot as MethodImpls may have caused complex virtual slot behavior. - continue; - } - - // Fall through to SetEntryDataForSlotIfNotYetSet logic below - } - else - { - // We reach here if the slot is a NullSlot which indicates that no complex virtual slot behavior is associated - // with this slot, and that the method has never been called. Thus we should run the SetEntryDataForSlotIfNotYetSet logic. - _ASSERTE(unprocessedNonOverriddenSlots > 0); - ClearBitVectorEntry(pNullSlotBitvector, slot); - unprocessedNonOverriddenSlots -= 1; - - // Fall through to SetEntryDataForSlotIfNotYetSet logic below - } + // There is a methodImpl in the hierarchy, so only pre-resolve this virtual slot if its + // final vtable entry is NULL, which guarantees it could not have participated in complex + // virtual slot behavior. Any non-NULL slot is skipped, and its MethodDesc will instead be + // resolved on demand via GetMethodDescForSlot_NoThrow. + if (pDeclCanonMT->GetSlotForVirtualVolatileLoadWithoutBarrier(slot) != (PCODE)NULL) + continue; } + } + else + { + // Non-virtual methods are only meaningful for the most-derived type we're gathering data for. + if (pMT != m_pDeclMT) + continue; // Filter out SetEntryDataForSlotIfNotYetSet calls for non-virtual methods when requested if (m_virtualsOnly && slot >= nVTableLikeSlots) @@ -6874,47 +6781,9 @@ void MethodTable::MethodDataObject::FillEntryDataForAncestor(MethodTable * pMT) continue; } } - else - { - if (slot >= nVirtuals) - continue; - } SetEntryDataForSlotIfNotYetSet(slot, pMD); } - - // Walk the parent chain until we've processed all of the slots that have not been overridden or called by a subclass. - // NOTE: We only run this code for the top-level FillEntryDataForAncestor case where (pMT == m_pDeclMT). - // This check is enforced by unprocessedNonOverriddenSlots only being non-zero in that scenario. - while (unprocessedNonOverriddenSlots > 0) - { - _ASSERTE(pNullSlotBitvector != NULL); - pMT = pMT->GetParentMethodTable(); - _ASSERTE(pMT != NULL); - nVirtuals = pMT->GetNumVirtuals(); - MethodTable::IntroducedMethodIterator it(pMT, FALSE); - for (; it.IsValid(); it.Next()) - { - MethodDesc * pMD = it.GetMethodDesc(); - - unsigned slot = pMD->GetSlot(); - if (slot == MethodTable::NO_SLOT) - continue; - - if (slot >= nVirtuals) - continue; - - if (!IsBitVectorEntrySet(pNullSlotBitvector, slot)) - { - continue; - } - _ASSERTE(unprocessedNonOverriddenSlots > 0); - ClearBitVectorEntry(pNullSlotBitvector, slot); - unprocessedNonOverriddenSlots -= 1; - - SetEntryDataForSlotIfNotYetSet(slot, pMD); - } - } } // MethodTable::MethodDataObject::FillEntryDataForAncestor //==========================================================================================