Some cleanups in Assembly/Loader area #57023

VSadov · 2021-08-07T05:21:01Z

For the most part just mechanical changes and refactoring with no impact to functionality.

renamed GAC -> TPA
removed ICLRPrivAssembly interface
replaced ICLRPrivBinder interface with AssemblyBinder - a base binder class.
Assembly does not need to be a binder. (it has a binder, but no longer is a binder itself)
moved ApplicationContext down to AssemblyBinder to remove some redundancy.
removed GetBinderID from everything that had it as BinderID appears to be a self-supporting feature.
renamed CLRPrivBinderCoreCLR and CLRPrivBinderAssemblyLoadContext
to more sensible DefaultAssemblyBinder and CustomAssemblyBinder

Since ICLRPriv. . . and COM related stuff is gone, we can now have a simple hierarchy of binders that looks like this:

                ................................. use-> ...  BINDER_SPACE::AssemblyBinderCommon 
              /                              /                      (bunch of static helpers)
DefaultAssemblyBinder        CustomAssemblyBinder
                    \        /
                AssemblyLoadContext
                         |
                   AssemblyBinder

ghost · 2021-08-07T05:21:08Z

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

rename GAC -> TPA
remove ICLRPrivAssembly
remove ICLRPrivBinder

Author:	VSadov
Assignees:	-
Labels:	`area-AssemblyLoader-coreclr`
Milestone:	-

VSadov · 2021-08-17T02:28:18Z

I think this is ready for review.
There are certainly more cleanup opportunities, but this change achieves a particular goal - getting rid of COM interfaces in binder/loader area and simplifying binder hierarchy.

vitek-karas · 2021-08-18T14:45:37Z

Thinking forward a bit (future changes, not this one): could we please rename all of the "binder context" variables/names to just "binder"? It's just confusing. Or alternatively (and maybe even better) - merge AssemblyBinder and AssemblyLoadContext to a single AssemblyLoadContext. This might be a bit tricky with keeping the binder separation, but maybe that should go as well. In which case the whole "binder" term could go away as well.

vitek-karas

This is really nice!

Mostly just nits and suggestions for possible future improvements. Feel free to keep this as is since it's already big enough :-)

vitek-karas · 2021-08-18T13:43:36Z

src/coreclr/binder/assembly.cpp

@@ -174,40 +131,17 @@ namespace BINDER_SPACE
    }

    // --------------------------------------------------------------------
-    // ICLRPrivAssembly methods
+    // BINDER_SPACE::Assembly methods


I would remove this comment - I don't think it makes sense anymore.

Right. Some comments moved with the code and the style could be out of place in the new location or may not be very helpful. Some were not very helpful to start with and likely were forced by some kind of "every method/class must have a comment" policy.
'An assembly represents a particular set of bits.' is my favorite :-).

vitek-karas · 2021-08-18T14:08:21Z

src/coreclr/binder/inc/defaultassemblybinder.h

+class PEAssembly;
+class PEImage;
+
+class DefaultAssemblyBinder final : public AssemblyLoadContext


Nit: I would prefer DefaultAssemblyLoadContext. It just feels weird that the inheritance is AssemblyBinder -> AssemblyLoadContext -> DefaultAssemblyBinder.
Same for CustomAssemblyLoadContext.
But it's just a subjective naming - so feel free to keep as is.

I think "Context" works better for something passive - like environment used for binding. When the class itself provides binding functionality it should be Binder, since it is more than just context.
The base class main purpose is to provide "Bind" APIs, so it feels like it should be "Binder".

The AssemblyLoadContext feels a bit odd in the hierarchy until you realize its purpose is to link to the managed AssemblyLoadContext counterpart.

ALC does not add any "Bind" methods and is not normally used for binding (or should not be encouraged). We could in theory imagine a Binder that inherits from AssemblyBinder directly and does not have ALC stuff on it. In theory.

Technically ALC could be merged with the base class, but since it serves distinct purposes, it feels better to keep it a separate concern in the hierarchy and keep its name too.

ALC does not add any "Bind" methods and is not normally used for binding (or should not be encouraged).

I'm not sure I follow this. Logically ALC.LoadFromAssemblyPath is the managed equivalent of AssemblyBinder.BindUsingPEImage (this is actually pretty much exactly the relationship). ALC.LoadFromAssemblyName
maps onto AssemblyBinder.BindUsingAssemblyName (it's also a pretty good match in reality). It's not exact since there are additional layers between the managed code and native (to deal with marshalling validation and so on). So while I agree that nobody in the runtime should call the Bind methods directly, they are sort of called "directly" from managed a lot.

But I guess I see your point. Runtime has a separate concept of "binding" (finding the right file for a given assembly spec) and a separate concept of "loading" (actually parsing a file and making it available to run code from). The managed APIs don't have these as separate concepts (although in the past we discussed that having it would be beneficial in some cases).

I still think that the sealed classes should be called DefaultAssemblyLoadContext/CustomAssemblyLoadContext - since they're a specialization of AssemblyLoadContext - but it's just a subjective naming thing :-)

DefaultAssemblyLoadContext/CustomAssemblyLoadContext would fit in my brain better as well. Although I also have the native AssemblyLoadContext and AssemblyBinder classes lumped together in my brain, so that may be why.

it serves distinct purposes, it feels better to keep it a separate concern in the hierarchy and keep its name too.

Since what is clearer / or less clear to each person is pretty subjective, it might help to have comments in the headers with the intent / describing the intentional separation.

As we discuss this I am getting more and more convinced that native AssemblyLoadContext should probably be merged with its base.

Native AssemblyLoadContext is not a native representation of the managed one, it just has a link to the managed, and can provide it via GetManagedAssemblyLoadContext. As a matter of completely mechanical naming it would be something like AssemblyBinderWithManagedAssemblyLoadContext.

However we do not really have concrete binders that cannot provide a managed context. If we do not envision such a case, then maybe we do not need an abstract one either.
Native AssemblyLoadContext seems to add more naming confusion than actual functionality (which is not all that much).

Native AssemblyLoadContext is not a native representation of the managed one, it just has a link to the managed, and can provide it via GetManagedAssemblyLoadContext

True, but for all intents and purposes AssemblyLoadContext-native has 1:1 relationship with AssemblyLoadContext-managed. The native one has a pointer to the managed one (and vice versa). Also all of the functionality of managed ALC is implemented (indirectly) in the native ALC (lot of this is hidden in the ApplicationContext which is embeded in the native ALC - and yes - that name is TERRIBLE and has to go :-), but that's a different refactoring)

the ApplicationContext which is embeded in the native ALC - and yes - that name is TERRIBLE and has to go

The ApplicationContext which also contains an ExecutionContext which is a subclass of LoadContext! So much context.

And the fact that in managed there is a public class ApplicationContext (in winforms) and that there is an AppContext class (also public) in CoreLib. Either of which has absolutely no relationship to the native one :-)

vitek-karas · 2021-08-18T14:15:06Z

src/coreclr/binder/customassemblybinder.cpp

-                                            UINT_PTR ptrAssemblyLoadContext,
-                                            CLRPrivBinderAssemblyLoadContext **ppBindContext)
+HRESULT CustomAssemblyBinder::SetupContext(DefaultAssemblyBinder *pTPABinder,
+                                                       AssemblyLoaderAllocator* pLoaderAllocator,


Nit: indentation seems to be off

vitek-karas · 2021-08-18T14:27:45Z

src/coreclr/vm/multicorejitimpl.h

@@ -614,7 +614,7 @@ class MulticoreJitRecorder
 {
 private:
    AppDomain               * m_pDomain;            // AutoStartProfile could be called from SystemDomain
-    ICLRPrivBinder * m_pBinderContext;
+    AssemblyBinder * m_pBinderContext;


Nit: match indentation of the code around

vitek-karas · 2021-08-18T14:27:57Z

src/coreclr/vm/multicorejitimpl.h

@@ -274,7 +274,7 @@ class MulticoreJitProfilePlayer
 friend class MulticoreJitRecorder;

 private:
-    ICLRPrivBinder * m_pBinderContext;
+    AssemblyBinder * m_pBinderContext;


Nit: match indentation

vitek-karas · 2021-08-18T14:35:44Z

src/coreclr/vm/pefile.cpp

-        UINT_PTR thisBinderId = 0;
-        if (FAILED(this->GetHostAssembly()->GetBinderID(&thisBinderId)))
-            return FALSE;
+        AssemblyBinder* fileBinderId = pFile->GetHostAssembly()->GetBinder();


Nit: fileBinderId -> fileBinder and also thisBinderId -> thisBinder

Right. I missed that these are still Ids

vitek-karas · 2021-08-18T14:37:46Z

src/coreclr/vm/pefile.cpp

-    m_pAssemblyLoadContext = (pOpaqueBinder != NULL) ?
-        (AssemblyLoadContext*)pOpaqueBinder :
+    m_pAssemblyLoadContext = (pBindingContext != NULL) ?
+        (AssemblyLoadContext*)pBindingContext :


Just curious: This means that all AssemblyBinder instances are also AssemblyLoadContext instances. Maybe in the future we can remove another layer of abstraction...

I thought about merging AssemblyBinder and AssemblyLoadContext an decided to keep them separate on purpose.

AssemblyLoadContext should keep providing the link to the managed ALC, but should itself not be involved in binding business.

Well - it has to be involved by the virtue of having a link to the managed extensions points (events and so on). But I guess it's OK to view them as slightly separate.

At the first glance I saw enough usage of AssemblyLoadContext on the native side, but looking closer it mostly refers to the the managed one or to its name. The part that there is a native class that matches the name of the managed type may not be all that helpful.

Perhaps we do not need AssemblyLoadContext on the native side and should just move its stuff to the AssemblyBinder.

I need to think about this.
It is easier to merge types than to un-merge them back later, if we regret it :-)

It is easier to merge types than to un-merge them back later, if we regret it :-)

That is very true. 👍

vitek-karas · 2021-08-18T14:41:57Z

src/coreclr/vm/appdomain.hpp

@@ -1103,7 +1103,7 @@ class BaseDomain

 #endif // DACCESS_COMPILE && !CROSSGEN_COMPILE

-    CLRPrivBinderCoreCLR *GetTPABinderContext() {LIMITED_METHOD_CONTRACT;  return m_pTPABinderContext; }
+    DefaultAssemblyBinder *GetTPABinderContext() {LIMITED_METHOD_CONTRACT;  return m_pTPABinderContext; }


I would like if we could rename this method to GetDefaultBinderContext - or even better GetDefaultBinder. And in general get rid of the "binder context" term, and just use "binder".

The "TPA" part should be more or less an implementation detail. I know we're now keeping it on per-assembly level, but eventually that should go away as well (as a term and maybe even as the functionality - I'm having certain doubts about its usefulness - but that's a different discussion).

Yes. It might be ok to use "TPA" when referring to the actual set of assemblies, but there is really no such thing as TPA binder - there is DefaultBinder who's binding behavior mostly guided by TPA set, but that is an implementation detail for the binder.

vitek-karas · 2021-08-18T14:47:25Z

src/coreclr/vm/assemblyspec.cpp

        {
            // If the parent assembly is a platform (TPA) assembly, then its binding context will always be the TPABinder context. In
            // such case, we will return the default context for binding to allow the bind to go
            // via the custom binder context, if it was overridden. If it was not overridden, then we will get the expected
            // TPABinder context anyways.
            //
            // Get the reference to the default binding context (this could be the TPABinder context or custom AssemblyLoadContext)
-            pParentAssemblyBinder = static_cast<ICLRPrivBinder*>(pDomain->GetTPABinderContext());
+            pParentAssemblyBinder = static_cast<AssemblyBinder*>(pDomain->GetTPABinderContext());


Do we even need the specific cast here? This should cast implicitly just fine.

Actually - we don't any of this now - if pParentAssemblyBinder is already the default binder - then this will assign the exact same value to it - no change.
We should simply remove this entire if and just keep the one below which uses default binder if it's null.

Maybe keep the comments in some way.

Right. I also think adding IsDefault() to the AssemblyBinder could simplify a few places that do if (binder == GetDomain()->GetBinder()) { . . . }

vitek-karas · 2021-08-18T14:51:13Z

src/coreclr/vm/assemblynative.cpp

-        hr = pTPABinder->BindUsingPEImage(pImage, fIsNativeImage, &pAssembly);
-    }
+    DefaultAssemblyBinder *pTPABinder = pCurDomain->GetTPABinderContext();
+    hr = pBinderContext->BindUsingPEImage(pImage, fIsNativeImage, &pAssembly);


vitek-karas · 2021-08-18T14:53:49Z

Side note: First time for me to use the . key on a PR - it was SO MUCH easier to review that way.

VSadov · 2021-08-18T17:57:05Z

could we please rename all of the "binder context" variables/names to just "binder"

YES, just did not want to put too much into one change.

elinor-fung

This is great 🎉

elinor-fung · 2021-08-18T18:18:52Z

src/coreclr/binder/assemblybindercommon.cpp

    // and fExplicitBindToNativeImage, and see code:CEECompileInfo::LoadAssemblyByPath
    // for an example of how they're used.
-    HRESULT AssemblyBinder::BindAssembly(/* in */  ApplicationContext  *pApplicationContext,
+    HRESULT AssemblyBinderCommon::BindAssembly(/* in */  AssemblyBinder      *pBinder,


nit: alignment

Throughout due to a bunch of the AssemblyBinder -> AssemblyBinderCommon renames.

elinor-fung · 2021-08-18T18:48:54Z

src/coreclr/vm/assemblynative.cpp

-        // Bind the assembly using TPA binder
-        hr = pTPABinder->BindUsingPEImage(pImage, fIsNativeImage, &pAssembly);
-    }
+    DefaultAssemblyBinder *pTPABinder = pCurDomain->GetTPABinderContext();


Don't think you need pCurDomain or pTPABinder anymore.

elinor-fung · 2021-08-18T19:18:09Z

src/coreclr/binder/inc/defaultassemblybinder.h

+class PEAssembly;
+class PEImage;
+
+class DefaultAssemblyBinder final : public AssemblyLoadContext


DefaultAssemblyLoadContext/CustomAssemblyLoadContext would fit in my brain better as well. Although I also have the native AssemblyLoadContext and AssemblyBinder classes lumped together in my brain, so that may be why.

it serves distinct purposes, it feels better to keep it a separate concern in the hierarchy and keep its name too.

Since what is clearer / or less clear to each person is pretty subjective, it might help to have comments in the headers with the intent / describing the intentional separation.

…rence counting.

…Common

ghost · 2021-08-18T22:17:55Z

Hello @VSadov!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

janvorli · 2021-09-02T14:33:52Z

src/coreclr/binder/inc/assemblybindercommon.hpp

@@ -71,6 +76,15 @@ namespace BINDER_SPACE

        static HRESULT TranslatePEToArchitectureType(DWORD  *pdwPAFlags, PEKIND *PeKind);

+        static HRESULT DefaultBinderSetupContext(DefaultAssemblyBinder** ppTPABinder);
+
+        // TODO: The call indicates that this can come from a case where


@VSadov is this TODO obsolete? If not, it would be nice to have an issue for it and reference it here.

Possibly obsolete. CorLib may have NULL binder, but I am not sure if it goes through here.

I will check on this TODO in later changes.
I also think the statics on AssemblyBinderCommon may be moved to AssemblyBinder, but have not decided on that yet.

I think some of this code will go to managed eventually, just want to clean it up a bit as-is beforehand. It seems like a good thing to do regardless.

dotnet-issue-labeler bot added the area-AssemblyLoader-coreclr label Aug 7, 2021

VSadov force-pushed the aloader01 branch 3 times, most recently from d300f4c to 731f68c Compare August 7, 2021 19:16

VSadov force-pushed the aloader01 branch from 5043079 to f0b4524 Compare August 16, 2021 02:19

VSadov changed the title ~~[WIP] Some cleanups in Assembly/Loader area~~ Some cleanups in Assembly/Loader area Aug 16, 2021

VSadov marked this pull request as ready for review August 16, 2021 22:28

VSadov force-pushed the aloader01 branch 2 times, most recently from 6a3199c to 624f386 Compare August 16, 2021 23:42

VSadov requested review from elinor-fung, janvorli, vitek-karas and jkotas August 17, 2021 02:23

VSadov force-pushed the aloader01 branch from 624f386 to 4f6f0db Compare August 17, 2021 06:52

runfoapp bot mentioned this pull request Aug 17, 2021

Test failure in System.Diagnostics.Process.Tests #57452

Closed

VSadov force-pushed the aloader01 branch from 4f6f0db to ca5736f Compare August 17, 2021 20:54

runfoapp bot mentioned this pull request Aug 18, 2021

Test failure readytorun/determinism/crossgen2determinism/crossgen2determinism.sh #57620

Closed

vitek-karas approved these changes Aug 18, 2021

View reviewed changes

runfoapp bot mentioned this pull request Aug 18, 2021

Feed unreliability affecting CI #55449

Closed

vitek-karas mentioned this pull request Aug 18, 2021

Remove support for the APP_NI_PATHS property #57616

Merged

elinor-fung approved these changes Aug 18, 2021

View reviewed changes

VSadov added 4 commits August 18, 2021 14:42

rename GAC -> TPA

be13383

remove GetAvailableImageTypes

440bd6e

Removed ICLRPrivAssembly

081f08c

Move Assembly:: Add/Release to .inl

27d414c

VSadov added 19 commits August 18, 2021 14:42

ApplicationContext is not implementing IUnknown

767ef95

ApplicationContext does not need AppDomainId

a984bd5

ICLRPrivBinder is not a COM object

5b2218c

Assembly is not a binder and should not have BindAssemblyByName

68a9b4c

ApplicationContext is always an embedded value and does not need refe…

24ca380

…rence counting.

Simplified GetBinderID and GetLoaderAllocator, since not COM

6ed1653

Moved AppContext to up to ICLRPrivBinder

d435108

Removed GetBinderHash

eb3bc8e

Removed a couple now pointless AddRef/Release

331eccb

virtualized BindUsingAssemblyName

a186ca4

renamed ICLRPrivBinder --> AssemblyBinder

a616b18

renamed BINDER_SPACE::AssemblyBinder --> BINDER_SPACE::AssemblyBinder…

63ef26b

…Common

Merge CCoreCLRBinderHelper into AssemblyBinderCommon

8d61a2d

Rename CLRPrivBinderCoreCLR -->DefaultAssemblyBinder

61068b0

Renamed CLRPrivBinderAssemblyLoadContext --> CustomAssemblyBinder

f1ee77f

Renamed PTR_ICLRPrivBinder --> PTR_AssemblyBinder

9920bc9

Remove clrprivbinding_i.cpp

6cc9cef

A few touch ups

babbcd2

fix Linux build

b241939

VSadov force-pushed the aloader01 branch from ca5736f to b241939 Compare August 18, 2021 21:51

VSadov added the auto-merge label Aug 18, 2021

ghost merged commit 7f85a8c into dotnet:main Aug 18, 2021

VSadov deleted the aloader01 branch August 19, 2021 00:21

janvorli reviewed Sep 2, 2021

View reviewed changes

agocke mentioned this pull request Sep 17, 2021

Refactor hosting and assembly loading to improve reliability, perf, and portability #59227

Closed

2 tasks

ghost locked as resolved and limited conversation to collaborators Oct 2, 2021

This pull request was closed.

Some cleanups in Assembly/Loader area #57023

Some cleanups in Assembly/Loader area #57023

Conversation

VSadov commented Aug 7, 2021 • edited Loading

ghost commented Aug 7, 2021

VSadov commented Aug 17, 2021

vitek-karas commented Aug 18, 2021

vitek-karas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinor-fung Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitek-karas commented Aug 18, 2021

VSadov commented Aug 18, 2021

elinor-fung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elinor-fung Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

ghost commented Aug 18, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov commented Aug 7, 2021 •

edited

Loading

VSadov Aug 18, 2021 •

edited

Loading

VSadov Aug 18, 2021 •

edited

Loading

elinor-fung Aug 18, 2021 •

edited

Loading

VSadov Aug 18, 2021 •

edited

Loading

VSadov Aug 18, 2021 •

edited

Loading

elinor-fung Aug 18, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.