Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level (dotnet#107224)

There is no logical distinction between creating a `DomainAssembly` and an `Assembly` now. Making `DomainAssembly` create the `Assembly` as soon as it is constructed should make it so that we can switch assorted things that currently store/use `DomainAssembly` to `Assembly`, since they will be created at the same time.

Author: elinor-fung

src/coreclr/vm/appdomain.cpp

@@ -1035,7 +1035,7 @@ void SystemDomain::LoadBaseSystemClasses()
 
         // Only partially load the system assembly. Other parts of the code will want to access
         // the globals in this function before finishing the load.
-        m_pSystemAssembly = DefaultDomain()->LoadDomainAssembly(NULL, m_pSystemPEAssembly, FILE_LOAD_POST_ALLOCATE)->GetAssembly();
+        m_pSystemAssembly = DefaultDomain()->LoadDomainAssembly(NULL, m_pSystemPEAssembly, FILE_LOAD_BEFORE_TYPE_LOAD)->GetAssembly();
 
         // Set up binder for CoreLib
         CoreLibBinder::AttachModule(m_pSystemAssembly->GetModule());
@@ -2006,8 +2006,7 @@ static const char *fileLoadLevelName[] =
 {
     "CREATE",                             // FILE_LOAD_CREATE
     "BEGIN",                              // FILE_LOAD_BEGIN
-    "ALLOCATE",                           // FILE_LOAD_ALLOCATE
-    "POST_ALLOCATE",                      // FILE_LOAD_POST_ALLOCATE
+    "BEFORE_TYPE_LOAD",                   // FILE_LOAD_BEFORE_TYPE_LOAD
     "EAGER_FIXUPS",                       // FILE_LOAD_EAGER_FIXUPS
     "DELIVER_EVENTS",                     // FILE_LOAD_DELIVER_EVENTS
     "VTABLE FIXUPS",                      // FILE_LOAD_VTABLE_FIXUPS
@@ -2074,7 +2073,6 @@ BOOL FileLoadLock::CompleteLoadLevel(FileLoadLevel level, BOOL success)
 #ifndef DACCESS_COMPILE
         switch(level)
         {
-            case FILE_LOAD_ALLOCATE:
             case FILE_LOAD_DELIVER_EVENTS:
             case FILE_LOADED:
             case FILE_ACTIVE: // The timing of stress logs is not critical, so even for the FILE_ACTIVE stage we need not do it while the m_pList lock is held.
@@ -2494,7 +2492,9 @@ DomainAssembly *AppDomain::LoadDomainAssemblyInternal(AssemblySpec* pIdentity,
 
         // Allocate the DomainAssembly a bit early to avoid GC mode problems. We could potentially avoid
         // a rare redundant allocation by moving this closer to FileLoadLock::Create, but it's not worth it.
-        NewHolder<DomainAssembly> pDomainAssembly = new DomainAssembly(pPEAssembly, pLoaderAllocator);
+        AllocMemTracker amTracker;
+        AllocMemTracker *pamTracker = &amTracker;
+        NewHolder<DomainAssembly> pDomainAssembly = new DomainAssembly(pPEAssembly, pLoaderAllocator, pamTracker);
 
         LoadLockHolder lock(this);
 
@@ -2511,6 +2511,7 @@ DomainAssembly *AppDomain::LoadDomainAssemblyInternal(AssemblySpec* pIdentity,
                 registerNewAssembly = true;
                 fileLock = FileLoadLock::Create(lock, pPEAssembly, pDomainAssembly);
                 pDomainAssembly.SuppressRelease();
+                pamTracker->SuppressRelease();
                 if (pDomainAssembly->IsCollectible())
                 {
                     // We add the assembly to the LoaderAllocator only when we are sure that it can be added

src/coreclr/vm/assembly.cpp

@@ -436,6 +436,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar
     AppDomain* pDomain = ::GetAppDomain();
 
     NewHolder<DomainAssembly> pDomainAssembly;
+    Assembly* pAssem;
     BOOL                      createdNewAssemblyLoaderAllocator = FALSE;
 
     {
@@ -479,7 +480,9 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar
         }
 
         // Create a domain assembly
-        pDomainAssembly = new DomainAssembly(pPEAssembly, pLoaderAllocator);
+        pDomainAssembly = new DomainAssembly(pPEAssembly, pLoaderAllocator, pamTracker);
+        pAssem = pDomainAssembly->GetAssembly();
+        pAssem->m_isDynamic = true;
         if (pDomainAssembly->IsCollectible())
         {
             // We add the assembly to the LoaderAllocator only when we are sure that it can be added
@@ -489,66 +492,47 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar
     }
 
     // Start loading process
+    if (createdNewAssemblyLoaderAllocator)
     {
-        // Create a concrete assembly
-        // (!Do not remove scoping brace: order is important here: the Assembly holder must destruct before the AllocMemTracker!)
-        NewHolder<Assembly> pAssem;
+        GCX_PREEMP();
 
-        {
-            GCX_PREEMP();
-            // Assembly::Create will call SuppressRelease on the NewHolder that holds the LoaderAllocator when it transfers ownership
-            pAssem = Assembly::Create(pPEAssembly, pDomainAssembly->GetDebuggerInfoBits(), pLoaderAllocator->IsCollectible(), pamTracker, pLoaderAllocator);
+        // Initializing the virtual call stub manager is delayed to remove the need for the LoaderAllocator destructor to properly handle
+        // uninitializing the VSD system. (There is a need to suspend the runtime, and that's tricky)
+        pLoaderAllocator->InitVirtualCallStubManager();
+    }
 
-            ReflectionModule* pModule = (ReflectionModule*) pAssem->GetModule();
+    {
+        GCX_PREEMP();
 
-            if (createdNewAssemblyLoaderAllocator)
-            {
-                // Initializing the virtual call stub manager is delayed to remove the need for the LoaderAllocator destructor to properly handle
-                // uninitializing the VSD system. (There is a need to suspend the runtime, and that's tricky)
-                pLoaderAllocator->InitVirtualCallStubManager();
-            }
-        }
+        // Finish loading process
+        // <TODO> would be REALLY nice to unify this with main loading loop </TODO>
+        pDomainAssembly->Begin();
+        pDomainAssembly->DeliverSyncEvents();
+        pDomainAssembly->DeliverAsyncEvents();
+        pDomainAssembly->FinishLoad();
+        pDomainAssembly->ClearLoading();
+        pDomainAssembly->m_level = FILE_ACTIVE;
+    }
 
-        pAssem->m_isDynamic = true;
+    {
+        CANNOTTHROWCOMPLUSEXCEPTION();
+        FAULT_FORBID();
 
-        //we need to suppress release for pAssem to avoid double release
-        pAssem.SuppressRelease ();
+        // Cannot fail after this point
 
-        {
-            GCX_PREEMP();
-
-            // Finish loading process
-            // <TODO> would be REALLY nice to unify this with main loading loop </TODO>
-            pDomainAssembly->Begin();
-            pDomainAssembly->SetAssembly(pAssem);
-            pDomainAssembly->m_level = FILE_LOAD_ALLOCATE;
-            pDomainAssembly->DeliverSyncEvents();
-            pDomainAssembly->DeliverAsyncEvents();
-            pDomainAssembly->FinishLoad();
-            pDomainAssembly->ClearLoading();
-            pDomainAssembly->m_level = FILE_ACTIVE;
-        }
+        pDomainAssembly.SuppressRelease();
+        pamTracker->SuppressRelease();
 
+        // Once we reach this point, the loader allocator lifetime is controlled by the Assembly object.
+        if (createdNewAssemblyLoaderAllocator)
         {
-            CANNOTTHROWCOMPLUSEXCEPTION();
-            FAULT_FORBID();
-
-            //Cannot fail after this point
-
-            pDomainAssembly.SuppressRelease(); // This also effectively suppresses the release of the pAssem
-            pamTracker->SuppressRelease();
-
-            // Once we reach this point, the loader allocator lifetime is controlled by the Assembly object.
-            if (createdNewAssemblyLoaderAllocator)
-            {
-                // Atomically transfer ownership to the managed heap
-                pLoaderAllocator->ActivateManagedTracking();
-                pLoaderAllocator.SuppressRelease();
-            }
-
-            pAssem->SetIsTenured();
-            pRetVal = pAssem;
+            // Atomically transfer ownership to the managed heap
+            pLoaderAllocator->ActivateManagedTracking();
+            pLoaderAllocator.SuppressRelease();
         }
+
+        pAssem->SetIsTenured();
+        pRetVal = pAssem;
     }
 
     RETURN pRetVal;

src/coreclr/vm/ceeload.cpp

@@ -4473,24 +4473,6 @@ VOID Module::EnsureActive()
 
 #include <optdefault.h>
 
-
-#ifndef DACCESS_COMPILE
-
-VOID Module::EnsureAllocated()
-{
-    CONTRACTL
-    {
-        THROWS;
-        GC_TRIGGERS;
-        MODE_ANY;
-    }
-    CONTRACTL_END;
-
-    GetDomainAssembly()->EnsureAllocated();
-}
-
-#endif // !DACCESS_COMPILE
-
 CHECK Module::CheckActivated()
 {
     CONTRACTL

src/coreclr/vm/ceeload.h

@@ -948,7 +948,6 @@ class Module : public ModuleBase
 
 #ifndef DACCESS_COMPILE
     VOID EnsureActive();
-    VOID EnsureAllocated();
 #endif
 
     CHECK CheckActivated();

src/coreclr/vm/domainassembly.cpp

@@ -24,21 +24,21 @@
 #include "peimagelayout.inl"
 
 #ifndef DACCESS_COMPILE
-DomainAssembly::DomainAssembly(PEAssembly* pPEAssembly, LoaderAllocator* pLoaderAllocator) :
-    m_pAssembly(NULL),
-    m_pPEAssembly(pPEAssembly),
-    m_pModule(NULL),
-    m_fCollectible(pLoaderAllocator->IsCollectible()),
-    m_NextDomainAssemblyInSameALC(NULL),
-    m_pLoaderAllocator(pLoaderAllocator),
-    m_level(FILE_LOAD_CREATE),
-    m_loading(TRUE),
-    m_pError(NULL),
-    m_bDisableActivationCheck(FALSE),
-    m_fHostAssemblyPublished(FALSE),
-    m_debuggerFlags(DACF_NONE),
-    m_notifyflags(NOT_NOTIFIED),
-    m_fDebuggerUnloadStarted(FALSE)
+DomainAssembly::DomainAssembly(PEAssembly* pPEAssembly, LoaderAllocator* pLoaderAllocator, AllocMemTracker* memTracker)
+    : m_pAssembly(NULL)
+    , m_pPEAssembly(pPEAssembly)
+    , m_pModule(NULL)
+    , m_fCollectible(pLoaderAllocator->IsCollectible())
+    , m_NextDomainAssemblyInSameALC(NULL)
+    , m_pLoaderAllocator(pLoaderAllocator)
+    , m_level(FILE_LOAD_CREATE)
+    , m_loading(TRUE)
+    , m_pError(NULL)
+    , m_bDisableActivationCheck(FALSE)
+    , m_fHostAssemblyPublished(FALSE)
+    , m_debuggerFlags(DACF_NONE)
+    , m_notifyflags(NOT_NOTIFIED)
+    , m_fDebuggerUnloadStarted(FALSE)
 {
     CONTRACTL
     {
@@ -53,6 +53,18 @@ DomainAssembly::DomainAssembly(PEAssembly* pPEAssembly, LoaderAllocator* pLoader
     pPEAssembly->ValidateForExecution();
 
     SetupDebuggingConfig();
+
+    // Create the Assembly
+    NewHolder<Assembly> assembly = Assembly::Create(GetPEAssembly(), GetDebuggerInfoBits(), IsCollectible(), memTracker, IsCollectible() ? GetLoaderAllocator() : NULL);
+    assembly->SetIsTenured();
+
+    m_pAssembly = assembly.Extract();
+    m_pModule = m_pAssembly->GetModule();
+
+    m_pAssembly->SetDomainAssembly(this);
+
+    // Creating the Assembly should have ensured the PEAssembly is loaded
+    _ASSERT(GetPEAssembly()->IsLoaded());
 }
 
 DomainAssembly::~DomainAssembly()
@@ -319,12 +331,8 @@ BOOL DomainAssembly::DoIncrementalLoad(FileLoadLevel level)
         Begin();
         break;
 
-    case FILE_LOAD_ALLOCATE:
-        Allocate();
-        break;
-
-    case FILE_LOAD_POST_ALLOCATE:
-        PostAllocate();
+    case FILE_LOAD_BEFORE_TYPE_LOAD:
+        BeforeTypeLoad();
         break;
 
     case FILE_LOAD_EAGER_FIXUPS:
@@ -365,7 +373,7 @@ BOOL DomainAssembly::DoIncrementalLoad(FileLoadLevel level)
     return TRUE;
 }
 
-void DomainAssembly::PostAllocate()
+void DomainAssembly::BeforeTypeLoad()
 {
     CONTRACTL
     {
@@ -482,19 +490,6 @@ void DomainAssembly::Activate()
     RETURN;
 }
 
-void DomainAssembly::SetAssembly(Assembly* pAssembly)
-{
-    STANDARD_VM_CONTRACT;
-
-    _ASSERTE(pAssembly->GetModule()->GetPEAssembly()==m_pPEAssembly);
-    _ASSERTE(m_pAssembly == NULL);
-
-    m_pAssembly = pAssembly;
-    m_pModule = pAssembly->GetModule();
-
-    pAssembly->SetDomainAssembly(this);
-}
-
 void DomainAssembly::Begin()
 {
     STANDARD_VM_CONTRACT;
@@ -540,38 +535,6 @@ void DomainAssembly::UnregisterFromHostAssembly()
     }
 }
 
-void DomainAssembly::Allocate()
-{
-    CONTRACTL
-    {
-        INSTANCE_CHECK;
-        STANDARD_VM_CHECK;
-        INJECT_FAULT(COMPlusThrowOM(););
-        PRECONDITION(m_pAssembly == NULL);
-    }
-    CONTRACTL_END;
-
-    AllocMemTracker   amTracker;
-    AllocMemTracker * pamTracker = &amTracker;
-
-    Assembly * pAssembly;
-    {
-        // Order is important here - in the case of an exception, the Assembly holder must destruct before the AllocMemTracker declared above.
-        NewHolder<Assembly> assemblyHolder(NULL);
-
-        assemblyHolder = pAssembly = Assembly::Create(GetPEAssembly(), GetDebuggerInfoBits(), this->IsCollectible(), pamTracker, this->IsCollectible() ? this->GetLoaderAllocator() : NULL);
-        assemblyHolder->SetIsTenured();
-
-        pamTracker->SuppressRelease();
-        assemblyHolder.SuppressRelease();
-    }
-
-    SetAssembly(pAssembly);
-
-    // Creating the Assembly should have ensured the PEAssembly is loaded
-    _ASSERT(GetPEAssembly()->IsLoaded());
-}
-
 void DomainAssembly::DeliverAsyncEvents()
 {
     CONTRACTL

src/coreclr/vm/domainassembly.h

@@ -35,16 +35,14 @@ enum FileLoadLevel
 
     FILE_LOAD_CREATE,
     FILE_LOAD_BEGIN,
-    FILE_LOAD_ALLOCATE,
-    FILE_LOAD_POST_ALLOCATE,
+    FILE_LOAD_BEFORE_TYPE_LOAD,
     FILE_LOAD_EAGER_FIXUPS,
     FILE_LOAD_DELIVER_EVENTS,
     FILE_LOAD_VTABLE_FIXUPS,
     FILE_LOADED,                    // Loaded by not yet active
     FILE_ACTIVE                     // Fully active (constructors run & security checked)
 };
 
-
 enum NotificationStatus
 {
 	NOT_NOTIFIED=0,
@@ -186,13 +184,6 @@ class DomainAssembly final
         return EnsureLoadLevel(FILE_ACTIVE);
     }
 
-    // Ensure that an assembly has reached at least the Allocated state.  Throw if not.
-    void EnsureAllocated()
-    {
-        WRAPPER_NO_CONTRACT;
-        return EnsureLoadLevel(FILE_LOAD_ALLOCATE);
-    }
-
     // EnsureLoadLevel is a generic routine used to ensure that the file is not in a delay loaded
     // state (unless it needs to be.)  This should be used when a particular level of loading
     // is required for an operation.  Note that deadlocks are tolerated so the level may be one
@@ -260,16 +251,15 @@ class DomainAssembly final
     friend class Module;
     friend class FileLoadLock;
 
-    DomainAssembly(PEAssembly* pPEAssembly, LoaderAllocator* pLoaderAllocator);
+    DomainAssembly(PEAssembly* pPEAssembly, LoaderAllocator* pLoaderAllocator, AllocMemTracker* memTracker);
 
     BOOL DoIncrementalLoad(FileLoadLevel targetLevel);
     void ClearLoading() { LIMITED_METHOD_CONTRACT; m_loading = FALSE; }
     void SetLoadLevel(FileLoadLevel level) { LIMITED_METHOD_CONTRACT; m_level = level; }
 
 #ifndef DACCESS_COMPILE
     void Begin();
-    void Allocate();
-    void PostAllocate();
+    void BeforeTypeLoad();
     void EagerFixups();
     void VtableFixups();
     void DeliverSyncEvents();
@@ -283,7 +273,6 @@ class DomainAssembly final
 
     // This should be used to permanently set the load to fail. Do not use with transient conditions
     void SetError(Exception *ex);
-    void SetAssembly(Assembly* pAssembly);
 
     void SetProfilerNotified() { LIMITED_METHOD_CONTRACT; m_notifyflags|= PROFILER_NOTIFIED; }
     void SetDebuggerNotified() { LIMITED_METHOD_CONTRACT; m_notifyflags|=DEBUGGER_NOTIFIED; }