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

Merged
elinor-fung merged 4 commits into
dotnet:mainfrom
elinor-fung:domain-assembly-create
Sep 4, 2024
Merged

Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level#107224
elinor-fung merged 4 commits into
dotnet:mainfrom
elinor-fung:domain-assembly-create

Conversation

@elinor-fung

@elinor-fungelinor-fung commented Aug 31, 2024•
edited
Loading

Copy link
Copy Markdown
Member

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

@ghostghost added the area-AssemblyLoader-coreclr only use for closed issues label Aug 31, 2024
@dotnet-policy-service

dotnet-policy-serviceBot commented Aug 31, 2024

Copy link
Copy Markdown
Contributor

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

@elinor-fung
elinor-fung marked this pull request as ready for review August 31, 2024 04:37
jkotas
jkotas reviewed Aug 31, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp Outdated
@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

//Cannot fail after this point

@jkotasjkotasAug 31, 2024

Copy link
Copy Markdown
Member

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?

@elinor-fungelinor-fungAug 31, 2024

Copy link
Copy Markdown
MemberAuthor

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.

@jkotasjkotasSep 1, 2024

Copy link
Copy Markdown
Member

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);
is going leak.

@jkotasjkotasSep 1, 2024

Copy link
Copy Markdown
Member

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.

@elinor-fungelinor-fungSep 3, 2024•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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 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.

@jkotasjkotasSep 4, 2024

Copy link
Copy Markdown
Member

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.

jkotas
jkotas approved these changes Sep 1, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Comment threadsrc/coreclr/vm/assembly.cpp Outdated
Comment threadsrc/coreclr/vm/assembly.cpp Outdated
@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

//Cannot fail after this point

@jkotasjkotasSep 1, 2024

Copy link
Copy Markdown
Member

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);
is going leak.

This was referenced Sep 4, 2024
Closed
Closed
@elinor-fung
elinor-fung merged commit ec2f534 into dotnet:mainSep 4, 2024
@elinor-fung
elinor-fung deleted the domain-assembly-create branch September 4, 2024 23:34
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Sep 6, 2024
@elinor-fung@radekdoulik
Make DomainAssembly create its Assembly in its constructor and re…
5d40942
…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.
@elinor-fungelinor-fung mentioned this pull request Sep 6, 2024
Merged
jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024
@elinor-fung@jtschuster
Make DomainAssembly create its Assembly in its constructor and re…
bb5c65d
…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.
sirntar pushed a commit to sirntar/runtime that referenced this pull request Sep 30, 2024
@elinor-fung@sirntar
Make DomainAssembly create its Assembly in its constructor and re…
3cf66d4
…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.
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 12, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes

Assignees

@elinor-fungelinor-fung

Labels

area-AssemblyLoader-coreclronly use for closed issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@elinor-fung@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level - #107224
Merged
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Sep 4, 2024
Merged
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level#107224
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Conversation
@elinor-fung
@elinor-fungelinor-fung
commented
Aug 31, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@ghostghost
added
the 
area-AssemblyLoader-coreclr
only use for closed issues
label
Aug 31, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Aug 31, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov

See info in area-owners.md if you want to be subscribed.
@elinor-fung
elinor-fung
marked this pull request as ready for review
August 31, 2024 04:37
jkotas
jkotas
reviewed
Aug 31, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasAug 31, 2024

Copy link


Copy Markdown

Member
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?
@elinor-fungelinor-fungAug 31, 2024

Copy link


Copy Markdown

MemberAuthor
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.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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.
@elinor-fungelinor-fungSep 3, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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 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.
@jkotasjkotasSep 4, 2024

Copy link


Copy Markdown

Member
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.
jkotas
jkotas
approved these changes
Sep 1, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.

This was referenced Sep 4, 2024

Closed


Closed

@elinor-fung
elinor-fung
merged commit ec2f534
into
dotnet:mainSep 4, 2024
@elinor-fung
elinor-fung
deleted the

domain-assembly-create

branch
September 4, 2024 23:34
radekdoulik pushed a commit
to radekdoulik/runtime
that referenced
this pull request
Sep 6, 2024
@elinor-fung@radekdoulik
Make DomainAssembly create its Assembly in its constructor and re…
5d40942
…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.
@elinor-fungelinor-fung
mentioned this pull request
Sep 6, 2024
 Merged

jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@elinor-fung@jtschuster
Make DomainAssembly create its Assembly in its constructor and re…
bb5c65d
…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.
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@elinor-fung@sirntar
Make DomainAssembly create its Assembly in its constructor and re…
3cf66d4
…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.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

Assignees

@elinor-fungelinor-fung

Labels

area-AssemblyLoader-coreclronly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@elinor-fung@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level - #107224
Merged
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Sep 4, 2024
Merged
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level#107224
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Conversation
@elinor-fung
@elinor-fungelinor-fung
commented
Aug 31, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@ghostghost
added
the 
area-AssemblyLoader-coreclr
only use for closed issues
label
Aug 31, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Aug 31, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov

See info in area-owners.md if you want to be subscribed.
@elinor-fung
elinor-fung
marked this pull request as ready for review
August 31, 2024 04:37
jkotas
jkotas
reviewed
Aug 31, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasAug 31, 2024

Copy link


Copy Markdown

Member
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?
@elinor-fungelinor-fungAug 31, 2024

Copy link


Copy Markdown

MemberAuthor
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.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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.
@elinor-fungelinor-fungSep 3, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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 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.
@jkotasjkotasSep 4, 2024

Copy link


Copy Markdown

Member
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.
jkotas
jkotas
approved these changes
Sep 1, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.

This was referenced Sep 4, 2024

Closed


Closed

@elinor-fung
elinor-fung
merged commit ec2f534
into
dotnet:mainSep 4, 2024
@elinor-fung
elinor-fung
deleted the

domain-assembly-create

branch
September 4, 2024 23:34
radekdoulik pushed a commit
to radekdoulik/runtime
that referenced
this pull request
Sep 6, 2024
@elinor-fung@radekdoulik
Make DomainAssembly create its Assembly in its constructor and re…
5d40942
…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.
@elinor-fungelinor-fung
mentioned this pull request
Sep 6, 2024
 Merged

jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@elinor-fung@jtschuster
Make DomainAssembly create its Assembly in its constructor and re…
bb5c65d
…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.
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@elinor-fung@sirntar
Make DomainAssembly create its Assembly in its constructor and re…
3cf66d4
…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.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

Assignees

@elinor-fungelinor-fung

Labels

area-AssemblyLoader-coreclronly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@elinor-fung@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level - #107224
Merged
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Sep 4, 2024
Merged
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level#107224
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Conversation
@elinor-fung
@elinor-fungelinor-fung
commented
Aug 31, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@ghostghost
added
the 
area-AssemblyLoader-coreclr
only use for closed issues
label
Aug 31, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Aug 31, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov

See info in area-owners.md if you want to be subscribed.
@elinor-fung
elinor-fung
marked this pull request as ready for review
August 31, 2024 04:37
jkotas
jkotas
reviewed
Aug 31, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasAug 31, 2024

Copy link


Copy Markdown

Member
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?
@elinor-fungelinor-fungAug 31, 2024

Copy link


Copy Markdown

MemberAuthor
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.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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.
@elinor-fungelinor-fungSep 3, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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 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.
@jkotasjkotasSep 4, 2024

Copy link


Copy Markdown

Member
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.
jkotas
jkotas
approved these changes
Sep 1, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.

This was referenced Sep 4, 2024

Closed


Closed

@elinor-fung
elinor-fung
merged commit ec2f534
into
dotnet:mainSep 4, 2024
@elinor-fung
elinor-fung
deleted the

domain-assembly-create

branch
September 4, 2024 23:34
radekdoulik pushed a commit
to radekdoulik/runtime
that referenced
this pull request
Sep 6, 2024
@elinor-fung@radekdoulik
Make DomainAssembly create its Assembly in its constructor and re…
5d40942
…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.
@elinor-fungelinor-fung
mentioned this pull request
Sep 6, 2024
 Merged

jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@elinor-fung@jtschuster
Make DomainAssembly create its Assembly in its constructor and re…
bb5c65d
…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.
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@elinor-fung@sirntar
Make DomainAssembly create its Assembly in its constructor and re…
3cf66d4
…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.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

Assignees

@elinor-fungelinor-fung

Labels

area-AssemblyLoader-coreclronly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@elinor-fung@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level - #107224
Merged
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Sep 4, 2024
Merged
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level#107224
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Conversation
@elinor-fung
@elinor-fungelinor-fung
commented
Aug 31, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@ghostghost
added
the 
area-AssemblyLoader-coreclr
only use for closed issues
label
Aug 31, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Aug 31, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov

See info in area-owners.md if you want to be subscribed.
@elinor-fung
elinor-fung
marked this pull request as ready for review
August 31, 2024 04:37
jkotas
jkotas
reviewed
Aug 31, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasAug 31, 2024

Copy link


Copy Markdown

Member
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?
@elinor-fungelinor-fungAug 31, 2024

Copy link


Copy Markdown

MemberAuthor
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.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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.
@elinor-fungelinor-fungSep 3, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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 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.
@jkotasjkotasSep 4, 2024

Copy link


Copy Markdown

Member
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.
jkotas
jkotas
approved these changes
Sep 1, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.

This was referenced Sep 4, 2024

Closed


Closed

@elinor-fung
elinor-fung
merged commit ec2f534
into
dotnet:mainSep 4, 2024
@elinor-fung
elinor-fung
deleted the

domain-assembly-create

branch
September 4, 2024 23:34
radekdoulik pushed a commit
to radekdoulik/runtime
that referenced
this pull request
Sep 6, 2024
@elinor-fung@radekdoulik
Make DomainAssembly create its Assembly in its constructor and re…
5d40942
…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.
@elinor-fungelinor-fung
mentioned this pull request
Sep 6, 2024
 Merged

jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@elinor-fung@jtschuster
Make DomainAssembly create its Assembly in its constructor and re…
bb5c65d
…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.
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@elinor-fung@sirntar
Make DomainAssembly create its Assembly in its constructor and re…
3cf66d4
…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.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

Assignees

@elinor-fungelinor-fung

Labels

area-AssemblyLoader-coreclronly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@elinor-fung@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level - #107224
Merged
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Sep 4, 2024
Merged
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level#107224
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Conversation
@elinor-fung
@elinor-fungelinor-fung
commented
Aug 31, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@ghostghost
added
the 
area-AssemblyLoader-coreclr
only use for closed issues
label
Aug 31, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Aug 31, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov

See info in area-owners.md if you want to be subscribed.
@elinor-fung
elinor-fung
marked this pull request as ready for review
August 31, 2024 04:37
jkotas
jkotas
reviewed
Aug 31, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasAug 31, 2024

Copy link


Copy Markdown

Member
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?
@elinor-fungelinor-fungAug 31, 2024

Copy link


Copy Markdown

MemberAuthor
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.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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.
@elinor-fungelinor-fungSep 3, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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 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.
@jkotasjkotasSep 4, 2024

Copy link


Copy Markdown

Member
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.
jkotas
jkotas
approved these changes
Sep 1, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.

This was referenced Sep 4, 2024

Closed


Closed

@elinor-fung
elinor-fung
merged commit ec2f534
into
dotnet:mainSep 4, 2024
@elinor-fung
elinor-fung
deleted the

domain-assembly-create

branch
September 4, 2024 23:34
radekdoulik pushed a commit
to radekdoulik/runtime
that referenced
this pull request
Sep 6, 2024
@elinor-fung@radekdoulik
Make DomainAssembly create its Assembly in its constructor and re…
5d40942
…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.
@elinor-fungelinor-fung
mentioned this pull request
Sep 6, 2024
 Merged

jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@elinor-fung@jtschuster
Make DomainAssembly create its Assembly in its constructor and re…
bb5c65d
…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.
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@elinor-fung@sirntar
Make DomainAssembly create its Assembly in its constructor and re…
3cf66d4
…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.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

Assignees

@elinor-fungelinor-fung

Labels

area-AssemblyLoader-coreclronly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@elinor-fung@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level - #107224
Merged
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Sep 4, 2024
Merged
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level#107224
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Conversation
@elinor-fung
@elinor-fungelinor-fung
commented
Aug 31, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@ghostghost
added
the 
area-AssemblyLoader-coreclr
only use for closed issues
label
Aug 31, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Aug 31, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov

See info in area-owners.md if you want to be subscribed.
@elinor-fung
elinor-fung
marked this pull request as ready for review
August 31, 2024 04:37
jkotas
jkotas
reviewed
Aug 31, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasAug 31, 2024

Copy link


Copy Markdown

Member
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?
@elinor-fungelinor-fungAug 31, 2024

Copy link


Copy Markdown

MemberAuthor
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.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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.
@elinor-fungelinor-fungSep 3, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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 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.
@jkotasjkotasSep 4, 2024

Copy link


Copy Markdown

Member
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.
jkotas
jkotas
approved these changes
Sep 1, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.

This was referenced Sep 4, 2024

Closed


Closed

@elinor-fung
elinor-fung
merged commit ec2f534
into
dotnet:mainSep 4, 2024
@elinor-fung
elinor-fung
deleted the

domain-assembly-create

branch
September 4, 2024 23:34
radekdoulik pushed a commit
to radekdoulik/runtime
that referenced
this pull request
Sep 6, 2024
@elinor-fung@radekdoulik
Make DomainAssembly create its Assembly in its constructor and re…
5d40942
…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.
@elinor-fungelinor-fung
mentioned this pull request
Sep 6, 2024
 Merged

jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@elinor-fung@jtschuster
Make DomainAssembly create its Assembly in its constructor and re…
bb5c65d
…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.
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@elinor-fung@sirntar
Make DomainAssembly create its Assembly in its constructor and re…
3cf66d4
…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.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

Assignees

@elinor-fungelinor-fung

Labels

area-AssemblyLoader-coreclronly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@elinor-fung@jkotas

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level - #107224
Merged
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Sep 4, 2024
Merged
Make DomainAssembly create its Assembly in its constructor and remove separate allocate load level#107224
elinor-fung merged 4 commits into
dotnet:mainfrom 
elinor-fung:domain-assembly-create
Conversation
@elinor-fung
@elinor-fungelinor-fung
commented
Aug 31, 2024•

edited

Loading

Copy link


Copy Markdown

Member
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
@ghostghost
added
the 
area-AssemblyLoader-coreclr
only use for closed issues
label
Aug 31, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Aug 31, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov

See info in area-owners.md if you want to be subscribed.
@elinor-fung
elinor-fung
marked this pull request as ready for review
August 31, 2024 04:37
jkotas
jkotas
reviewed
Aug 31, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasAug 31, 2024

Copy link


Copy Markdown

Member
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?
@elinor-fungelinor-fungAug 31, 2024

Copy link


Copy Markdown

MemberAuthor
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.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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.
@elinor-fungelinor-fungSep 3, 2024•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
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 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.
@jkotasjkotasSep 4, 2024

Copy link


Copy Markdown

Member
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.
jkotas
jkotas
approved these changes
Sep 1, 2024
View reviewed changes
Comment threadsrc/coreclr/vm/assembly.cpp
Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

Comment threadsrc/coreclr/vm/assembly.cpp
Outdated

@@ -535,8 +518,7 @@ Assembly *Assembly::CreateDynamic(AssemblyBinder* pBinder, NativeAssemblyNamePar

 //Cannot fail after this point
@jkotasjkotasSep 1, 2024

Copy link


Copy Markdown

Member
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); 

is going leak.

This was referenced Sep 4, 2024

Closed


Closed

@elinor-fung
elinor-fung
merged commit ec2f534
into
dotnet:mainSep 4, 2024
@elinor-fung
elinor-fung
deleted the

domain-assembly-create

branch
September 4, 2024 23:34
radekdoulik pushed a commit
to radekdoulik/runtime
that referenced
this pull request
Sep 6, 2024
@elinor-fung@radekdoulik
Make DomainAssembly create its Assembly in its constructor and re…
5d40942
…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.
@elinor-fungelinor-fung
mentioned this pull request
Sep 6, 2024
 Merged

jtschuster pushed a commit
to jtschuster/runtime
that referenced
this pull request
Sep 17, 2024
@elinor-fung@jtschuster
Make DomainAssembly create its Assembly in its constructor and re…
bb5c65d
…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.
sirntar pushed a commit
to sirntar/runtime
that referenced
this pull request
Sep 30, 2024
@elinor-fung@sirntar
Make DomainAssembly create its Assembly in its constructor and re…
3cf66d4
…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.
@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
Oct 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

Assignees

@elinor-fungelinor-fung

Labels

area-AssemblyLoader-coreclronly use for closed issues

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

2 participants

@elinor-fung@jkotas