-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Make DomainAssembly
create its Assembly
in its constructor and remove separate allocate load level
#107224
Conversation
… separate allocate load level
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
src/coreclr/vm/assembly.cpp
Outdated
@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar | |||
|
|||
//Cannot fail after this point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 viaAllocMemTracker
is going to be released in case the operation fails. Moving theAllocMemTracker
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar | |||
|
|||
//Cannot fail after this point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
…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.
There is no logical distinction between creating a
DomainAssembly
and anAssembly
now. MakingDomainAssembly
create theAssembly
as soon as it is constructed should make it so that we can switch assorted things that currently store/useDomainAssembly
toAssembly
, since they will be created at the same time.Contributes to #104590
cc @jkotas @AaronRobinsonMSFT