Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level
#107224
Conversation
… separate allocate load level
ghost
added
the
|
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
src/coreclr/vm/assembly.cpp
Outdated
| CANNOTTHROWCOMPLUSEXCEPTION(); | ||
| FAULT_FORBID(); | ||
|
|
||
| //Cannot fail after this point |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is better to have
AllocMemTracker amTrackerin the top-level scope of the operation for this reason. It makes it easy to see that everything allocated viaAllocMemTrackeris going to be released in case the operation fails. Moving theAllocMemTrackerinto 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.
There was a problem hiding this comment.
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.
src/coreclr/vm/assembly.cpp
Outdated
| CANNOTTHROWCOMPLUSEXCEPTION(); | ||
| FAULT_FORBID(); | ||
|
|
||
| //Cannot fail after this point |
There was a problem hiding this comment.
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
…move separate allocate load level (dotnet#107224) 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.
…move separate allocate load level (dotnet#107224) 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.
…move separate allocate load level (dotnet#107224) 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.
2 participants
There is no logical distinction between creating a
DomainAssemblyand anAssemblynow. MakingDomainAssemblycreate theAssemblyas soon as it is constructed should make it so that we can switch assorted things that currently store/useDomainAssemblytoAssembly, since they will be created at the same time.Contributes to #104590
cc @jkotas @AaronRobinsonMSFT