Some more cleanups in Assembly/Binder area #59288

VSadov · 2021-09-17T22:46:18Z

Reducing BINDER_SPACE::Assembly closer to what it represents. (basically a {PEImage, binder} tuple )
Made System lib to also have a binder. It is now possible and trivial to arrange, and it makes things more consistent.
Some unused code along the way

ghost · 2021-09-17T22:46:24Z

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

Issue Details

In progress. PR is mostly to run tests at this time.

Reducing BINDER_SPACE::Assembly closer to what it represents

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

VSadov · 2021-09-21T15:44:45Z

@vitek-karas @elinor-fung - there is more refactoring possible, but I think the PR is getting bigger and harder to review, so maybe it is time to take a look at this iteration?

vitek-karas

Looks great - just a couple of small things.

vitek-karas · 2021-09-22T11:17:17Z

src/coreclr/binder/inc/assembly.hpp

-    // This allows us to preferentially use the NGEN image if it is available.
+    // BINDER_SPACE::Assembly represents a result of binding to an actual assembly (PEImage)
+    // It is basically a tuple of 1) physical assembly and 2) binder which created/owns this binding
+    // We also store whether it was bound using TPA list
    class Assembly


Opening a discussion (not for this PR): Should this be called something like BoundAssembly or ResolvedAssembly or so? I always found it super confusing to have two Assembly types in the VM. And this one is not THE assembly, it's basically just a result of a successful bind (the runtime barely loaded the assembly at this point in fact, if at all).

Yes. I think this needs to be renamed, but could not yet come up with a good name.
Mechanically speaking it could be BoundPEImage, but I do not think PEImage is itself a very intuitive name. Besides we are binding assemblies, not images.

The image is basically an assembly loaded into the process, but not yet "loaded" by the Loader (that would be a PEAssembly). That is the distinction between PEImage and PEFile. They are not very related to each other and PEFile is not necessarily a file. One is "loaded" by the Loader and another is input for the Loader.

We could benefit from a better naming, but better naming is not easy :-)
Also maybe having fewer things called Assembly could help too. We also have DomainAssembly, for example.

I have definitely gotten confused by the naming as well. I do think Bound/ResolvedAssembly (I think we have more 'binding' naming in native and more 'resolving' in managed) would be better - but agree that we have lots of *Assembly going on.

basically just a result of a successful bind

Not to be confused with BindResult :)
I imagine there might be some future clean up / reconciling possible between those.

I did not get into details of BindResult yet. I assume it can represent a trial/failing attempt to bind with some context for the failure, but that is just a guess as I think we would need something like that and BindResult is that.

I will definitely look at what can be shared or whether we need a separate type.

src/coreclr/binder/assemblybindercommon.cpp

src/coreclr/vm/appdomain.cpp

src/coreclr/vm/assemblyspec.cpp

elinor-fung · 2021-09-22T21:04:21Z

src/coreclr/vm/coreassemblyspec.cpp

@@ -251,6 +221,8 @@ HRESULT BaseAssemblySpec::ParseName()

        // Copy and own any fields we do not already own
        CloneFields();
+
+        pAssemblyIdentity.SuppressRelease();


Can we just stop using NewHolder for this? The returned identity should always be owned by the ApplicationContext (and does not get add ref-ed), so we should never want the backout/release.

right, we are not keeping pAssemblyIdentity regardless, so no reason to mess with refcount.

elinor-fung · 2021-09-22T21:17:39Z

src/coreclr/binder/assemblybindercommon.cpp

+                }
+                EX_CATCH
+                {
+                    hr = E_FAIL;


Should this do GET_EXCEPTION()->GetHR() (or use EX_CATCH_HRESULT_NO_ERRORINFO) so that we can propagate the HRESULT from the exception?

I have a feeling that this cannot fail. Since we are looking at instances of PEImage we must have already looked into matadata as a part of AssemblyName::Init, but I am not completely sure, so I have a catch here.

GET_EXCEPTION()->GetHR() sounds like a good idea

elinor-fung · 2021-09-22T21:52:02Z

src/coreclr/binder/inc/assembly.hpp

-    // This allows us to preferentially use the NGEN image if it is available.
+    // BINDER_SPACE::Assembly represents a result of binding to an actual assembly (PEImage)
+    // It is basically a tuple of 1) physical assembly and 2) binder which created/owns this binding
+    // We also store whether it was bound using TPA list
    class Assembly


I have definitely gotten confused by the naming as well. I do think Bound/ResolvedAssembly (I think we have more 'binding' naming in native and more 'resolving' in managed) would be better - but agree that we have lots of *Assembly going on.

basically just a result of a successful bind

Not to be confused with BindResult :)
I imagine there might be some future clean up / reconciling possible between those.

elinor-fung · 2021-09-22T21:59:01Z

src/coreclr/binder/inc/assemblybindercommon.hpp

+
+STDAPI BinderAcquireImport(PEImage* pPEImage,
+                           IMDInternalImport** pIMetaDataAssemblyImport,
+                           DWORD* pdwPAFlags);


These are only used in one place (each) now, right? Can we just put each in the .cpp that uses it?

elinor-fung · 2021-09-22T22:06:41Z

src/coreclr/binder/assemblyname.cpp

+        PEKIND PeKind = peNone;
+        DWORD dwPAFlags[2];
+        IF_FAIL_GO(BinderAcquireImport(pPEImage, &pIMetaDataAssemblyImport, dwPAFlags));
+        IF_FAIL_GO(AssemblyBinderCommon::TranslatePEToArchitectureType(dwPAFlags, &PeKind));


nit: can we move SetArchitecture up (or move this down) so they are closer together?

elinor-fung · 2021-09-22T22:43:30Z

src/coreclr/vm/assemblyspec.cpp

@@ -1406,11 +1382,8 @@ BOOL AssemblySpecBindingCache::StoreException(AssemblySpec *pSpec, Exception* pE
        pBinderToSaveException = pSpec->GetBinder();
        if (pBinderToSaveException == NULL)
        {
-            if (!pSpec->IsAssemblySpecForCoreLib())


Is IsAssemblySpecForCoreLib still used or can we remove it?

Yep, IsAssemblySpecForCoreLib can be removed.

VSadov · 2021-09-24T00:57:17Z

The failures are caused by #59541

dotnet-issue-labeler bot added the area-AssemblyLoader-coreclr label Sep 17, 2021

VSadov force-pushed the aloader04 branch from c2248bb to 9e0d521 Compare September 21, 2021 07:21

VSadov changed the title ~~Some more cleanups in Assembly/Loader/Binder area~~ Some more cleanups in Assembly/Binder area Sep 21, 2021

VSadov marked this pull request as ready for review September 21, 2021 15:34

VSadov requested review from elinor-fung and vitek-karas September 21, 2021 15:35

VSadov mentioned this pull request Sep 21, 2021

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

Closed

2 tasks

vitek-karas approved these changes Sep 22, 2021

View reviewed changes

elinor-fung approved these changes Sep 22, 2021

View reviewed changes

VSadov force-pushed the aloader04 branch from b43931d to b3c9635 Compare September 23, 2021 06:25

VSadov added 9 commits September 23, 2021 13:13

Reducing BINDER_SPACE::Assembly closer to what it represents

06b5579

make GCC happy

3d08c43

Removed Binder[AddRef/Release]PEImage. Can just use AddRef/Release

984fe80

moved IsValidArchitecture helper to bindercommon

a1750b3

Removed the notion of PEModule - it does not exist

fa93acd

some more unused code

d8816d5

fix for GCC

cb77b37

associate System lib with DefaultBinder - makes things simpler

f12b470

PR feedback

84cbd4a

VSadov force-pushed the aloader04 branch from b3c9635 to 84cbd4a Compare September 23, 2021 20:13

runfoapp bot mentioned this pull request Sep 23, 2021

System.Text.RegularExpressions.Tests.RegexMatchTests.Match_ExcessPrefix Failing in CI #59541

Closed

VSadov merged commit 172059a into dotnet:main Sep 24, 2021

VSadov deleted the aloader04 branch September 24, 2021 21:36

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some more cleanups in Assembly/Binder area #59288

Some more cleanups in Assembly/Binder area #59288

VSadov commented Sep 17, 2021 •

edited

Loading

ghost commented Sep 17, 2021

VSadov commented Sep 21, 2021

vitek-karas left a comment

vitek-karas Sep 22, 2021

VSadov Sep 22, 2021 •

edited

Loading

elinor-fung Sep 22, 2021

VSadov Sep 22, 2021

elinor-fung Sep 22, 2021

VSadov Sep 22, 2021

VSadov Sep 22, 2021

elinor-fung Sep 22, 2021

VSadov Sep 22, 2021 •

edited

Loading

elinor-fung Sep 22, 2021

elinor-fung Sep 22, 2021

elinor-fung Sep 22, 2021

elinor-fung Sep 22, 2021

VSadov Sep 23, 2021

VSadov commented Sep 24, 2021 •

edited

Loading

Some more cleanups in Assembly/Binder area #59288

Some more cleanups in Assembly/Binder area #59288

Conversation

VSadov commented Sep 17, 2021 • edited Loading

ghost commented Sep 17, 2021

VSadov commented Sep 21, 2021

vitek-karas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov Sep 22, 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

VSadov Sep 22, 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

VSadov commented Sep 24, 2021 • edited Loading

VSadov commented Sep 17, 2021 •

edited

Loading

VSadov Sep 22, 2021 •

edited

Loading

VSadov Sep 22, 2021 •

edited

Loading

VSadov commented Sep 24, 2021 •

edited

Loading