Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

[WIP] Add support for collectible assemblies via AssemblyLoadContext #1684

Closed
wants to merge 17 commits into from
Closed

[WIP] Add support for collectible assemblies via AssemblyLoadContext #1684

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ececae0
Some progress on collectible AssemblyLoadContexts
Rohansi Oct 6, 2015
4d79cee
Fix assert in LoaderAllocator GC when used with multiple assemblies
Rohansi Oct 6, 2015
dc7c062
Workaround ambiguous static_cast compile error
Rohansi Oct 6, 2015
57b2f1e
mscorlib changes used to debug collectible AssemblyLoadContext
Rohansi Oct 6, 2015
095eb48
Cleanup the native part of AssemblyLoadContext
Rohansi Oct 8, 2015
748b05f
Replaced the root assembly with a set of LoaderAllocators
Rohansi Oct 8, 2015
e3ac590
Remove some assembly references from AppDomain when unloading
Rohansi Oct 9, 2015
264f969
Fixes some issues with the current changes
Rohansi Oct 10, 2015
d35f50a
Fix memory leak when unloading, fix DefineDynamicAssembly unload crash
Rohansi Oct 10, 2015
7885ede
Fix memory leak in AppDomain::Exit, remove unused field
Rohansi Oct 12, 2015
d0e1a88
Fix big memory leak in release build
Rohansi Oct 12, 2015
0d93f35
Fix crash when unloading assemblies that have not been fully loaded
Rohansi Oct 13, 2015
0564c73
Fix another memory leak
Rohansi Oct 15, 2015
801f888
Revert previous commit, use something more reliable
Rohansi Oct 16, 2015
c875ca5
Fix Clang build
Rohansi Oct 16, 2015
9b258b2
Fix string literals being deleted too early, use a proper Crst type
Rohansi Oct 17, 2015
aeb85b7
Responding to some feedback from @gkhanna79
Rohansi Oct 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,7 @@ add_definitions(-DFEATURE_EXCEPTIONDISPATCHINFO)
add_definitions(-DFEATURE_FRAMEWORK_INTERNAL)
add_definitions(-DFEATURE_HIJACK)
add_definitions(-DFEATURE_HOST_ASSEMBLY_RESOLVER)
add_definitions(-DFEATURE_COLLECTIBLE_ALC)
add_definitions(-DFEATURE_HOSTED_BINDER)
if(WIN32)
add_definitions(-DFEATURE_ISOSTORE)
Expand Down
1 change: 1 addition & 0 deletions clr.coreclr.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
<FeatureCominteropWinRTManagedActivation>true</FeatureCominteropWinRTManagedActivation>
<FeatureCrypto>true</FeatureCrypto>
<FeatureHostAssemblyResolver>true</FeatureHostAssemblyResolver>
<FeatureCollectibleALC>true</FeatureCollectibleALC>
<FeatureLazyCOWPages Condition="('$(TargetArch)' == 'i386') or ('$(TargetArch)' == 'arm')">true</FeatureLazyCOWPages>
<FeatureLegacyNetCFCrypto>true</FeatureLegacyNetCFCrypto>
<FeatureLatin1>true</FeatureLatin1>
Expand Down
2 changes: 2 additions & 0 deletions clr.defines.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
<CDefines Condition="'$(FeatureFusion)' == 'true'">$(CDefines);FEATURE_FUSION</CDefines>
<CDefines Condition="'$(FeatureHijack)' == 'true'">$(CDefines);FEATURE_HIJACK</CDefines>
<CDefines Condition="'$(FeatureHostAssemblyResolver)' == 'true'">$(CDefines);FEATURE_HOST_ASSEMBLY_RESOLVER</CDefines>
<CDefines Condition="'$(FeatureCollectibleALC)' == 'true'">$(CDefines);FEATURE_COLLECTIBLE_ALC</CDefines>
<CDefines Condition="'$(FeatureHostedBinder)' == 'true'">$(CDefines);FEATURE_HOSTED_BINDER</CDefines>
<CDefines Condition="'$(FeatureImpersonation)' == 'true'">$(CDefines);FEATURE_IMPERSONATION</CDefines>
<CDefines Condition="'$(FeatureIncludeAllInterfaces)' == 'true'">$(CDefines);FEATURE_INCLUDE_ALL_INTERFACES</CDefines>
Expand Down Expand Up @@ -166,6 +167,7 @@
<DefineConstants Condition="'$(FeatureFrameworkInternal)' == 'true'">$(DefineConstants);FEATURE_FRAMEWORK_INTERNAL</DefineConstants>
<DefineConstants Condition="'$(FeatureFusion)' == 'true'">$(DefineConstants);FEATURE_FUSION</DefineConstants>
<DefineConstants Condition="'$(FeatureHostAssemblyResolver)' == 'true'">$(DefineConstants);FEATURE_HOST_ASSEMBLY_RESOLVER</DefineConstants>
<DefineConstants Condition="'$(FeatureCollectibleALC)' == 'true'">$(DefineConstants);FEATURE_COLLECTIBLE_ALC</DefineConstants>
<DefineConstants Condition="'$(FeatureHostedBinder)' == 'true'">$(DefineConstants);FEATURE_HOSTED_BINDER</DefineConstants>
<DefineConstants Condition="'$(FeatureHosting)' == 'true'">$(DefineConstants);FEATURE_HOSTING</DefineConstants>
<DefineConstants Condition="'$(FeatureIdentityReference)' == 'true'">$(DefineConstants);FEATURE_IDENTITY_REFERENCE</DefineConstants>
Expand Down
108 changes: 105 additions & 3 deletions src/binder/clrprivbinderassemblyloadcontext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,16 +220,43 @@ HRESULT CLRPrivBinderAssemblyLoadContext::FindAssemblyBySpec(
return E_FAIL;
}

#ifdef FEATURE_COLLECTIBLE_ALC

HRESULT CLRPrivBinderAssemblyLoadContext::GetIsCollectible(BOOL *pIsCollectible)
{
*pIsCollectible = m_isCollectible;
return S_OK;
}

HRESULT CLRPrivBinderAssemblyLoadContext::ReferenceLoaderAllocator(LoaderAllocator *pLoaderAllocator)
{
CrstHolder holder(m_loadersCrst);

// The same LoaderAllocator should not be used twice
_ASSERTE(m_loaderAllocators.Lookup(pLoaderAllocator) == NULL);

VERIFY(pLoaderAllocator->AddReferenceIfAlive());

m_loaderAllocators.Add(pLoaderAllocator);

return S_OK;
}

#endif // FEATURE_COLLECTIBLE_ALC

//=============================================================================
// Creates an instance of the AssemblyLoadContext Binder
//
// This method does not take a lock since it is invoked from the ctor of the
// managed AssemblyLoadContext type.
//=============================================================================
/* static */
HRESULT CLRPrivBinderAssemblyLoadContext::SetupContext(DWORD dwAppDomainId,
HRESULT CLRPrivBinderAssemblyLoadContext::SetupContext(AppDomain *pAppDomain,
CLRPrivBinderCoreCLR *pTPABinder,
UINT_PTR ptrAssemblyLoadContext,
UINT_PTR ptrAssemblyLoadContext,
#ifdef FEATURE_COLLECTIBLE_ALC
BOOL fIsCollectible,
#endif // FEATURE_COLLECTIBLE_ALC
CLRPrivBinderAssemblyLoadContext **ppBindContext)
{
HRESULT hr = E_FAIL;
Expand All @@ -244,7 +271,7 @@ HRESULT CLRPrivBinderAssemblyLoadContext::SetupContext(DWORD dwAppDomainId,
if(SUCCEEDED(hr))
{
// Save the reference to the AppDomain in which the binder lives
pBinder->m_appContext.SetAppDomainId(dwAppDomainId);
pBinder->m_appContext.SetAppDomainId(pAppDomain->GetId().m_dwId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason we need to pass AppDomain* to get the ID, instead of just passing the ID?


// Mark that this binder can explicitly bind to native images
pBinder->m_appContext.SetExplicitBindToNativeImages(true);
Expand All @@ -257,6 +284,10 @@ HRESULT CLRPrivBinderAssemblyLoadContext::SetupContext(DWORD dwAppDomainId,
// AssemblyLoadContext instance
pBinder->m_ptrManagedAssemblyLoadContext = ptrAssemblyLoadContext;

#ifdef FEATURE_COLLECTIBLE_ALC
pBinder->m_isCollectible = fIsCollectible;
#endif // FEATURE_COLLECTIBLE_ALC

// Return reference to the allocated Binder instance
*ppBindContext = clr::SafeAddRef(pBinder.Extract());
}
Expand All @@ -268,9 +299,80 @@ HRESULT CLRPrivBinderAssemblyLoadContext::SetupContext(DWORD dwAppDomainId,
return hr;
}

#ifdef FEATURE_COLLECTIBLE_ALC

/* static */
BOOL CLRPrivBinderAssemblyLoadContext::DestroyContext(CLRPrivBinderAssemblyLoadContext *pBindContext)
{
{
CrstHolder holder(pBindContext->m_loadersCrst);

LoaderAllocatorSet &loaderAllocators = pBindContext->m_loaderAllocators;

LoaderAllocatorSet::Iterator iter = loaderAllocators.Begin();
while (iter != loaderAllocators.End())
{
LoaderAllocator *pLoaderAllocator = *iter;

// This context should be holding onto a reference to the LoaderAllocator, so it
// should still be alive
_ASSERTE(pLoaderAllocator->IsAlive());

if (pLoaderAllocator->IsManagedScoutAlive())
{
// We can't destroy until there is no managed reference to any of our LoaderAllocators
return FALSE;
}

iter++;
}

iter = loaderAllocators.Begin();
while (iter != loaderAllocators.End())
{
LoaderAllocator *pLoaderAllocator = *iter;

pLoaderAllocator->ReleaseAllReferences();

// Release our reference to the LoaderAllocator so it can be deleted by the LoaderAllocator GC
VERIFY(pLoaderAllocator->Release());

iter++;
}
}

// TODO: Is AppDomain::GetCurrentDomain() going to work properly?
AppDomain *pAppDomain = AppDomain::GetCurrentDomain();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above will work only when there is a single AppDomain in the process (which happens to be the current design for CoreCLR). What you really want is to get reference to the AppDomain that is associated with the Binder.

CLRPrivBinderAssemblyLoadContext::SetupContext associates the Binder's AppContext with the AppDomain who assemblies it is managing. Use SystemDomain::GetAppDomainFromId to get the AppDomain* for AppDomainID saved in SetupContext.


// The managed LoaderAllocatorScout finalizer would normally trigger the LoaderAllocator GC
// but because we hold a reference that outlasts it, we need to trigger it as well
LoaderAllocator::GCLoaderAllocators(pAppDomain);

// The managed AssemblyLoadContext should be the only remaining reference to the native
// binding context
VERIFY(pBindContext->Release() == 0);

return TRUE;
}

#endif // FEATURE_COLLECTIBLE_ALC

CLRPrivBinderAssemblyLoadContext::CLRPrivBinderAssemblyLoadContext()
{
m_pTPABinder = NULL;

#ifdef FEATURE_COLLECTIBLE_ALC
m_isCollectible = FALSE;

m_loadersCrst = new Crst(CrstType::CrstAssemblyLoadContextLoaderAllocators);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use "new (nothrow)" here. And as a consequence, you will need to fixup the uses as well accordingly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to do that, Crst overrides new which is preventing me from using nothrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using CrstStatic instead and use its InitNoThrow functionality.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should I do if it fails to initialize?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collectible ALC support should remain disabled at runtime. Any attempt to create a collectible ALC should raise an exception.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I use the CrstStatic class in clrprivbinderassemblyloadcontext.h? Including crst.h gives me lots of errors like: error C3861: 'Dont_Use_EnterCriticalSection': identifier not found.

Also how/what exception should I throw? Should it be something like this in CLRPrivBinderAssemblyLoadContext::SetupContext(), but only called when collectible?

if (!pBinder->m_loadersCrst.InitNoThrow(CrstType::CrstAssemblyLoadContextLoaderAllocators))
    GO_WITH_HRESULT(E_OUTOFMEMORY);

#endif // FEATURE_COLLECTIBLE_ALC
}

CLRPrivBinderAssemblyLoadContext::~CLRPrivBinderAssemblyLoadContext()
{
#ifdef FEATURE_COLLECTIBLE_ALC
delete m_loadersCrst;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for null prior to delete.

#endif
}

#endif // defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) && !defined(MDILNIGEN)
3 changes: 2 additions & 1 deletion src/binder/inc/assembly.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ namespace BINDER_SPACE
class CLRPrivResourceAssembly :
public ICLRPrivResource, public ICLRPrivResourceAssembly
{
public:
public:
STDMETHOD(QueryInterface)(REFIID riid, void ** ppv);
STDMETHOD_(ULONG, AddRef)();
STDMETHOD_(ULONG, Release)();
Expand All @@ -199,6 +199,7 @@ namespace BINDER_SPACE
m_pBinder = pBinder;
}

public:
inline ICLRPrivBinder* GetBinder()
{
return m_pBinder;
Expand Down
63 changes: 59 additions & 4 deletions src/binder/inc/clrprivbinderassemblyloadcontext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,32 @@ namespace BINDER_SPACE
class AssemblyIdentityUTF8;
};

class CLRPrivBinderAssemblyLoadContext : public IUnknownCommon<ICLRPrivBinder>
class AppDomain;

#ifdef FEATURE_COLLECTIBLE_ALC

class Object;
class Assembly;
class LoaderAllocator;

class DECLSPEC_UUID("68220E65-3D3F-42E2-BAD6-2D07419DAB5E") IAssemblyLoadContext : public IUnknown
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest renaming the interface to be more incontext with its purpose - something like "ICollectibleAssemblyLoadContext".

{
public:
STDMETHOD(GetIsCollectible)(
/* [retval][out] */ BOOL *pIsCollectible) = 0;

STDMETHOD(ReferenceLoaderAllocator)(
/* [in] */ LoaderAllocator *pLoaderAllocator) = 0;
};

#endif // FEATURE_COLLECTIBLE_ALC

class CLRPrivBinderAssemblyLoadContext :
#ifndef FEATURE_COLLECTIBLE_ALC
public IUnknownCommon<ICLRPrivBinder>
#else // !FEATURE_COLLECTIBLE_ALC
public IUnknownCommon<ICLRPrivBinder, IAssemblyLoadContext>
#endif // FEATURE_COLLECTIBLE_ALC
{
public:

Expand Down Expand Up @@ -46,15 +71,37 @@ class CLRPrivBinderAssemblyLoadContext : public IUnknownCommon<ICLRPrivBinder>
/* [out] */ HRESULT *pResult,
/* [out] */ ICLRPrivAssembly **ppAssembly);

#ifdef FEATURE_COLLECTIBLE_ALC

//=========================================================================
// IAssemblyLoadContext functions
//-------------------------------------------------------------------------
STDMETHOD(GetIsCollectible)(
/* [retval][out] */ BOOL *pIsCollectible);

STDMETHOD(ReferenceLoaderAllocator)(
/* [in] */ LoaderAllocator *pLoaderAllocator);

#endif // FEATURE_COLLECTIBLE_ALC

public:
//=========================================================================
// Class functions
//-------------------------------------------------------------------------

static HRESULT SetupContext(DWORD dwAppDomainId, CLRPrivBinderCoreCLR *pTPABinder,
UINT_PTR ptrAssemblyLoadContext, CLRPrivBinderAssemblyLoadContext **ppBindContext);

static HRESULT SetupContext(AppDomain *pAppDomain, CLRPrivBinderCoreCLR *pTPABinder,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the other comment, it is not clear why AppDomain* is being passed as opposed to the ID.

UINT_PTR ptrAssemblyLoadContext,
#ifdef FEATURE_COLLECTIBLE_ALC
BOOL fIsCollectible,
#endif // FEATURE_COLLECTIBLE_ALC
CLRPrivBinderAssemblyLoadContext **ppBindContext);

#ifdef FEATURE_COLLECTIBLE_ALC
static BOOL DestroyContext(CLRPrivBinderAssemblyLoadContext *pBindContext);
#endif // FEATURE_COLLECTIBLE_ALC

CLRPrivBinderAssemblyLoadContext();
~CLRPrivBinderAssemblyLoadContext();

inline BINDER_SPACE::ApplicationContext *GetAppContext()
{
Expand All @@ -81,6 +128,14 @@ class CLRPrivBinderAssemblyLoadContext : public IUnknownCommon<ICLRPrivBinder>
CLRPrivBinderCoreCLR *m_pTPABinder;

INT_PTR m_ptrManagedAssemblyLoadContext;

#ifdef FEATURE_COLLECTIBLE_ALC
BOOL m_isCollectible;

class Crst *m_loadersCrst;
typedef SHash<PtrSetSHashTraits<LoaderAllocator * > > LoaderAllocatorSet;
LoaderAllocatorSet m_loaderAllocators;
#endif // FEATURE_COLLECTIBLE_ALC
};

#endif // defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) && !defined(MDILNIGEN)
Expand Down
4 changes: 4 additions & 0 deletions src/inc/CrstTypes.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ End
Crst AssemblyIdentityCache
End

Crst AssemblyLoadContextLoaderAllocators
AcquiredBefore LoaderAllocatorReferences
End

Crst AssemblyLoader
AcquiredBefore DeadlockDetection UniqueStack
End
Expand Down
Loading