From 5aacb1dfa019eb4865800d05a408665be322ea77 Mon Sep 17 00:00:00 2001 From: David Mason Date: Tue, 19 Mar 2019 14:00:36 -0700 Subject: Reintroduce PR #22617 (Update added types and methoddefs on ApplyMetadata) (#23202) * Add ApplyMetadata changes back This reverts commit f9c10f995fe65c0e7c30aa1734f7bb22c519983d. * Fix race condition in loading available class hash for R2R with old R2R images or profiler modified R2R images --- .../Profiling/Profiler Breaking Changes.md | 6 ++ src/vm/ceeload.cpp | 66 ++++++++++++++++------ src/vm/ceeload.h | 7 +++ src/vm/clsload.cpp | 26 ++++++++- src/vm/proftoeeinterfaceimpl.cpp | 4 +- 5 files changed, 87 insertions(+), 22 deletions(-) create mode 100644 Documentation/Profiling/Profiler Breaking Changes.md diff --git a/Documentation/Profiling/Profiler Breaking Changes.md b/Documentation/Profiling/Profiler Breaking Changes.md new file mode 100644 index 0000000000..f63b22764e --- /dev/null +++ b/Documentation/Profiling/Profiler Breaking Changes.md @@ -0,0 +1,6 @@ +# Profiler Breaking Changes # + +Over time we will need to modify the Profiler APIs, this document will serve as a record of any breaking changes. + +1. Code Versioning introduced changes documented [here](../design-docs/code-versioning-profiler-breaking-changes.md) +2. The work to allow adding new types and methods after module load means ICorProfilerInfo7::ApplyMetadata will now potentially trigger a GC, and will not be callable in situations where a GC can not happen (for example ICorProfilerCallback::RootReferences). \ No newline at end of file diff --git a/src/vm/ceeload.cpp b/src/vm/ceeload.cpp index c0fad3c595..1fec2c262d 100644 --- a/src/vm/ceeload.cpp +++ b/src/vm/ceeload.cpp @@ -191,6 +191,35 @@ BOOL Module::SetTransientFlagInterlocked(DWORD dwFlag) } #if PROFILING_SUPPORTED +void Module::UpdateNewlyAddedTypes() +{ + CONTRACTL + { + THROWS; + GC_TRIGGERS; + INJECT_FAULT(COMPlusThrowOM();); + } + CONTRACTL_END + + DWORD countTypesAfterProfilerUpdate = GetMDImport()->GetCountWithTokenKind(mdtTypeDef); + DWORD countExportedTypesAfterProfilerUpdate = GetMDImport()->GetCountWithTokenKind(mdtExportedType); + + // typeDefs rids 0 and 1 aren't included in the count, thus X typeDefs before means rid X+1 was valid and our incremental addition should start at X+2 + for (DWORD typeDefRid = m_dwTypeCount + 2; typeDefRid < countTypesAfterProfilerUpdate + 2; typeDefRid++) + { + GetAssembly()->AddType(this, TokenFromRid(typeDefRid, mdtTypeDef)); + } + + // exportedType rid 0 isn't included in the count, thus X exportedTypes before means rid X was valid and our incremental addition should start at X+1 + for (DWORD exportedTypeDef = m_dwExportedTypeCount + 1; exportedTypeDef < countExportedTypesAfterProfilerUpdate + 1; exportedTypeDef++) + { + GetAssembly()->AddExportedType(TokenFromRid(exportedTypeDef, mdtExportedType)); + } + + m_dwTypeCount = countTypesAfterProfilerUpdate; + m_dwExportedTypeCount = countExportedTypesAfterProfilerUpdate; +} + void Module::NotifyProfilerLoadFinished(HRESULT hr) { CONTRACTL @@ -208,12 +237,10 @@ void Module::NotifyProfilerLoadFinished(HRESULT hr) if (SetTransientFlagInterlocked(IS_PROFILER_NOTIFIED)) { // Record how many types are already present - DWORD countTypesOrig = 0; - DWORD countExportedTypesOrig = 0; if (!IsResource()) { - countTypesOrig = GetMDImport()->GetCountWithTokenKind(mdtTypeDef); - countExportedTypesOrig = GetMDImport()->GetCountWithTokenKind(mdtExportedType); + m_dwTypeCount = GetMDImport()->GetCountWithTokenKind(mdtTypeDef); + m_dwExportedTypeCount = GetMDImport()->GetCountWithTokenKind(mdtExportedType); } // Notify the profiler, this may cause metadata to be updated @@ -236,18 +263,7 @@ void Module::NotifyProfilerLoadFinished(HRESULT hr) // assembly if (!IsResource()) { - DWORD countTypesAfterProfilerUpdate = GetMDImport()->GetCountWithTokenKind(mdtTypeDef); - DWORD countExportedTypesAfterProfilerUpdate = GetMDImport()->GetCountWithTokenKind(mdtExportedType); - // typeDefs rids 0 and 1 aren't included in the count, thus X typeDefs before means rid X+1 was valid and our incremental addition should start at X+2 - for (DWORD typeDefRid = countTypesOrig + 2; typeDefRid < countTypesAfterProfilerUpdate + 2; typeDefRid++) - { - GetAssembly()->AddType(this, TokenFromRid(typeDefRid, mdtTypeDef)); - } - // exportedType rid 0 isn't included in the count, thus X exportedTypes before means rid X was valid and our incremental addition should start at X+1 - for (DWORD exportedTypeDef = countExportedTypesOrig + 1; exportedTypeDef < countExportedTypesAfterProfilerUpdate + 1; exportedTypeDef++) - { - GetAssembly()->AddExportedType(TokenFromRid(exportedTypeDef, mdtExportedType)); - } + UpdateNewlyAddedTypes(); } { @@ -585,6 +601,11 @@ void Module::Initialize(AllocMemTracker *pamTracker, LPCWSTR szName) m_ModuleID = NULL; m_ModuleIndex.m_dwIndex = (SIZE_T)-1; + // These will be initialized in NotifyProfilerLoadFinished, set them to + // a safe initial value now. + m_dwTypeCount = 0; + m_dwExportedTypeCount = 0; + // Prepare statics that are known at module load time AllocateStatics(pamTracker); @@ -1187,7 +1208,7 @@ void Module::ApplyMetaData() CONTRACTL { THROWS; - GC_NOTRIGGER; + GC_TRIGGERS; MODE_ANY; } CONTRACTL_END; @@ -1197,6 +1218,13 @@ void Module::ApplyMetaData() HRESULT hr = S_OK; ULONG ulCount; +#if PROFILING_SUPPORTED + if (!IsResource()) + { + UpdateNewlyAddedTypes(); + } +#endif // PROFILING_SUPPORTED + // Ensure for TypeRef ulCount = GetMDImport()->GetCountWithTokenKind(mdtTypeRef) + 1; EnsureTypeRefCanBeStored(TokenFromRid(ulCount, mdtTypeRef)); @@ -1204,6 +1232,10 @@ void Module::ApplyMetaData() // Ensure for AssemblyRef ulCount = GetMDImport()->GetCountWithTokenKind(mdtAssemblyRef) + 1; EnsureAssemblyRefCanBeStored(TokenFromRid(ulCount, mdtAssemblyRef)); + + // Ensure for MethodDef + ulCount = GetMDImport()->GetCountWithTokenKind(mdtMethodDef) + 1; + EnsureMethodDefCanBeStored(TokenFromRid(ulCount, mdtMethodDef)); } // diff --git a/src/vm/ceeload.h b/src/vm/ceeload.h index c3a9609ba0..01d5ba880a 100644 --- a/src/vm/ceeload.h +++ b/src/vm/ceeload.h @@ -1627,6 +1627,11 @@ private: BOOL m_nativeImageProfiling; CORCOMPILE_METHOD_PROFILE_LIST *m_methodProfileList; +#if PROFILING_SUPPORTED_DATA + DWORD m_dwTypeCount; + DWORD m_dwExportedTypeCount; +#endif // PROFILING_SUPPORTED_DATA + #if defined(FEATURE_COMINTEROP) public: @@ -2526,6 +2531,8 @@ public: DEBUGGER_INFO_SHIFT_PRIV); } + void UpdateNewlyAddedTypes(); + #ifdef PROFILING_SUPPORTED BOOL IsProfilerNotified() {LIMITED_METHOD_CONTRACT; return (m_dwTransientFlags & IS_PROFILER_NOTIFIED) != 0; } void NotifyProfilerLoadFinished(HRESULT hr); diff --git a/src/vm/clsload.cpp b/src/vm/clsload.cpp index 4ffbb3fced..323e29ba60 100644 --- a/src/vm/clsload.cpp +++ b/src/vm/clsload.cpp @@ -840,7 +840,8 @@ void ClassLoader::GetClassValue(NameHandleTable nhTable, continue; #ifdef FEATURE_READYTORUN - if (nhTable == nhCaseSensitive && pCurrentClsModule->IsReadyToRun() && pCurrentClsModule->GetReadyToRunInfo()->HasHashtableOfTypes()) + if (nhTable == nhCaseSensitive && pCurrentClsModule->IsReadyToRun() && pCurrentClsModule->GetReadyToRunInfo()->HasHashtableOfTypes() && + pCurrentClsModule->GetAvailableClassHash() == NULL) { // For R2R modules, we only search the hashtable of token types stored in the module's image, and don't fallback // to searching m_pAvailableClasses or m_pAvailableClassesCaseIns (in fact, we don't even allocate them for R2R modules). @@ -1618,7 +1619,7 @@ BOOL ClassLoader::FindClassModuleThrowing( EEClassHashEntry_t * pBucket = foundEntry.GetClassHashBasedEntryValue(); - if (pBucket == NULL && needsToBuildHashtable) + if (pBucket == NULL) { AvailableClasses_LockHolder lh(this); @@ -1628,7 +1629,7 @@ BOOL ClassLoader::FindClassModuleThrowing( pBucket = foundEntry.GetClassHashBasedEntryValue(); #ifndef DACCESS_COMPILE - if ((pBucket == NULL) && (m_cUnhashedModules > 0)) + if (needsToBuildHashtable && (pBucket == NULL) && (m_cUnhashedModules > 0)) { _ASSERT(needsToBuildHashtable); @@ -1650,6 +1651,7 @@ BOOL ClassLoader::FindClassModuleThrowing( #endif } + // Same check as above, but this time we've checked with the lock so the table will be populated if (pBucket == NULL) { #if defined(_DEBUG_IMPL) && !defined(DACCESS_COMPILE) @@ -4184,6 +4186,15 @@ VOID ClassLoader::AddAvailableClassDontHaveLock(Module *pModule, #endif CrstHolder ch(&m_AvailableClassLock); + + // R2R pre-computes an export table and tries to avoid populating a class hash at runtime. However the profiler can + // still add new types on the fly by calling here. If that occurs we fallback to the slower path of creating the + // in memory hashtable as usual. + if (!pModule->IsResource() && pModule->GetAvailableClassHash() == NULL) + { + LazyPopulateCaseSensitiveHashTables(); + } + AddAvailableClassHaveLock( pModule, classdef, @@ -4380,6 +4391,15 @@ VOID ClassLoader::AddExportedTypeDontHaveLock(Module *pManifestModule, CONTRACTL_END CrstHolder ch(&m_AvailableClassLock); + + // R2R pre-computes an export table and tries to avoid populating a class hash at runtime. However the profiler can + // still add new types on the fly by calling here. If that occurs we fallback to the slower path of creating the + // in memory hashtable as usual. + if (!pManifestModule->IsResource() && pManifestModule->GetAvailableClassHash() == NULL) + { + LazyPopulateCaseSensitiveHashTables(); + } + AddExportedTypeHaveLock( pManifestModule, cl, diff --git a/src/vm/proftoeeinterfaceimpl.cpp b/src/vm/proftoeeinterfaceimpl.cpp index 854288847e..663b82f403 100644 --- a/src/vm/proftoeeinterfaceimpl.cpp +++ b/src/vm/proftoeeinterfaceimpl.cpp @@ -9661,12 +9661,12 @@ HRESULT ProfToEEInterfaceImpl::ApplyMetaData( CONTRACTL { NOTHROW; - GC_NOTRIGGER; + GC_TRIGGERS; MODE_ANY; } CONTRACTL_END; - PROFILER_TO_CLR_ENTRYPOINT_SYNC_EX(kP2EEAllowableAfterAttach, (LF_CORPROF, LL_INFO1000, "**PROF: ApplyMetaData.\n")); + PROFILER_TO_CLR_ENTRYPOINT_SYNC_EX(kP2EEAllowableAfterAttach | kP2EETriggers, (LF_CORPROF, LL_INFO1000, "**PROF: ApplyMetaData.\n")); if (moduleId == NULL) { -- cgit v1.2.3