-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[WIP] Collectible Assemblies and AssemblyLoadContext #8677
Changes from all commits
73e0b05
7277c9e
a684502
0ead976
ed1e48b
8700f0b
9e8502d
c98620f
3d4aea0
193f43a
3fbb577
fc230d6
c144e65
82236db
175919a
450cce1
586c496
9e7eeb3
9e55d70
df223c1
0cc6d4d
0fcfbb7
b730cfc
1e34b7d
e4b17c6
ad97104
af24afe
b2c0a2e
46b113f
204bb85
b84cb07
3761429
021ff8e
100f96e
c1464f7
44e0f51
594668c
c2b428d
c3dfee1
76be9e9
9aef1e7
7aa1d01
1ad89ed
78c1e2c
63993c5
9e1dc35
6464f58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,29 @@ 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") ICollectibleAssemblyLoadContext : public IUnknown | ||
{ | ||
public: | ||
STDMETHOD(GetLoaderAllocator)( | ||
/* [retval][out] */ LoaderAllocator** pLoaderAllocator) = 0; | ||
}; | ||
|
||
#endif // FEATURE_COLLECTIBLE_ALC | ||
|
||
class CLRPrivBinderAssemblyLoadContext : | ||
#ifndef FEATURE_COLLECTIBLE_ALC | ||
public IUnknownCommon<ICLRPrivBinder> | ||
#else // !FEATURE_COLLECTIBLE_ALC | ||
public IUnknownCommon<ICLRPrivBinder, ICollectibleAssemblyLoadContext> | ||
#endif // FEATURE_COLLECTIBLE_ALC | ||
{ | ||
public: | ||
|
||
|
@@ -45,14 +67,35 @@ class CLRPrivBinderAssemblyLoadContext : public IUnknownCommon<ICLRPrivBinder> | |
/* [out] */ HRESULT *pResult, | ||
/* [out] */ ICLRPrivAssembly **ppAssembly); | ||
|
||
#ifdef FEATURE_COLLECTIBLE_ALC | ||
|
||
//========================================================================= | ||
// IAssemblyLoadContext functions | ||
//------------------------------------------------------------------------- | ||
STDMETHOD(GetLoaderAllocator)( | ||
/* [retval][out] */ LoaderAllocator** pLoaderAllocator); | ||
|
||
#endif // FEATURE_COLLECTIBLE_ALC | ||
|
||
public: | ||
//========================================================================= | ||
// Class functions | ||
//------------------------------------------------------------------------- | ||
|
||
static HRESULT SetupContext(DWORD dwAppDomainId, CLRPrivBinderCoreCLR *pTPABinder, | ||
UINT_PTR ptrAssemblyLoadContext, CLRPrivBinderAssemblyLoadContext **ppBindContext); | ||
|
||
static HRESULT SetupContext(DWORD dwAppDomainId, | ||
CLRPrivBinderCoreCLR *pTPABinder, | ||
#ifdef FEATURE_COLLECTIBLE_ALC | ||
LoaderAllocator* pLoaderAllocator, | ||
void* loaderAllocatorHandle, | ||
#endif | ||
UINT_PTR ptrAssemblyLoadContext, | ||
CLRPrivBinderAssemblyLoadContext **ppBindContext); | ||
|
||
#ifdef FEATURE_COLLECTIBLE_ALC | ||
void PrepareForLoadContextRelease(INT_PTR ptrManagedStrongAssemblyLoadContext); | ||
void ReleaseLoadContext(); | ||
#endif | ||
|
||
CLRPrivBinderAssemblyLoadContext(); | ||
|
||
inline BINDER_SPACE::ApplicationContext *GetAppContext() | ||
|
@@ -80,6 +123,11 @@ class CLRPrivBinderAssemblyLoadContext : public IUnknownCommon<ICLRPrivBinder> | |
CLRPrivBinderCoreCLR *m_pTPABinder; | ||
|
||
INT_PTR m_ptrManagedAssemblyLoadContext; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldnt these fields be under #ifdef FEATURE_COLLECTIBLE_ALC? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm actually maybe no, the field is used indirectly through GetManagedAssemblyLoadContext() which was already there before collectible ALC. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was referring to the loadAllocator fields below :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errata, fixed by commit c3dfee1 |
||
#ifdef FEATURE_COLLECTIBLE_ALC | ||
LoaderAllocator* m_pAssemblyLoaderAllocator; | ||
void* m_loaderAllocatorHandle; | ||
#endif | ||
}; | ||
|
||
#endif // defined(FEATURE_HOST_ASSEMBLY_RESOLVER) && !defined(DACCESS_COMPILE) && !defined(CROSSGEN_COMPILE) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DestroyContext implementation (earlier) assumes that
m_ptrManagedAssemblyLoadContext
is a weak handle while this one does not? How is this consistency being maintained (or what are invariants here)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually,
DestroyContext
switchedm_ptrManagedAssemblyLoadContext
from a weak to a strong handle.This method is called later by
AssemblyLoaderAllocator::OnUnloading
to actually let the managedAssemblyLoadContext
to be finalized (if necessary).Maybe we should rename these methods
DestroyContext
andReleaseManagedAssemblyLoadContext
to better reflect their actual behaviorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - the naming update, and comments, would be helpful here. I would suggest DestroyContext to be something like PrepareForLoadContextRelease and ReleaseManagedAssemblyLoadContext to be ReleaseLoadContext (so that, in future, additional cleanup can be added that is not related to managed instance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially fixed by commit 0cc6d4d (comments may have to be improved)