Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level

[elinor-fung]

There is no logical distinction between creating a DomainAssembly and an Assembly now. Making DomainAssembly create the Assembly as soon as it is constructed should make it so that we can switch assorted things that currently store/use DomainAssembly to Assembly, since they will be created at the same time.

Contributes to #104590

cc @jkotas @AaronRobinsonMSFT

[dotnet-policy-service]

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

[jkotas]

If something fails between the DomainAssembly constructor and this point, are we going to leak the memory allocated on the pamTracker now that we are releasing it much sooner?

[elinor-fung]

I don't think so? Once we're done with the DomainAssembly constructor, all the memory allocated on the tracker should be associated with the Assembly, which should be handled if DomainAssembly is released.

[jkotas]

I was worried about the situation where we do not have per-assembly loader allocator. Ideally, everything that was allocated is released if anything in this method fails.

After closer inspection, I do not think this change is introducing any new issues.

There are pre-existing issues though. For example, if InitVirtualCallStubManager above fails, the GC handle allocated at

runtime/src/coreclr/vm/loaderallocator.cpp

Line 1025 in ffd3b18

m_hLoaderAllocatorObjectHandle = GetDomain()->CreateLongWeakHandle(*pKeepLoaderAllocatorAlive);

is going leak.

[jkotas]

I was worried about the situation where we do not have per-assembly loader allocator. Ideally, everything that was allocated is released if anything in this method fails.

It is better to have AllocMemTracker amTracker in the top-level scope of the operation for this reason. It makes it easy to see that everything allocated via AllocMemTracker is going to be released in case the operation fails. Moving the AllocMemTracker into the inner scope does not look like an improvement.

[elinor-fung]

It is better to have AllocMemTracker amTracker in the top-level scope of the operation for this reason. It makes it easy to see that everything allocated via AllocMemTracker is going to be released in case the operation fails. Moving the AllocMemTracker into the inner scope does not look like an improvement.

Do you mean making the DomainAssembly constructor take in an AllocMemTracker (instead of it handling creating one specifically for creating the Assembly)? I was thinking that it was clearer for the caller to not have to be concerned about creating an AllocMemTracker at all and being able to expect the DomainAssembly/Assembly to properly handle themselves.

[jkotas]

Do you mean making the DomainAssembly constructor take in an AllocMemTracker (instead of it handling creating one specifically for creating the Assembly)?

Yes.

I was thinking that it was clearer for the caller to not have to be concerned about creating an AllocMemTracker at all

It is less correct. If there are any failure point between the AllocMemTracker.SuppressRelease call and returning from the QCall, we will have a memory leak on failure (for the non-collectible dynamic assembly). There does not seems to be any such failure points currently, but it is hard to see that.

Non-collectible DomainAssembly / Assembly cannot free the memory allocated on the loader heaps themselves when there is a failure.

[jkotas]

I was worried about the situation where we do not have per-assembly loader allocator. Ideally, everything that was allocated is released if anything in this method fails.

After closer inspection, I do not think this change is introducing any new issues.

There are pre-existing issues though. For example, if InitVirtualCallStubManager above fails, the GC handle allocated at

runtime/src/coreclr/vm/loaderallocator.cpp

Line 1025 in ffd3b18

m_hLoaderAllocatorObjectHandle = GetDomain()->CreateLongWeakHandle(*pKeepLoaderAllocatorAlive);

is going leak.