[WIP] Add support for collectible assemblies via AssemblyLoadContext

[Rohansi]

This adds support for collectible assemblies that are loaded with AssemblyLoadContext. As of right now the managed side of this is very incomplete but I am able to load and call something from an assembly, let it get collected by the GC, and can see the assembly disappear from the AppDomain assembly list.

My current issues with this are:

• the managed AssemblyLoadContext needs changes to properly clean up after itself (free GC handle, free native instance)
• assemblies are not fully unloaded from the AppDomain
• assembly files are locked on disk, even after they're collected
• assemblies might be cached too?
• might be able to get rid of the empty assembly and just lazily create a LoaderAllocator
• dependencies of collectible assemblies aren't loaded in the same context, and when they are they don't unload properly
• there's probably some GC holes in there
• need a crst for the native AssemblyLoadContext
• memory leaks: load, run, collect loop shows memory usage slowly increasing
• probably need more error checking
• I'm going to need to write a lot of tests for this

This is related to issue #552.

[Rohansi]

@jkotas Could you take a look at this to see if the way I'm approaching this is okay?

[Rohansi]

@jkotas I got rid of the call to Assembly::CreateDynamic() and gave each assembly loaded from AssemblyLoadContext its own LoaderAllocator. This should allow the LoaderAllocator GC to clean them up properly because it wasn't made to handle multiple assemblies per LoaderAllocator.

When trying to load a previously collected assembly, it crashes somewhere in AppDomain::CheckForMismatchedNativeImages() because the previous binder was used after freed or AppDomain::AddAssemblyToCache() fails because the assembly is already in the cache.

Those two caches allocate memory for entries in one of the AppDomain's LoaderHeaps. Should I be trying to remove the entries from there or should they not be in there at all?

[jkotas]

Should I be trying to remove the entries from there or should they not be in there at all?

I think they should be allocated on regular heap instead, so that they can be freed once the assembly load context is collected.

[Rohansi]

@jkotas I'm removing those entries from the AppDomain now. Now I can load, run and collect an assembly in a loop!

I tried adding a dependency to the collectible assembly but it doesn't get loaded through the AssemblyLoadContext, so it wont be collectible. Is this the intended behavior?

[jkotas]

I am not exactly sure what you mean. Each assembly is loaded into some load context. The intended behavior is that all assemblies loaded into unloadable load context are unloaded as a unit when the unloadable load context is collected by the GC.

[Rohansi]

Yes, but when I load an assembly into an unloadable context, and that same assembly depends on another assembly, should that also use the unloadable context? This comment seems to suggest it should, but it doesn't check the TPA list before loading.

[Rohansi]

@jkotas What I tested was loading Test.dll into a collectible AssemblyLoadContext. However, Test.dll depends on TestDep.dll. Should both Test.dll and TestDep.dll be loaded by the same AssemblyLoadContext?

When I try it TestDep.dll gets loaded by the TPA binder, even when it's not on the TPA list!

If I check if the assembly is in the TPA list before trying to bind using the TPA binder, TestDep.dll will be loaded into the same AssemblyLoadContext and they will be collected together.

[jkotas]

Should both Test.dll and TestDep.dll be loaded by the same AssemblyLoadContext?

It depends on implementation of your override for AssemblyLoadContext,Load(AssemblyName assemblyName). If the assembly is not on TPA list and the override for your unloadable context handles both Test and TestDep, e.g.:

      protected override Assembly Load(AssemblyName assemblyName)
      {
          string simpleName = assemblyName.Name;
          if (simpleName == "Test" || simpleName == "TestDep")
              return LoadFromAssemblyPath(Path.Combine(..., simpleName + ".dll");
         return null;
      }

then both Test.dll and TestDep.dll should be loaded into the same AssemblyLoadContext.

[Rohansi]

I am using something similar to that but without checking the assembly name. I load Test.dll with LoadFromAssemblyName on an instance of my CustomLoadContext, but TestDep.dll will load with the TPA binder.

My directory tree looks like this:

runtime\
  - CoreRun.exe
  - coreclr.dll
  - mscorlib.dll
  - System.*.dll
program\
  - Program.exe
  - Test.dll
  - TestDep.dll

And I run Program.exe with: ..\runtime\CoreRun.exe Program.exe

Everything in the runtime directory should be on the TPA list, so loads for those should go through the TPA binder. But when it comes to loading TestDep.dll, it runs this bit of code and ends up using the TPA binder for TestDep.dll.

If I replace that line with the following, it will load into my custom AssemblyLoadContext:

hr = HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND);
if (m_pTPABinder->IsInTpaList(pAssemblyName->GetSimpleName()))
    hr = m_pTPABinder->BindAssemblyByNameWorker(pAssemblyName, &pCoreCLRFoundAssembly);

The comments around there suggest it should be checking the TPA list for the assembly name in question, but it never does so.

Aside from that, I need to start writing tests for this. Where should they go? It looks like most should go in the corefx repo but what about stress tests?

[jkotas]

Where should they go? It looks like most should go in the corefx repo

Right, corefx repo is the preferred place for contract tests. There are some tests for the System.Runtime.Loader contract already.

what about stress tests?

Several options:

• Targeted stress tests can be just regular tests, as long as they not take too long to execute.
• Long running tests should be marked [OuterLoop], not really limited to stress tests.
• Stress test mix (e.g. multiple tests running in parallel, in different combinations, in a single process, for a long time - days). xunit has some of it built in, folks are looking into it - Add Generic/Exceptions test cases for JIT #839.

[davidfowl]

@Rohansi awesome job so far! I'm looking forward to this making it in!

/cc @pranavkm this would come in handy for view compilation in general.

[Rohansi]

I'm not really sure what I should be doing in the CoreFX repo. Should I be opening another issue there for the API addition? I tried to write some tests but the project references System.Runtime.Loader from MyGet so I couldn't use the new constructor. Getting CoreFX to use my build of CoreCLR also has its own problems: https://github.com/dotnet/coreclr/issues/1197

Thanks @davidfowl!

[jkotas]

@kouvel Could you please update instructions on how to write and run CoreFX tests against private CoreCLR with new APIs, since you went through it for your recent AssemblyLoadContext addition?

[kouvel]

The changes to CoreCLR and CoreFX have to be staged. Once the CoreCLR changes are merged and mscorlib is updated on myget.org, you can consume that and update any contracts in the CoreFX repo. Once the contract changes are merged and myget.org is updated with the new contracts, you can consume them for tests. I'll add some instructions on this.

[kouvel]

You'll need to update src/mscorlib/model.xml with any new public/protected APIs, otherwise I believe the new APIs will be stripped out prior to publishing. I'm not sure how you would do that though, since the new APIs are enabled conditionally. @weshaggard, do you have a suggestion for this?

[weshaggard]

model.xml accepts conditions the same way as we set up #ifdef's. However I don't really like public APIs be conditions so we should either take them or not. We should avoid having them sometimes and not other times.

[Rohansi]

@jkotas I'm still getting a memory leak with my load-run-collect loop. Is there something that can help me find it, or could someone else take a look? It looks like it's leaking about 8 bytes per assembly on each iteration. I'm compiling for x64 so it's probably a pointer being added to some array somewhere.

@kouvel This page is about running the CoreFX tests on a custom build of CoreCLR/mscorlib. It's out of date but would be really useful to make sure I didn't break anything.

[jkotas]

Is there something that can help me find it

I would use perfview because of I am familiar with it - any good memory analysis / leak hunting tool should work though.

[kouvel]

@Rohansi, you can use the these instructions for running CoreFX tests outside Windows. To use a private build of CoreCLR, see the arguments to run-test.sh, you just need to pass in --coreclr-bins and --mscorlib-bins. I'll update the instructions you linked.

On Windows, CoreFX tests are run during the build and I believe they use a published build of CoreCLR, not sure if there's currently a way to run against a private build of CoreCLR.

[kouvel]

I'm in the process of adding some relevant testing / new API instructions here: #1761.

[Rohansi]

I'm not having any luck tracking down this memory leak. Would someone else be able to take a look? I can upload the programs I'm using to test it.

I'm 99% sure it's not allocated in my code and is just something that needs to be deleted when unloading the assembly, but I have no idea what is saved when loading assemblies. I only found the ones I do remove because it crashes when I don't.

Also, looking over my current changes for issues would be nice so I can take a break from the memory leak!

[Rohansi]

I found it! A value is being added to AppDomain::m_pLargeHeapHandleTable on every assembly load but was never removed.

Now when running memory usage seems to be stable. I saw it increase from 6.7 MB to 7.0 MB over 5 million reloads of 2 assemblies. It was stuck around 7 MB for a while so that might have just been the heap expanding or something.

@jkotas Could you look over this to see if there's any problems with my additions?

[jkotas]

This should not be needed. The handle in StoreObjectInLazyHandle should be allocated from collectible LoaderAllocator and automatically reclaimed once the collectible LoaderAllocator is destroyed.

Otherwise, you will have this memory leak problem in every place where LoaderAllocator::AllocateHandle is called.

[Rohansi]

So I should be moving some of the LoaderAllocator initialization I added to Assembly::Create() into AppDomain::LoadDomainAssemblyInternal()? This would allow me to use the correct LoaderAllocator when creating the DomainAssembly and should fix other cases.

[Rohansi]

I just ran all the corefx tests with a debug build of this and nothing failed, good to know I didn't break anything!

@jkotas Is the fix used in Rohansi@1544af8 okay?

[jkotas]

@jkotas Is the fix used in Rohansi@ 1544af8 okay?

It is certainly an improvement. (But the 1:N mapping between AssemblyLoadContext and LoaderAllocator still does not look right.)

[Rohansi]

I was originally trying to only have one LoaderAllocator for the AssemblyLoadContext but I kept running into issues with it. Things like the LoaderAllocator GC were only written to handle one assembly per LoaderAllocator, so I tried other ways that didn't require changing how everything else works.

Is there lots of overhead per LoaderAllocator or is it just strange?

[jkotas]

I think you will run into cases where one assembly owned by the load context gets collected, but other assemblies owned by the same load context are still going to have reference to it (e.g. pointers burnt in JITed code or EE data structures).

[Rohansi]

That shouldn't happen, the CLRPrivBinderAssemblyLoadContext instance associated with the AssemblyLoadContext holds a reference to all the LoaderAllocators and will only release them when the managed part of all LoaderAllocators is gone (phase 3). The remaining reference will prevent the LoaderAllocator GC from collecting it.

[Rohansi]

@jkotas I should be defining my own Crst type, right? I reused an existing type to get it working but the documentation says I shouldn't do that. So I added one to CrstTypes.def but I can't find CrstTypeTool!

[jkotas]

Adding the tool in #1789

[gkhanna79]

Check for null prior to delete.

[Rohansi]

@jkotas I just found an issue in this and i'm not sure what problems it could cause, or how to fix it. The managed Assembly objects will get collected before the AssemblyLoadContext if there are no references to them.

Using the same example I described before, Program.exe loads Test.dll into a collectible AssemblyLoadContext, Test.dll depends on TestDep.dll. Program uses reflection to call a function in Test which calls a function in TestDep. When the GC runs, TestDep's Assembly object will be collected and disappears from the list returned by AppDomain.GetAssemblies().

The function continues to work after TestDep's managed Assembly object is collected. However, this would mean any WeakReferences to TestDep would be cleared. This is definitely not what we want but I'm not sure how to prevent them from being collected unless there are no references to them.

[jkotas]

This is definitely not what we want but I'm not sure how to prevent them from being collected unless there are no references to them.

Right, this is not what we want.

AssemblyLoadContext is referenced via weak handle, and each of the Assembly owned by the AssemblyLoadContext is referenced via weak handle as well. It explains why Assembly can be collected independently on AssemblyLoadContext.

The fix should be to get it rid of the per assembly weak handle - have weak handle per AssemblyLoadContext only.

[Rohansi]

The fix should be to get it rid of the per assembly weak handle - only have weak handle per AssemblyLoadContext.

There is no per Assembly weak handle. I was referring to user code having a weak handle for them.

Another problem is AssemblyLoadContext cannot have strong references to any of the Assemblies it loaded or it will never be able to be collected. The Assembly will keep the managed LoaderAllocator alive which prevents the AssemblyLoadContext from finalizing.

Finding out how to give the managed objects their proper lifetimes is making my head hurt. There's so many things to keep track of, cycles to avoid, and requirements to meet. I need to start making diagrams.

[jkotas]

There is no per Assembly weak handle.

There is weak handle per LoaderAllocator. And LoaderAllocator is per Assembly right now. It means that there is per Assembly weak handle - it is just not created in the new code that you have written.

I believe that you will need to go with LoaderAllocator for the AssemblyLoadContext to fix these sort of problems. I understand that it may not be straightforward to make it happen, but I do not see a way how to avoid it.

[Rohansi]

I believe that you will need to go with LoaderAllocator for the AssemblyLoadContext to fix these sort of problems.

I'm going to try doing this again. I just don't fully understand what keeps the Assembly objects alive. It looks like somewhere in MethodTable there's a handle for the LoaderAllocator, so maybe there's a reference to the Assembly in LoaderAllocator.m_slots?

[Kingnaoufal]

@Rohansi
No update for this pull request !!??

[Rohansi]

@Kingnaoufal I've been busy recently but I did start doing that. I'll see if I can get it into a working state this weekend.

[Rohansi]

I don't have much time to work on this, and this doesn't implement the proper lifetimes for assemblies in an AssemblyLoadContext. I'm closing this PR but if I get time to do it properly I'll open a new one.

[xoofx]

This PR looks great so far, thanks @Rohansi !

@jkotas trying to understand the discussion around LoaderAllocator (as it seems the last remaining big issue)... ideally, there should be only one new LoaderAllocator per AssemblyLoadContext?

[jkotas]

one new LoaderAllocator per AssemblyLoadContext

Yes, that's reasonable.

[xoofx]

one new LoaderAllocator per AssemblyLoadContext

cool, will rebase this PR and try to continue in this direction

[gkhanna79]

CC @rahku

[xoofx]

@jkotas currently the AssemblyLoadContext.LoadFromPath is expecting to load from the disk. Do you know why it was not designed with a LoadFromMemory instead? If there is no restriction, could I change the internal of that?

[jkotas]

There is LoadFromStream that loads from memory. LoadFromPath is basically a specialized version of it optimized for the common case where the assembly is on disk.

[xoofx]

Oops, indeed, sorry missed that one 😅

[xoofx]

So after looking a bit more at the code, there is currently 3 types of LoaderAllocator:

• LAT_Global ↔️ GlobalLoaderAllocator
• LAT_AppDomain ↔️ AppDomainLoaderAllocator
• LAT_Assembly ↔️ AssemblyLoaderAllocator

AssemblyLoaderAllocator seems that it was designed for Assembly::CreateDynamic

As we want to add a LoaderAllocator that would manage multiple assemblies as an unit, we would have something like LAT_AssemblyLoadContext ↔️ AssemblyLoadContextLoaderAllocator, I'm wondering if we still want to keep AssemblyLoaderAllocator or better try to merge both into a single one where AssemblyLoadContextLoaderAllocator is a generalized version (n assemblies instead of 1) of AssemblyLoaderAllocator?

In the end we would have only:

• LAT_Global ↔️ GlobalLoaderAllocator
• LAT_AppDomain ↔️ AppDomainLoaderAllocator
• LAT_AssemblyLoadContext ↔️ AssemblyLoadContextLoaderAllocator

What do you think?

[jkotas]

I'm wondering if we still want to keep AssemblyLoaderAllocator or better try to merge both into a single one

Either way sounds fine to me. Merging both may make near term progress harder ... if it is the case, I would lean towards keeping them separate, for now at least.

[xoofx]

I have opened a new PR #8677 as a followup of this one.