Skip to content

Commit

Permalink
Make DomainAssembly create its Assembly in its constructor and re…
Browse files Browse the repository at this point in the history 
…move separate allocate load level (#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.
  • Loading branch information
@elinor-fung
elinor-fung committed Sep 4, 2024
1 parent f8e9dd1 commit ec2f534
Show file tree
Hide file tree
Showing 8 changed files with 77 additions and 166 deletions.
11 changes: 6 additions & 5 deletions src/coreclr/vm/appdomain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);

Expand All @@ -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
Expand Down
86 changes: 35 additions & 51 deletions src/coreclr/vm/assembly.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar
AppDomain* pDomain = ::GetAppDomain();

NewHolder<DomainAssembly> pDomainAssembly;
Assembly* pAssem;
BOOL createdNewAssemblyLoaderAllocator = FALSE;

{
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand Down
18 changes: 0 additions & 18 deletions src/coreclr/vm/ceeload.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/vm/ceeload.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,6 @@ class Module : public ModuleBase

#ifndef DACCESS_COMPILE
VOID EnsureActive();
VOID EnsureAllocated();
#endif

CHECK CheckActivated();
Expand Down
97 changes: 30 additions & 67 deletions src/coreclr/vm/domainassembly.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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()
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -365,7 +373,7 @@ BOOL DomainAssembly::DoIncrementalLoad(FileLoadLevel level)
return TRUE;
}

void DomainAssembly::PostAllocate()
void DomainAssembly::BeforeTypeLoad()
{
CONTRACTL
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
17 changes: 3 additions & 14 deletions src/coreclr/vm/domainassembly.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand All @@ -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; }
Expand Down
Loading

0 comments on commit ec2f534

Please sign in to comment.