Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Collectible Assemblies and AssemblyLoadContext #8677

Closed
wants to merge 47 commits into
base: master
from

Conversation

Projects
None yet
@xoofx
Copy link
Member

xoofx commented Dec 18, 2016

This is a followup of the issue #1684 (work done by @Rohansi) and the original issue #552

This is PR is not mergeable as it is. It is a single commit for now to simplify the tracking of things that are changed. I will recommit everything once I have cleanup some bits here and there (the changes are very rough now... I haven't followed also using the define FEATURE_COLLECTIBLE_ALC...etc.)

The difference with #1684 is that a single AssemblyLoaderAllocator is now associated to the AssemblyLoadContext. Without changing much this class, I have added the ability to add a DomainAssembly (instead of the previous SetDomainAssembly method) to chain it to another domain assembly that is in the same ALC (via the member DomainAssembly::m_NextDomainAssemblyInSameALC). It allows to use the original code for single collectible assembly.

Also, the AssemblyLoadContext creates a weak handle on itself instead of a strong handle (otherwise it cannot self finalize). The Destroy of this context doesn't actually delete the native ALC but instead release a strong handle to the LoaderAllocator managed object (associated to the AssemblyLoaderAllocator). Then the finalizer of the LoaderAllocator via the LoaderAllocatorScout will be called, and there we will cleanup everything, including releasing the native AssemblyLoadContext.

A simple test with an assembly loaded in loop seems to indicate that the memory is stable and nothing so far is crashing, but I expect many problems in many places, so this is really an early POC.

I originally didn't use the PR #1684 and tried to perform the changes on my side, but I had to cherry pick some parts of the previous PR while encountering the same issues. One thing I'm really not happy currently is the code in the LoaderAllocator::GCLoaderAllocators_RemoveAssemblies (I had to fight with issues with part of the code GC_NOTRIGGER and others incompatible GC_TRIGGERS...etc.). One thing I don't like is to have multiple methods to remove all the internal caches associated to a DomainAssembly in the AppDomain. This code should be part of AppDomain instead. There is also some issues with some maps that are using some AssemblySpec that were problematic because they were doing some GC_TRIGGERS (and the original GCLoaderAllocators_RemoveAssemblies was GC_NOTRIGGER)... so yeah, this part has to be heavily rewrite/cleanup somehow.

Comments are welcome (sorry if it is a bit of a mess!)

cc: @jkotas @gkhanna79 @rahku

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 18, 2016

Adding some other details I forgot:

  • I had to change the behavior of the AssemblyLoadContext.cs of the handle created in the InitializeLoadContextmethod. Previously the handle was normal, hence preventing the AssemblyLoadContext itself to be collected. I guess that making it Weak is more adequate in this case. I believe that when we resolve back with GCHandle.FromIntPtr in the callbacks from native, there is always a LoadXXX blocking happening on the managed side, so this is always alive.
  • Another issue in AssemblyLoadContext.cs is that the event Unloading was added. I had to remove it because I don't see how to run anything on the finalizer of this object if there is a reference to it. This was added by issue corefx issue 5205 by @kouvel If we don't want to run it on the finalizer, then it will require a more complex lock setup and prevention for using any Load method (and accessing the underlying native context). What do we want to do with this?
  • Also in the LoaderAllocator::GCLoaderAllocators at the end, the code was previously calling pAppDomain->ShutdownFreeLoaderAllocators(TRUE); meaning that underneath, the LoaderAllocator was delayed collected until the next GEN2... the problem is that it makes the collection happening a long time after and it is letting many LoaderAllocator in flight until this happens (in my simple run, the memory was going up to 80Mo instead of staying around 7Mo) . I have changed this to ShutdownFreeLoaderAllocators(FALSE); but seeing the comment in this method saying that some MethodTable of the LoaderAllocator could be somewhere kept until GEN2, I'm not comfortable with this issue as well...

@xoofx xoofx changed the title [WIP] Collectible AssemblyLoadContext [WIP] Collectible Assemblies and AssemblyLoadContext Dec 18, 2016

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 18, 2016

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 18, 2016

meaning that underneath, the LoaderAllocator was delayed collected until the next GEN2...

The GC heap has to be always walkable. It means that all objects, even the dead ones that points to unloaded loader allocator have to have a valid method table. Gen2 turns all dead objects into free space. That's why it is safe to free the loader allocator memory only after the Gen2 GC. In theory, it may possible to invent a special pass over the GC heap to turn all objects that point to the unloaded loader allocator into free space, but it would not be very different from Gen2 GC and the synchronization between this special pass and the GC would be tricky.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 18, 2016

I don't see how to run anything on the finalizer of this object if there is a reference to it

The object graph that became unreachable has still references between objects, so there should not be any fundamental problem with getting this event fired.

The current design for the reflection.emit collectible assemblies depends on the GC to figure out that it is no longer reachable. It means that all references to it have to be done via WeakHandles, etc. It would be likely pretty inconvenient in practice. It may be useful to have explicit Unload or Dispose method on the AssemblyLoadContext to initiate the unloading process: The unloading event would fire, finalizers would start running, ... .

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 19, 2016

It would be likely pretty inconvenient in practice. It may be useful to have explicit Unload or Dispose method on the AssemblyLoadContext to initiate the unloading process: The unloading event would fire, finalizers would start running, ... .

So AssemblyLoadContext would have:

  • an explicit unloading process done via an Unload method
  • an implicit unloading process via the finalizer

Explicit mode

The native AssemblyLoadContext would have a strong handle on the LoaderAllocator.
Assuming that there is no longer any references to assemblies loaded, a call to AssemblyLoadContxet.Unload would release this handle, return immediately and the Unloading event would occur once the finalizer on the LoaderAllocator has been called (but the native LoaderAllocator would not be deleted straightaway, we will keep the Gen2 as it was then). Once a LoaderAllocator is released, the Unloading event would be fired on the AssemblyLoadContext

Here I'm just a little bit worried about calling back the Unloading event indirectly from native code that was called from the finalizer of the LoaderAllocator, is it a good idea or should it be delayed? (but for delay, I have no idea right now how this would work...)

In order to secure the access for the explicit Unload, it would require an internal lock in Unload and in any LoadFromXXXX. Also after a call to Unload, any subsequent call to LoadFromXXX would throw an exception.

I guess that the Dispose pattern might not be a good mental fit for this workflow, as the AssemblyLoadContext would still be partially valid (through its pending UnLoading event), so maybe only expose an UnLoad method?

Implicit node

AssemblyLoadContext finalizer would be run without a prior Unload. The main difference with the explicit mode is that we would not have any Unloading event fired (because it would mean that nobody subscribed to it because of the finalizer)

What do you think?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 20, 2016

I do not understand why the implicit mode cannot fire the unloading event.

@Rohansi

This comment has been minimized.

Copy link

Rohansi commented Dec 20, 2016

What would be passed as the sender for the Unloading event? Would it be the AssemblyLoadContext instance? If that's the case wouldn't it be possible to resurrect it?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 20, 2016

Yes, poorly written or malicious code can resurrect it - but it can resurrect it in number of other ways too, so it is not introducing a new unique problem.

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 20, 2016

I do not understand why the implicit mode cannot fire the unloading event.

Actually, re-reading the code with my original problem with Unloading, it was only due to the current workaround with subscribing the AssemblyLoadContext to the AppDomain.ProcessExit event, so as this would not exist anymore, we can indeed call the unloading event from the finalizer.

@kouvel

This comment has been minimized.

Copy link
Member

kouvel commented Dec 20, 2016

I assume the default assembly load context would not go through the explicit unload path during shutdown. If that's the case, only the default assembly load context would need to subscribe to ProcessExit so that the Unloading event on that instance is raised upon shutdown.

@kouvel

This comment has been minimized.

Copy link
Member

kouvel commented Dec 20, 2016

Also, what about non-default assembly load contexts on shutdown? Finalizers aren't guaranteed to run on shutdown anymore, and the Unloading event was meant to provide a notification of shutdown, so the event would still need to be raised, if those instances don't go through the explicit unload path somehow during shutdown.

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 21, 2016

@kouvel good catch about the shutdown process and finalizers.

@kouvel

This comment has been minimized.

Copy link
Member

kouvel commented Dec 21, 2016

I guess one possibility is to keep track of assembly load contexts that have not yet been unloaded in a static weak reference collection. Unload and the finalizer would remove the instance from the collection and raise the Unloading event. A static method can be registered for the ProcessExit event, to raise the Unloading event on the remaining instances in the collection.

@xoofx xoofx force-pushed the xoofx:collectible-assemblies branch from d4a0b51 to 9490418 Dec 22, 2016

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 22, 2016

So I have pushed a new version.
I tried to add in the first commits changes that are not strictly related to ALC.
The last commit (Add support for collectible ALC) is still a WIP.
The AssemblyLoadContext contains now a Unload method and the Unloading event is supported.
Thought it is working most of the time, there are issues with cache invalidation that are making the feature still unstable.
I have for example discovered that a few methods from AppDomain accessing the AssemblySpecBindingCache were not protecting the access with the associated Crst CrstHolder holder(&m_DomainCacheCrst);. For example, AppDomain::FindCachedAssembly. Adding a protection to these methods lead me to a different bug:

Assert failure(PID 9916 [0x000026bc], Thread: 16008 [0x3e88]): CONTRACT VIOLATION by CrstBase::Enter at "c:\code\dotnet\coreclr\src\vm\crst.cpp" @ 358

You cannot enter a lock in a GC_NOTRIGGER + MODE_COOPERATIVE region.

VIOLATED-->  CONTRACT in Module::GetAssemblyIfLoaded at "c:\code\dotnet\coreclr\src\vm\ceeload.cpp" @ 5698
                        CONTRACT in Module::LoadAssembly at "c:\code\dotnet\coreclr\src\vm\ceeload.cpp" @ 6108
                        CONTRACT in Assembly::FindModuleByExportedType at "c:\code\dotnet\coreclr\src\vm\assembly.cpp" @ 1498
                        CONTRACT in EEClassHashTable::UncompressModuleAndClassDef at "c:\code\dotnet\coreclr\src\vm\classhash.cpp" @ 165
                        CONTRACT in ClassLoader::FindClassModuleThrowing at "c:\code\dotnet\coreclr\src\vm\clsload.cpp" @ 1745
                        CONTRACT in ClassLoader::LoadTypeHandleThrowing at "c:\code\dotnet\coreclr\src\vm\clsload.cpp" @ 2016
                        GCX_COOP in TypeName::GetTypeWorker at "c:\code\dotnet\coreclr\src\vm\typeparse.cpp" @ 1439
                        CONTRACT in TypeName::GetTypeWorker at "c:\code\dotnet\coreclr\src\vm\typeparse.cpp" @ 1437
                        CONTRACT in TypeName::GetTypeManaged at "c:\code\dotnet\coreclr\src\vm\typeparse.cpp" @ 1193
                        CONTRACT in RuntimeTypeHandle::GetTypeByName at "c:\code\dotnet\coreclr\src\vm\runtimehandles.cpp" @ 1899
                        OVERRIDE_TYPE_LOAD_LEVEL_LIMIT in MethodDescCallSite::CallTargetWorker at "c:\code\dotnet\coreclr\src\vm\callhelpers.cpp" @ 382
                        CONTRACT in MethodDescCallSite::CallTargetWorker at "c:\code\dotnet\coreclr\src\vm\callhelpers.cpp" @ 372
                        CONTRACT in GetExceptionMessage at "c:\code\dotnet\coreclr\src\vm\excep.cpp" @ 379
                        CONTRACT in GetExceptionMessage at "c:\code\dotnet\coreclr\src\vm\excep.cpp" @ 300
                        GCX_COOP in DefaultCatchHandler at "c:\code\dotnet\coreclr\src\vm\excep.cpp" @ 5840
                        CONTRACT in DefaultCatchHandler at "c:\code\dotnet\coreclr\src\vm\excep.cpp" @ 5793
                        CONTRACT in MethodDescCallSite::CallTargetWorker at "c:\code\dotnet\coreclr\src\vm\callhelpers.cpp" @ 372
                        GCX_COOP in Assembly::ExecuteMainMethod at "c:\code\dotnet\coreclr\src\vm\assembly.cpp" @ 2725
                        CONTRACT in Assembly::ExecuteMainMethod at "c:\code\dotnet\coreclr\src\vm\assembly.cpp" @ 2709
                        GCX_COOP in CorHost2::ExecuteAssembly at "c:\code\dotnet\coreclr\src\vm\corhost.cpp" @ 1334
                        ...

CORECLR! CONTRACT_ASSERT + 0x342 (0x00007ff8`8913acf2)
CORECLR! CrstBase::Enter + 0x15E (0x00007ff8`89266ebe)
CORECLR! CrstBase::AcquireLock + 0x15 (0x00007ff8`891e1295)
CORECLR! CrstBase::CrstHolder::CrstHolder + 0x25 (0x00007ff8`891dfcb5)
CORECLR! AppDomain::FindCachedAssembly + 0x34 (0x00007ff8`89525dc4)
CORECLR! Module::GetAssemblyIfLoaded + 0xAD0 (0x00007ff8`8952be00)
CORECLR! Module::LoadAssembly + 0x92A (0x00007ff8`8954e6ca)
CORECLR! Assembly::FindModuleByExportedType + 0x531 (0x00007ff8`89369341)
CORECLR! EEClassHashTable::UncompressModuleAndClassDef + 0x711 (0x00007ff8`89988691)
CORECLR! ClassLoader::FindClassModuleThrowing + 0xC88 (0x00007ff8`8945f028)
    File: c:\code\dotnet\coreclr\src\vm\crst.cpp Line: 358
    Image: C:\code\dotnet\coreclr\bin\Product\Windows_NT.x64.Debug\CoreRun.exe

But for this, I'm not really sure how to solve this, as it goes deeply into dependencies between components accessing concurrently cache of the AppDomain, so currently, the code is not threadsafe for this... The solution might be obvious for someone familiar with this codebase, or the problem is more fundamental...
Any ideas?

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 22, 2016

Ok, so the problem above is indirectly caused by deleting LoaderAllocator in the SystemDomain::ProcessDelayedUnloadDomains so it looks like there is some code running that is referencing some data allocated in a LoaderAllocator that has been deleted, causing weird side effects after (null reference...etc.)... will try to check if I can protect the memory for read/write on a fake delete and check if I get any luck...

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 22, 2016

I'm stuck with understanding what the ICLRPrivBinder is supposed to be...

When called on AssemblyNative::LoadFromPath, it is calling AssemblyNative::LoadFromPEImage which is setting the binder via

// We are working with custom Assembly Load Context so bind the assembly using it.
        CLRPrivBinderAssemblyLoadContext *pBinder = reinterpret_cast<CLRPrivBinderAssemblyLoadContext *>(pBinderContext);
        hr = pBinder->BindUsingPEImage(pImage, fIsNativeImage, &pAssembly);

At this line BindUsingPEImage there is the original CLRPrivBinderAssemblyLoadContext but then the Extract is providing a new dual interface there and storing this in the pAssembly

and after the PEAssembly is instantiated with this pAssembly... but then PEAssembly.GetBindingContext is in the end using PEFile::GetBindingContext which returns a different ICLRPrivBinder (the one bound through a dual interface on pAssembly)... so in the end, it is ending with a different BindingContext.

I'm a bit lost between these different ICLRPrivBinder, and I don't know if this is a cause of my issues (e.g they end up creating different entries in the AssemblySpecBindingCache because of these different binders)... I thought initially that I would have a single ICLRPrivBinder for a DomainAssembly, but this is not the case...

Could someone explain why we have two different ICLRPrivBinder for the DomainAssembly and the PEAssembly?

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 23, 2016

More precisely, the BINDER_SPACE::Assembly class inherits indirectly from ICLRPrivBinder (this is this one used by AssemblySpec when initializing from a PEAssembly file) but it has also a ICLRPrivBinder GetBinder() method which return the ICLRPrivBinder created for the ALC.
So how do they relate to each other? Which one should be used when attached to an AssemblySpec? (currently both are used, leading to different entries in the binding cache)

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 23, 2016

After finding some documentation about ICLRPrivBinder in the ICLRPrivBinder.idl file and from the BINDER_SPACE::Assembly, I'm starting to understand a bit more (but not a lot) how they relate to each other:

The calling sequence from AssemblyLoadContext::LoadFromPath used in a Load override is like this:

AssemblyNative::LoadFromPath
|- AssemblyNative::LoadFromPEImage
   | // create a BINDER_SPACE::Assembly which inherits from ICLRPrivAssembly and ICLRPrivBinder. 
   | // This binder is having binderALC as the parent binder
   | binderALC->BindUsingPEImage(...)
   | appDomain->LoadDomainAssembly
       appDomain->LoadDomainAssemblyInternal(...)
       |- appDomain->LoadDomainFile(...)
       |    appDomain->TryIncrementalLoad()
       |      domainFile->DoIncrementalLoad()
       |        domainAssembly->Allocate()
       |        |  // Add to AssemblySpecBindingCache with a Spec defined with binding context 
       |        |  // from BINDER_SPACE::Assembly (not the binderALC)  domainfile.cpp
       |        |- appDomain->AddAssemblyToCache (1)
       |  // Add to AssemblySpecBindingCache with the spec bound to binderALC
       |- appDomain->AddAssemblyToCache  (2)

So the (1) AddAssemblyToCache is using a spec bound to the BINDER_SPACE::Assembly allocated for the loaded assembly.
The (2) is bound to the binderALC.

My question is: Why (1) would not get the parent binder of BINDER_SPACE::Assembly, which in our case is binderALC to push it to the cache? Is it relevant to store this one? (as it looks like the spec is per assembly...)

In fact my original code from cleaning the AssemblyBindingCache was doing it on the DomainAssembly value and not the spec key, so It was working. But when I switched back to use an AssemblySpec to remove the Domainassembly from the cache, it was only able to remove the (1) but not (2). So (2) was later accessed through a AssemblyNative::Load but It was destroyed in the meantime (I still have to understand from where this AssemblyNative::Load is fired from the managed side)

So I guess that when trying to remove the cache (1), I will remove also (2) by getting the parent GetBinder() on it. Not sure if it is all clear and the right way to do it...

Any hints If I'm doing something obviously wrong would be appreciated! 😅

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 23, 2016

nevermind, the problem is somewhere else, the AssemblySpecBindingCache is using ultimately the BinderId which looks like the same for both... so still investigating...

@xoofx xoofx force-pushed the xoofx:collectible-assemblies branch from 9490418 to d2dd977 Dec 26, 2016

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 26, 2016

So I just pushed a stable and working version. 🎉

Finally got rid of the caching problems by removing from the cache based on the DomainAssembly and not on the AssemblySpec (thought I had a bug but discovered that there was many entries with slightly different AssemblySpec in the AssemblySpecBindingCache while they could point to the same DomainAssembly)

The AssemblyLoadContext.cs implements also the UnLoading event on AppDomain.ProcessExit

There is a slight increase of memory when running over 1M of reload, but I haven't been able to find exactly what is leaking (tried taking Memory heap native snapshot from VS2015 but not sure how much it is reliable...)

There are a few bits not completely polished (missing a few #ifdef FEATURE_COLLECTIBLE_ALC and localized exception messages)... but before going further, I will wait for some feedback here! 😉

@xoofx

This comment has been minimized.

Copy link
Member Author

xoofx commented Dec 26, 2016

Going to look at the failing tests and how to add new tests related to collectible ALC.

@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.ASSEMBLY

This comment has been minimized.

@jkotas

jkotas Dec 26, 2016

Member

Nit: Extra ASSEMBLY

This comment has been minimized.

@xoofx

xoofx Dec 26, 2016

Author Member

fixed

Unloading = null;
// Initialize the VM side of AssemblyLoadContext if not already done.
isDefault = fRepresentsTPALoadContext;
var thisWeakHandle = GCHandle.Alloc(this, GCHandleType.Weak);

This comment has been minimized.

@jkotas

jkotas Dec 26, 2016

Member

The unloadability comes with extra overhead. We do not want to make everybody (even folks who do not need unloading) to pay for it. There should be a new constructor that takes flag on whether the ALC is unloadable.

This comment has been minimized.

@xoofx

xoofx Dec 27, 2016

Author Member

I'm wondering where is the overhead exactly coming from? (the GC that has more work to do on a collectible assembly?).

Currently the unload is automatically done on the ALC finalizer if not called explicitly. So that would mean that if an ALC is not unloadable, we would throw an exception on the Unload method? Behind the scene, the only difference would be that the native Assemblies + AssemblyLoadContext + AssemblyLoaderAllocator would not me marked collectible I guess.

This comment has been minimized.

@jkotas

jkotas Dec 27, 2016

Member

GC has more work to do, JIT generates less efficient code in some cases, runtime data structures are bigger, ... .

// Remove this instance from the assembly to cleanup on ProcessExit
lock (assemblyLoadContextAliveList)
{
if (isProcessExiting)

This comment has been minimized.

@jkotas

jkotas Dec 26, 2016

Member

There is no finalization on shutdown in .NET Core - this path should be unreachable.

This comment has been minimized.

@xoofx

xoofx Dec 26, 2016

Author Member

Ok, I was not sure. So a finalizer cannot be called while callbacks on AppDomain.ProcessExit method is running? (GC.Collect for example has no effect there?)

This comment has been minimized.

@jkotas

jkotas Dec 27, 2016

Member

The finalizer can be still called if the object became unreachable before and was sitting in the finalization queue when shutdown started. (I should have said that this path should not be necessary; not unreachable.)

This comment has been minimized.

@xoofx

xoofx Dec 27, 2016

Author Member

Good, so a collectible boolean is definitely necessary.

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Dec 26, 2016

Here is a small test that I have written that fails with the asserts in FileLoadLock::Release: https://gist.github.com/jkotas/0113226ff908484dca9b7353a5da51fd . I am not sure about the best way to fix them. It may be necessary simplify/refactor the various assembly loader hashtables to make this work well. Some of these tables are redundant and/or hard to reason about.

My test shows how to get this feature tested under stress, e.g. replace loading of hello.exe by loading all CoreCLR tests. https://github.com/Microsoft/dotnet-reliability has harness that we are using for stress testing. It may be a good idea to build a stress mode for unloadable ALC into it. cc @schaabs

Also, we will need to make sure that it works well with other features - interop, debugger, ... .

The overall direction looks reasonable to me so far. @xoofx Thank you for doing this!

@masonwheeler

This comment has been minimized.

Copy link

masonwheeler commented Feb 6, 2018

@karelz all right, so what are we blocked on?

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Feb 6, 2018

@masonwheeler we're "blocked" on investment into a large feature :) ... stay tuned, I am figuring out offline what is the plan here (I am not directly involved in the feature and its planning).

@masonwheeler

This comment has been minimized.

Copy link

masonwheeler commented Feb 6, 2018

@karelz All right, thanks :)

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Feb 8, 2018

So, here's the official word to set expectations - likely not what most people hoped for (sorry for that ☹️), but at least it provides clarity:

  • Majority of the remaining work is around testing (mostly stress testing). It will require some non-trivial investment (months).
  • The work is not planned for 2.1 release.
  • We do not have firm plans beyond 2.1 yet.
@masonwheeler

This comment has been minimized.

Copy link

masonwheeler commented Feb 8, 2018

@karelz You're right, that's not what we want.

But remember that this is an open-source project now. So, what can the community do to help improve on that?

@kwaclaw

This comment has been minimized.

Copy link

kwaclaw commented Feb 8, 2018

@masonwheeler The community (in the person of @xoofx) already went as far as it could, but if I remember correctly, the specialized test environment was (is?) not accessible to the community, so it was up to MS to finish the job.

In other words, this issue is dead.

@masonwheeler

This comment has been minimized.

Copy link

masonwheeler commented Feb 8, 2018

@kwaclaw Sorry, but no. I'm not willing to accept that.

This is a feature that has not only been needed, but obviously needed, from the days of .NET Framework version 1. We've waited too long on Microsoft not doing anything about it. Now that it's finally available to the community to work on it, Microsoft continuing to not do anything about it is no longer a reasonable excuse.

What can the community do to improve this?

@DemiMarie

This comment has been minimized.

Copy link

DemiMarie commented Feb 8, 2018

@masonwheeler

This comment has been minimized.

Copy link

masonwheeler commented Feb 8, 2018

@DemiMarie That sounds like a good idea. Who here has experience with stress testing?

@hokusp

This comment has been minimized.

Copy link

hokusp commented Feb 22, 2018

Very disappointing that this feature is not considered important. We really wanted to use .net core for a larger project but will now have to use another platform.

@kwaclaw

This comment has been minimized.

Copy link

kwaclaw commented Feb 24, 2018

@DemiMarie I think a community-driven stress test won't work because if I remember the thread correctly, Microsoft would only accept the pull request if it passed their own stress test environment.

However, Microsoft is not able to share this environment with the community and has no intention of performing the stress testing themselves. Therefore I conclude that this pull request/feature is dead for the foreseeable future.

@DemiMarie

This comment has been minimized.

Copy link

DemiMarie commented Feb 25, 2018

@jakesays

This comment has been minimized.

Copy link

jakesays commented Mar 3, 2018

Months of work? Really? I find that hard to believe. This is really bad news. I am surprised that such a needed feature has been shelved.

@Diaskhan

This comment has been minimized.

Copy link

Diaskhan commented Mar 9, 2018

There have a few problems that ms team could not be solved.

1.Not enough skilled C++ programmers.
2.there problem that all c++ code leaks a memory! Thats why a need a months to make it work! Threads are very leak memory sensivity!
3. Net core platform is not for enterprise development! Net core is for startup small projects ! Products that written on django and ruby in rails! Enterprise is not the main goal! The aim small teams, small projects thats why nobody want mess with leak memory!
4.there a problem that ms team has lost mobile market, and ms team want lose a small market of startup projects ! They do everything for azure platform dependency!
I learn net core, and im think many problem with net core! Many things that could not be cover by buisinnes requremns! Assembly is main goal of ernterprise development!

@Diaskhan

This comment has been minimized.

Copy link

Diaskhan commented Mar 9, 2018

  1. The reflection usage drop down .net permomance! Maybe why ms team dont like usage of reflectuions!
@seriouz

This comment has been minimized.

Copy link

seriouz commented Mar 30, 2018

Whats going on here? Can someone from MS fix this?

@masonwheeler

This comment has been minimized.

Copy link

masonwheeler commented Mar 30, 2018

@seriouz Yes, but they're choosing not to. Even though this is something that .NET has needed from the very beginning, which developers have been requesting from the very beginning, even though someone else has gone and done for them the work that they chose not to do, they won't integrate it until it's been tested in a special testing system that they're choosing not to use and they won't permit anyone else to do for them.

They could. But they aren't.

@dabretin

This comment has been minimized.

Copy link

dabretin commented Mar 31, 2018

It's ridiculous...
Perhaps ms doesn't want to be next web app container standard promoting web devs with Java and Oracle (ironic inside) ?

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Mar 31, 2018

The changes in this PR are a good prototype. It is very light on testing. It has not been hard to find problems in it by just looking at the code that should have been found by testing it well instead. Majority work on this feature is on testing and getting it to the right quality level. Our estimate is that this needs months, likely close to a year, of experienced runtime developer to make it happen. We were not able to dedicate one for this due to competing priorities. I do acknowledge that we could have handle this better. I am sorry.

The sources for the stress testing system are at https://github.com/Microsoft/dotnet-reliability. It is basically xunit with stress testing extensions. It is a special system, but we have not been actively preventing anybody from using it. Inventing a new system for testing this feature would be ok too similar to what we have for GC stress or JIT stress.

We are starting to plan for where we are going to invest once .NET Core 2.1 ships. Having better story for code unloading is high on our list. I am hopeful that it will become a part of plan for the next release.

@masonwheeler

This comment has been minimized.

Copy link

masonwheeler commented Mar 31, 2018

@jkotas Thanks, that's really helpful! 👍

OK, so now we have the means to build a stress test environment. Anyone up for a bit of work on this?

@Apollo3zehn Apollo3zehn referenced this pull request Apr 12, 2018

Open

Nuget plugin manager: Remaining tasks #140

8 of 20 tasks complete
@kamikyo

This comment has been minimized.

Copy link

kamikyo commented May 3, 2018

Expect this function

janvorli added a commit to janvorli/coreclr that referenced this pull request May 11, 2018

Enable assembly unloading
This is just a rebased version of dotnet#8677

janvorli added a commit to janvorli/coreclr that referenced this pull request May 14, 2018

Enable assembly unloading
This is just a rebased version of dotnet#8677

janvorli added a commit to janvorli/coreclr that referenced this pull request Jun 14, 2018

Enable assembly unloading
This is just a rebased version of dotnet#8677
@benaadams

This comment has been minimized.

Copy link
Collaborator

benaadams commented Jun 14, 2018

Follow up PR #18476

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jun 15, 2018

Superseded by #18476

@jkotas jkotas closed this Jun 15, 2018

janvorli added a commit to janvorli/coreclr that referenced this pull request Aug 19, 2018

Enable assembly unloading
This is just a rebased version of dotnet#8677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.