[Mono] Implement eager finalization of WeakReference - #76173

Merged
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref
Sep 29, 2022
Merged

[Mono] Implement eager finalization of WeakReference#76173
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref

Conversation

@filipnavara

@filipnavarafilipnavara commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member labels Sep 26, 2022
@filipnavara
filipnavara marked this pull request as ready for review September 26, 2022 10:27
Comment threadsrc/mono/mono/metadata/gc.c Outdated
@naricc

Copy link
Copy Markdown
Contributor

I'm not sure I understand what is happening here. What does making finalization eager actually mean for WeakRefs? What is the behavior supposed to be?

@VSadov

VSadov commented Sep 26, 2022

Copy link
Copy Markdown
Member

WeakReference types are finalizable types. Weak references could be used in big quantities and that can have bad effects on finalization queue.
The finalization of weak references is very trivial though - it is basically closing the underlying handle. It is generally cheaper to just close the handle when GC finds unreachable weak reference object, instead of posting it to the finalization queue.

The eager approach also has some reliability advantages. Since eager finalization happens when managed threads are stopped, there is no chance of getting into races between finalization and concurrent use of the weak references from other threads.

CoreCLR had eager finalization of weak references for a long time. NativeAOT has added that recently - #75436

Another advantage is that if Mono implements eager finalization, it would be able to use the shared implementation of WeakReference and WeakReference<T> without any ifdefs.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks @VSadov for explaining it better than I would 👍

@lambdageeklambdageek left a comment

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.

lgtm. although i'm not sure we should put weakreference into mono_defaults

Comment threadsrc/mono/mono/metadata/class-internals.h
@BrzVlad

Copy link
Copy Markdown
Member

This change could further be improved by avoiding a useless object allocation. In sgen_finalize_in_range, copy_func is used to promote / mark the object, since it still needs to be alive to be passed as this to the finalizer. This should no longer be needed if it's eagerly finalized.

@VSadov

VSadov commented Sep 27, 2022

Copy link
Copy Markdown
Member

@BrzVlad As I understand the suggestion, we need to move the specialcasing of weak references a bit earlier - from sgen_queue_finalization_entry up into sgen_finalize_in_range. Basically instead of copy_func (&copy, queue); we can just SGEN_HASH_TABLE_FOREACH_REMOVE (TRUE); and continue.

Is this the idea?

@BrzVlad

Copy link
Copy Markdown
Member

@VSadov Yes, that is correct

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for taking care of the last feedback, @VSadov!

@VSadov

Copy link
Copy Markdown
Member

I assume the change addresses the suggestions and once llvmfullaot finishes we can merge the change.

@BrzVlad
BrzVlad merged commit 4287453 into dotnet:mainSep 29, 2022
@filipnavara
filipnavara deleted the mono-eager-weakref branch September 29, 2022 07:08
@uweigand

Copy link
Copy Markdown
Contributor

As of this commit, I'm now seeing failures when running the libs.tests suite on s390x (Mono-based runtime):

 Process terminated. Assertion failed.
WeakReference<T> finalizer should never run

Any suggestions what this could be, or how to further debug?

@BrzVlad

Copy link
Copy Markdown
Member

I remember seeing this assertion locally but I didn't get to investigate it. I'll see if I can repro this week

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@uweigand Do you have any details about which test is failing?

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

@uweigand

Copy link
Copy Markdown
Contributor

Also interesting is that for some unknown reason, I'm not seeing any of these failures in the CI here:
https://dev.azure.com/dnceng-public/public/_build?definitionId=148
(which is doing a cross-build followed by native execution), but I'm seeing the failures all the time when building + testing fully natively on my own s390x machine.

@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@BrzVlad did you have a chance to look into this?

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Should be fixed by #77170

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks a lot!

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@uweigand

Copy link
Copy Markdown
Contributor

Yes, this has fixed the test suite on s390x. Thanks!

@ghostghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara@naricc@VSadov@BrzVlad@uweigand@lambdageek
, '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

[Mono] Implement eager finalization of WeakReference - #76173

Merged
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref
Sep 29, 2022
Merged

[Mono] Implement eager finalization of WeakReference#76173
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref

Conversation

@filipnavara

@filipnavarafilipnavara commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member labels Sep 26, 2022
@filipnavara
filipnavara marked this pull request as ready for review September 26, 2022 10:27
Comment threadsrc/mono/mono/metadata/gc.c Outdated
@naricc

Copy link
Copy Markdown
Contributor

I'm not sure I understand what is happening here. What does making finalization eager actually mean for WeakRefs? What is the behavior supposed to be?

@VSadov

VSadov commented Sep 26, 2022

Copy link
Copy Markdown
Member

WeakReference types are finalizable types. Weak references could be used in big quantities and that can have bad effects on finalization queue.
The finalization of weak references is very trivial though - it is basically closing the underlying handle. It is generally cheaper to just close the handle when GC finds unreachable weak reference object, instead of posting it to the finalization queue.

The eager approach also has some reliability advantages. Since eager finalization happens when managed threads are stopped, there is no chance of getting into races between finalization and concurrent use of the weak references from other threads.

CoreCLR had eager finalization of weak references for a long time. NativeAOT has added that recently - #75436

Another advantage is that if Mono implements eager finalization, it would be able to use the shared implementation of WeakReference and WeakReference<T> without any ifdefs.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks @VSadov for explaining it better than I would 👍

@lambdageeklambdageek left a comment

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.

lgtm. although i'm not sure we should put weakreference into mono_defaults

Comment threadsrc/mono/mono/metadata/class-internals.h
@BrzVlad

Copy link
Copy Markdown
Member

This change could further be improved by avoiding a useless object allocation. In sgen_finalize_in_range, copy_func is used to promote / mark the object, since it still needs to be alive to be passed as this to the finalizer. This should no longer be needed if it's eagerly finalized.

@VSadov

VSadov commented Sep 27, 2022

Copy link
Copy Markdown
Member

@BrzVlad As I understand the suggestion, we need to move the specialcasing of weak references a bit earlier - from sgen_queue_finalization_entry up into sgen_finalize_in_range. Basically instead of copy_func (&copy, queue); we can just SGEN_HASH_TABLE_FOREACH_REMOVE (TRUE); and continue.

Is this the idea?

@BrzVlad

Copy link
Copy Markdown
Member

@VSadov Yes, that is correct

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for taking care of the last feedback, @VSadov!

@VSadov

Copy link
Copy Markdown
Member

I assume the change addresses the suggestions and once llvmfullaot finishes we can merge the change.

@BrzVlad
BrzVlad merged commit 4287453 into dotnet:mainSep 29, 2022
@filipnavara
filipnavara deleted the mono-eager-weakref branch September 29, 2022 07:08
@uweigand

Copy link
Copy Markdown
Contributor

As of this commit, I'm now seeing failures when running the libs.tests suite on s390x (Mono-based runtime):

 Process terminated. Assertion failed.
WeakReference<T> finalizer should never run

Any suggestions what this could be, or how to further debug?

@BrzVlad

Copy link
Copy Markdown
Member

I remember seeing this assertion locally but I didn't get to investigate it. I'll see if I can repro this week

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@uweigand Do you have any details about which test is failing?

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

@uweigand

Copy link
Copy Markdown
Contributor

Also interesting is that for some unknown reason, I'm not seeing any of these failures in the CI here:
https://dev.azure.com/dnceng-public/public/_build?definitionId=148
(which is doing a cross-build followed by native execution), but I'm seeing the failures all the time when building + testing fully natively on my own s390x machine.

@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@BrzVlad did you have a chance to look into this?

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Should be fixed by #77170

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks a lot!

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@uweigand

Copy link
Copy Markdown
Contributor

Yes, this has fixed the test suite on s390x. Thanks!

@ghostghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara@naricc@VSadov@BrzVlad@uweigand@lambdageek
, '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

[Mono] Implement eager finalization of WeakReference - #76173

Merged
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref
Sep 29, 2022
Merged

[Mono] Implement eager finalization of WeakReference#76173
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref

Conversation

@filipnavara

@filipnavarafilipnavara commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member labels Sep 26, 2022
@filipnavara
filipnavara marked this pull request as ready for review September 26, 2022 10:27
Comment threadsrc/mono/mono/metadata/gc.c Outdated
@naricc

Copy link
Copy Markdown
Contributor

I'm not sure I understand what is happening here. What does making finalization eager actually mean for WeakRefs? What is the behavior supposed to be?

@VSadov

VSadov commented Sep 26, 2022

Copy link
Copy Markdown
Member

WeakReference types are finalizable types. Weak references could be used in big quantities and that can have bad effects on finalization queue.
The finalization of weak references is very trivial though - it is basically closing the underlying handle. It is generally cheaper to just close the handle when GC finds unreachable weak reference object, instead of posting it to the finalization queue.

The eager approach also has some reliability advantages. Since eager finalization happens when managed threads are stopped, there is no chance of getting into races between finalization and concurrent use of the weak references from other threads.

CoreCLR had eager finalization of weak references for a long time. NativeAOT has added that recently - #75436

Another advantage is that if Mono implements eager finalization, it would be able to use the shared implementation of WeakReference and WeakReference<T> without any ifdefs.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks @VSadov for explaining it better than I would 👍

@lambdageeklambdageek left a comment

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.

lgtm. although i'm not sure we should put weakreference into mono_defaults

Comment threadsrc/mono/mono/metadata/class-internals.h
@BrzVlad

Copy link
Copy Markdown
Member

This change could further be improved by avoiding a useless object allocation. In sgen_finalize_in_range, copy_func is used to promote / mark the object, since it still needs to be alive to be passed as this to the finalizer. This should no longer be needed if it's eagerly finalized.

@VSadov

VSadov commented Sep 27, 2022

Copy link
Copy Markdown
Member

@BrzVlad As I understand the suggestion, we need to move the specialcasing of weak references a bit earlier - from sgen_queue_finalization_entry up into sgen_finalize_in_range. Basically instead of copy_func (&copy, queue); we can just SGEN_HASH_TABLE_FOREACH_REMOVE (TRUE); and continue.

Is this the idea?

@BrzVlad

Copy link
Copy Markdown
Member

@VSadov Yes, that is correct

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for taking care of the last feedback, @VSadov!

@VSadov

Copy link
Copy Markdown
Member

I assume the change addresses the suggestions and once llvmfullaot finishes we can merge the change.

@BrzVlad
BrzVlad merged commit 4287453 into dotnet:mainSep 29, 2022
@filipnavara
filipnavara deleted the mono-eager-weakref branch September 29, 2022 07:08
@uweigand

Copy link
Copy Markdown
Contributor

As of this commit, I'm now seeing failures when running the libs.tests suite on s390x (Mono-based runtime):

 Process terminated. Assertion failed.
WeakReference<T> finalizer should never run

Any suggestions what this could be, or how to further debug?

@BrzVlad

Copy link
Copy Markdown
Member

I remember seeing this assertion locally but I didn't get to investigate it. I'll see if I can repro this week

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@uweigand Do you have any details about which test is failing?

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

@uweigand

Copy link
Copy Markdown
Contributor

Also interesting is that for some unknown reason, I'm not seeing any of these failures in the CI here:
https://dev.azure.com/dnceng-public/public/_build?definitionId=148
(which is doing a cross-build followed by native execution), but I'm seeing the failures all the time when building + testing fully natively on my own s390x machine.

@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@BrzVlad did you have a chance to look into this?

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Should be fixed by #77170

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks a lot!

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@uweigand

Copy link
Copy Markdown
Contributor

Yes, this has fixed the test suite on s390x. Thanks!

@ghostghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara@naricc@VSadov@BrzVlad@uweigand@lambdageek
, '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

[Mono] Implement eager finalization of WeakReference - #76173

Merged
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref
Sep 29, 2022
Merged

[Mono] Implement eager finalization of WeakReference#76173
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref

Conversation

@filipnavara

@filipnavarafilipnavara commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member labels Sep 26, 2022
@filipnavara
filipnavara marked this pull request as ready for review September 26, 2022 10:27
Comment threadsrc/mono/mono/metadata/gc.c Outdated
@naricc

Copy link
Copy Markdown
Contributor

I'm not sure I understand what is happening here. What does making finalization eager actually mean for WeakRefs? What is the behavior supposed to be?

@VSadov

VSadov commented Sep 26, 2022

Copy link
Copy Markdown
Member

WeakReference types are finalizable types. Weak references could be used in big quantities and that can have bad effects on finalization queue.
The finalization of weak references is very trivial though - it is basically closing the underlying handle. It is generally cheaper to just close the handle when GC finds unreachable weak reference object, instead of posting it to the finalization queue.

The eager approach also has some reliability advantages. Since eager finalization happens when managed threads are stopped, there is no chance of getting into races between finalization and concurrent use of the weak references from other threads.

CoreCLR had eager finalization of weak references for a long time. NativeAOT has added that recently - #75436

Another advantage is that if Mono implements eager finalization, it would be able to use the shared implementation of WeakReference and WeakReference<T> without any ifdefs.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks @VSadov for explaining it better than I would 👍

@lambdageeklambdageek left a comment

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.

lgtm. although i'm not sure we should put weakreference into mono_defaults

Comment threadsrc/mono/mono/metadata/class-internals.h
@BrzVlad

Copy link
Copy Markdown
Member

This change could further be improved by avoiding a useless object allocation. In sgen_finalize_in_range, copy_func is used to promote / mark the object, since it still needs to be alive to be passed as this to the finalizer. This should no longer be needed if it's eagerly finalized.

@VSadov

VSadov commented Sep 27, 2022

Copy link
Copy Markdown
Member

@BrzVlad As I understand the suggestion, we need to move the specialcasing of weak references a bit earlier - from sgen_queue_finalization_entry up into sgen_finalize_in_range. Basically instead of copy_func (&copy, queue); we can just SGEN_HASH_TABLE_FOREACH_REMOVE (TRUE); and continue.

Is this the idea?

@BrzVlad

Copy link
Copy Markdown
Member

@VSadov Yes, that is correct

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for taking care of the last feedback, @VSadov!

@VSadov

Copy link
Copy Markdown
Member

I assume the change addresses the suggestions and once llvmfullaot finishes we can merge the change.

@BrzVlad
BrzVlad merged commit 4287453 into dotnet:mainSep 29, 2022
@filipnavara
filipnavara deleted the mono-eager-weakref branch September 29, 2022 07:08
@uweigand

Copy link
Copy Markdown
Contributor

As of this commit, I'm now seeing failures when running the libs.tests suite on s390x (Mono-based runtime):

 Process terminated. Assertion failed.
WeakReference<T> finalizer should never run

Any suggestions what this could be, or how to further debug?

@BrzVlad

Copy link
Copy Markdown
Member

I remember seeing this assertion locally but I didn't get to investigate it. I'll see if I can repro this week

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@uweigand Do you have any details about which test is failing?

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

@uweigand

Copy link
Copy Markdown
Contributor

Also interesting is that for some unknown reason, I'm not seeing any of these failures in the CI here:
https://dev.azure.com/dnceng-public/public/_build?definitionId=148
(which is doing a cross-build followed by native execution), but I'm seeing the failures all the time when building + testing fully natively on my own s390x machine.

@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@BrzVlad did you have a chance to look into this?

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Should be fixed by #77170

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks a lot!

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@uweigand

Copy link
Copy Markdown
Contributor

Yes, this has fixed the test suite on s390x. Thanks!

@ghostghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara@naricc@VSadov@BrzVlad@uweigand@lambdageek
, '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

[Mono] Implement eager finalization of WeakReference - #76173

Merged
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref
Sep 29, 2022
Merged

[Mono] Implement eager finalization of WeakReference#76173
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref

Conversation

@filipnavara

@filipnavarafilipnavara commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member labels Sep 26, 2022
@filipnavara
filipnavara marked this pull request as ready for review September 26, 2022 10:27
Comment threadsrc/mono/mono/metadata/gc.c Outdated
@naricc

Copy link
Copy Markdown
Contributor

I'm not sure I understand what is happening here. What does making finalization eager actually mean for WeakRefs? What is the behavior supposed to be?

@VSadov

VSadov commented Sep 26, 2022

Copy link
Copy Markdown
Member

WeakReference types are finalizable types. Weak references could be used in big quantities and that can have bad effects on finalization queue.
The finalization of weak references is very trivial though - it is basically closing the underlying handle. It is generally cheaper to just close the handle when GC finds unreachable weak reference object, instead of posting it to the finalization queue.

The eager approach also has some reliability advantages. Since eager finalization happens when managed threads are stopped, there is no chance of getting into races between finalization and concurrent use of the weak references from other threads.

CoreCLR had eager finalization of weak references for a long time. NativeAOT has added that recently - #75436

Another advantage is that if Mono implements eager finalization, it would be able to use the shared implementation of WeakReference and WeakReference<T> without any ifdefs.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks @VSadov for explaining it better than I would 👍

@lambdageeklambdageek left a comment

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.

lgtm. although i'm not sure we should put weakreference into mono_defaults

Comment threadsrc/mono/mono/metadata/class-internals.h
@BrzVlad

Copy link
Copy Markdown
Member

This change could further be improved by avoiding a useless object allocation. In sgen_finalize_in_range, copy_func is used to promote / mark the object, since it still needs to be alive to be passed as this to the finalizer. This should no longer be needed if it's eagerly finalized.

@VSadov

VSadov commented Sep 27, 2022

Copy link
Copy Markdown
Member

@BrzVlad As I understand the suggestion, we need to move the specialcasing of weak references a bit earlier - from sgen_queue_finalization_entry up into sgen_finalize_in_range. Basically instead of copy_func (&copy, queue); we can just SGEN_HASH_TABLE_FOREACH_REMOVE (TRUE); and continue.

Is this the idea?

@BrzVlad

Copy link
Copy Markdown
Member

@VSadov Yes, that is correct

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for taking care of the last feedback, @VSadov!

@VSadov

Copy link
Copy Markdown
Member

I assume the change addresses the suggestions and once llvmfullaot finishes we can merge the change.

@BrzVlad
BrzVlad merged commit 4287453 into dotnet:mainSep 29, 2022
@filipnavara
filipnavara deleted the mono-eager-weakref branch September 29, 2022 07:08
@uweigand

Copy link
Copy Markdown
Contributor

As of this commit, I'm now seeing failures when running the libs.tests suite on s390x (Mono-based runtime):

 Process terminated. Assertion failed.
WeakReference<T> finalizer should never run

Any suggestions what this could be, or how to further debug?

@BrzVlad

Copy link
Copy Markdown
Member

I remember seeing this assertion locally but I didn't get to investigate it. I'll see if I can repro this week

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@uweigand Do you have any details about which test is failing?

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

@uweigand

Copy link
Copy Markdown
Contributor

Also interesting is that for some unknown reason, I'm not seeing any of these failures in the CI here:
https://dev.azure.com/dnceng-public/public/_build?definitionId=148
(which is doing a cross-build followed by native execution), but I'm seeing the failures all the time when building + testing fully natively on my own s390x machine.

@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@BrzVlad did you have a chance to look into this?

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Should be fixed by #77170

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks a lot!

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@uweigand

Copy link
Copy Markdown
Contributor

Yes, this has fixed the test suite on s390x. Thanks!

@ghostghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara@naricc@VSadov@BrzVlad@uweigand@lambdageek
, '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

[Mono] Implement eager finalization of WeakReference - #76173

Merged
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref
Sep 29, 2022
Merged

[Mono] Implement eager finalization of WeakReference#76173
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref

Conversation

@filipnavara

@filipnavarafilipnavara commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member labels Sep 26, 2022
@filipnavara
filipnavara marked this pull request as ready for review September 26, 2022 10:27
Comment threadsrc/mono/mono/metadata/gc.c Outdated
@naricc

Copy link
Copy Markdown
Contributor

I'm not sure I understand what is happening here. What does making finalization eager actually mean for WeakRefs? What is the behavior supposed to be?

@VSadov

VSadov commented Sep 26, 2022

Copy link
Copy Markdown
Member

WeakReference types are finalizable types. Weak references could be used in big quantities and that can have bad effects on finalization queue.
The finalization of weak references is very trivial though - it is basically closing the underlying handle. It is generally cheaper to just close the handle when GC finds unreachable weak reference object, instead of posting it to the finalization queue.

The eager approach also has some reliability advantages. Since eager finalization happens when managed threads are stopped, there is no chance of getting into races between finalization and concurrent use of the weak references from other threads.

CoreCLR had eager finalization of weak references for a long time. NativeAOT has added that recently - #75436

Another advantage is that if Mono implements eager finalization, it would be able to use the shared implementation of WeakReference and WeakReference<T> without any ifdefs.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks @VSadov for explaining it better than I would 👍

@lambdageeklambdageek left a comment

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.

lgtm. although i'm not sure we should put weakreference into mono_defaults

Comment threadsrc/mono/mono/metadata/class-internals.h
@BrzVlad

Copy link
Copy Markdown
Member

This change could further be improved by avoiding a useless object allocation. In sgen_finalize_in_range, copy_func is used to promote / mark the object, since it still needs to be alive to be passed as this to the finalizer. This should no longer be needed if it's eagerly finalized.

@VSadov

VSadov commented Sep 27, 2022

Copy link
Copy Markdown
Member

@BrzVlad As I understand the suggestion, we need to move the specialcasing of weak references a bit earlier - from sgen_queue_finalization_entry up into sgen_finalize_in_range. Basically instead of copy_func (&copy, queue); we can just SGEN_HASH_TABLE_FOREACH_REMOVE (TRUE); and continue.

Is this the idea?

@BrzVlad

Copy link
Copy Markdown
Member

@VSadov Yes, that is correct

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for taking care of the last feedback, @VSadov!

@VSadov

Copy link
Copy Markdown
Member

I assume the change addresses the suggestions and once llvmfullaot finishes we can merge the change.

@BrzVlad
BrzVlad merged commit 4287453 into dotnet:mainSep 29, 2022
@filipnavara
filipnavara deleted the mono-eager-weakref branch September 29, 2022 07:08
@uweigand

Copy link
Copy Markdown
Contributor

As of this commit, I'm now seeing failures when running the libs.tests suite on s390x (Mono-based runtime):

 Process terminated. Assertion failed.
WeakReference<T> finalizer should never run

Any suggestions what this could be, or how to further debug?

@BrzVlad

Copy link
Copy Markdown
Member

I remember seeing this assertion locally but I didn't get to investigate it. I'll see if I can repro this week

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@uweigand Do you have any details about which test is failing?

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

@uweigand

Copy link
Copy Markdown
Contributor

Also interesting is that for some unknown reason, I'm not seeing any of these failures in the CI here:
https://dev.azure.com/dnceng-public/public/_build?definitionId=148
(which is doing a cross-build followed by native execution), but I'm seeing the failures all the time when building + testing fully natively on my own s390x machine.

@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@BrzVlad did you have a chance to look into this?

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Should be fixed by #77170

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks a lot!

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@uweigand

Copy link
Copy Markdown
Contributor

Yes, this has fixed the test suite on s390x. Thanks!

@ghostghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara@naricc@VSadov@BrzVlad@uweigand@lambdageek
, '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

[Mono] Implement eager finalization of WeakReference - #76173

Merged
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref
Sep 29, 2022
Merged

[Mono] Implement eager finalization of WeakReference#76173
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref

Conversation

@filipnavara

@filipnavarafilipnavara commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member labels Sep 26, 2022
@filipnavara
filipnavara marked this pull request as ready for review September 26, 2022 10:27
Comment threadsrc/mono/mono/metadata/gc.c Outdated
@naricc

Copy link
Copy Markdown
Contributor

I'm not sure I understand what is happening here. What does making finalization eager actually mean for WeakRefs? What is the behavior supposed to be?

@VSadov

VSadov commented Sep 26, 2022

Copy link
Copy Markdown
Member

WeakReference types are finalizable types. Weak references could be used in big quantities and that can have bad effects on finalization queue.
The finalization of weak references is very trivial though - it is basically closing the underlying handle. It is generally cheaper to just close the handle when GC finds unreachable weak reference object, instead of posting it to the finalization queue.

The eager approach also has some reliability advantages. Since eager finalization happens when managed threads are stopped, there is no chance of getting into races between finalization and concurrent use of the weak references from other threads.

CoreCLR had eager finalization of weak references for a long time. NativeAOT has added that recently - #75436

Another advantage is that if Mono implements eager finalization, it would be able to use the shared implementation of WeakReference and WeakReference<T> without any ifdefs.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks @VSadov for explaining it better than I would 👍

@lambdageeklambdageek left a comment

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.

lgtm. although i'm not sure we should put weakreference into mono_defaults

Comment threadsrc/mono/mono/metadata/class-internals.h
@BrzVlad

Copy link
Copy Markdown
Member

This change could further be improved by avoiding a useless object allocation. In sgen_finalize_in_range, copy_func is used to promote / mark the object, since it still needs to be alive to be passed as this to the finalizer. This should no longer be needed if it's eagerly finalized.

@VSadov

VSadov commented Sep 27, 2022

Copy link
Copy Markdown
Member

@BrzVlad As I understand the suggestion, we need to move the specialcasing of weak references a bit earlier - from sgen_queue_finalization_entry up into sgen_finalize_in_range. Basically instead of copy_func (&copy, queue); we can just SGEN_HASH_TABLE_FOREACH_REMOVE (TRUE); and continue.

Is this the idea?

@BrzVlad

Copy link
Copy Markdown
Member

@VSadov Yes, that is correct

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for taking care of the last feedback, @VSadov!

@VSadov

Copy link
Copy Markdown
Member

I assume the change addresses the suggestions and once llvmfullaot finishes we can merge the change.

@BrzVlad
BrzVlad merged commit 4287453 into dotnet:mainSep 29, 2022
@filipnavara
filipnavara deleted the mono-eager-weakref branch September 29, 2022 07:08
@uweigand

Copy link
Copy Markdown
Contributor

As of this commit, I'm now seeing failures when running the libs.tests suite on s390x (Mono-based runtime):

 Process terminated. Assertion failed.
WeakReference<T> finalizer should never run

Any suggestions what this could be, or how to further debug?

@BrzVlad

Copy link
Copy Markdown
Member

I remember seeing this assertion locally but I didn't get to investigate it. I'll see if I can repro this week

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@uweigand Do you have any details about which test is failing?

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

@uweigand

Copy link
Copy Markdown
Contributor

Also interesting is that for some unknown reason, I'm not seeing any of these failures in the CI here:
https://dev.azure.com/dnceng-public/public/_build?definitionId=148
(which is doing a cross-build followed by native execution), but I'm seeing the failures all the time when building + testing fully natively on my own s390x machine.

@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@BrzVlad did you have a chance to look into this?

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Should be fixed by #77170

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks a lot!

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@uweigand

Copy link
Copy Markdown
Contributor

Yes, this has fixed the test suite on s390x. Thanks!

@ghostghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara@naricc@VSadov@BrzVlad@uweigand@lambdageek
, '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

[Mono] Implement eager finalization of WeakReference - #76173

Merged
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref
Sep 29, 2022
Merged

[Mono] Implement eager finalization of WeakReference#76173
BrzVlad merged 3 commits into
dotnet:mainfrom
filipnavara:mono-eager-weakref

Conversation

@filipnavara

@filipnavarafilipnavara commented Sep 26, 2022

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added area-VM-meta-mono community-contribution Indicates that the PR has been added by a community member labels Sep 26, 2022
@filipnavara
filipnavara marked this pull request as ready for review September 26, 2022 10:27
Comment threadsrc/mono/mono/metadata/gc.c Outdated
@naricc

Copy link
Copy Markdown
Contributor

I'm not sure I understand what is happening here. What does making finalization eager actually mean for WeakRefs? What is the behavior supposed to be?

@VSadov

VSadov commented Sep 26, 2022

Copy link
Copy Markdown
Member

WeakReference types are finalizable types. Weak references could be used in big quantities and that can have bad effects on finalization queue.
The finalization of weak references is very trivial though - it is basically closing the underlying handle. It is generally cheaper to just close the handle when GC finds unreachable weak reference object, instead of posting it to the finalization queue.

The eager approach also has some reliability advantages. Since eager finalization happens when managed threads are stopped, there is no chance of getting into races between finalization and concurrent use of the weak references from other threads.

CoreCLR had eager finalization of weak references for a long time. NativeAOT has added that recently - #75436

Another advantage is that if Mono implements eager finalization, it would be able to use the shared implementation of WeakReference and WeakReference<T> without any ifdefs.

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks @VSadov for explaining it better than I would 👍

@lambdageeklambdageek left a comment

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.

lgtm. although i'm not sure we should put weakreference into mono_defaults

Comment threadsrc/mono/mono/metadata/class-internals.h
@BrzVlad

Copy link
Copy Markdown
Member

This change could further be improved by avoiding a useless object allocation. In sgen_finalize_in_range, copy_func is used to promote / mark the object, since it still needs to be alive to be passed as this to the finalizer. This should no longer be needed if it's eagerly finalized.

@VSadov

VSadov commented Sep 27, 2022

Copy link
Copy Markdown
Member

@BrzVlad As I understand the suggestion, we need to move the specialcasing of weak references a bit earlier - from sgen_queue_finalization_entry up into sgen_finalize_in_range. Basically instead of copy_func (&copy, queue); we can just SGEN_HASH_TABLE_FOREACH_REMOVE (TRUE); and continue.

Is this the idea?

@BrzVlad

Copy link
Copy Markdown
Member

@VSadov Yes, that is correct

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks for taking care of the last feedback, @VSadov!

@VSadov

Copy link
Copy Markdown
Member

I assume the change addresses the suggestions and once llvmfullaot finishes we can merge the change.

@BrzVlad
BrzVlad merged commit 4287453 into dotnet:mainSep 29, 2022
@filipnavara
filipnavara deleted the mono-eager-weakref branch September 29, 2022 07:08
@uweigand

Copy link
Copy Markdown
Contributor

As of this commit, I'm now seeing failures when running the libs.tests suite on s390x (Mono-based runtime):

 Process terminated. Assertion failed.
WeakReference<T> finalizer should never run

Any suggestions what this could be, or how to further debug?

@BrzVlad

Copy link
Copy Markdown
Member

I remember seeing this assertion locally but I didn't get to investigate it. I'll see if I can repro this week

@filipnavara

Copy link
Copy Markdown
MemberAuthor

@uweigand Do you have any details about which test is failing?

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

@uweigand

Copy link
Copy Markdown
Contributor

Also interesting is that for some unknown reason, I'm not seeing any of these failures in the CI here:
https://dev.azure.com/dnceng-public/public/_build?definitionId=148
(which is doing a cross-build followed by native execution), but I'm seeing the failures all the time when building + testing fully natively on my own s390x machine.

@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@uweigand

Copy link
Copy Markdown
Contributor

@uweigand Do you have any details about which test is failing?

It varies, I've seen multiple different tests failing. One that seems to fail most of the time is System.IO.FileSystem.Watcher.Tests.

I can reproduce, will take a look later

@BrzVlad did you have a chance to look into this?

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@BrzVlad

Copy link
Copy Markdown
Member

@uweigand Should be fixed by #77170

@filipnavara

Copy link
Copy Markdown
MemberAuthor

Thanks a lot!

simonrozsival added a commit to simonrozsival/runtime that referenced this pull request Oct 19, 2022
@uweigand

Copy link
Copy Markdown
Contributor

Yes, this has fixed the test suite on s390x. Thanks!

@ghostghost locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-meta-monocommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@filipnavara@naricc@VSadov@BrzVlad@uweigand@lambdageek