Skip to content

Initialize Volatile value - #74959

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation
Sep 20, 2022
Merged

Initialize Volatile value#74959
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation

Conversation

@cshung

@cshungcshung commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

In #74363, we changed the code in gc.cpp such that memset (&last_ephemeral_gc_info, 0, sizeof (last_ephemeral_gc_info)); is replaced by last_ephemeral_gc_info = {};

last_ephemeral_gc_info has type Volatile<size_t> and the default constructor of Volatile leave the underlying value uninitialized, this is causing the issue. In the old code, the volatile field was zero-initialized but in the new code, it isn't.

This change added back the default initialization for the volatile field and the issue associated with #74631 is fixed.

All GC code related to this change is reviewed and I confirm volatile is the only issue.

@cshungcshung self-assigned this Sep 1, 2022
@ghost

ghost commented Sep 1, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Adding some instrumentation so that we can debug #74631.

Author:cshung
Assignees:cshung
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/test-instrumentation branch 5 times, most recently from 0cd0d9a to b5250dfCompareSeptember 16, 2022 23:15
@cshung
cshungforce-pushed the public/test-instrumentation branch from b5250df to 6b4aee0CompareSeptember 19, 2022 19:04
@cshungcshung changed the title Test instrumentationInitialize Volatile valueSep 19, 2022
@cshung
cshung requested review from am11 and jkotas and removed request for am11September 19, 2022 19:52
@jkotas

Copy link
Copy Markdown
Member

We also have a copy for local GC here:

https://github.com/dotnet/runtime/blob/main/src/coreclr/gc/env/volatile.h

@cshungcshung linked an issue Sep 19, 2022 that may be closed by this pull request
@cshung
cshung merged commit 001c1b1 into dotnet:mainSep 20, 2022
@cshung
cshung deleted the public/test-instrumentation branch September 20, 2022 05:08
@ghostghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Tests.GCExtendedTests.GetGCMemoryInfo Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd

3 participants

@cshung@jkotas@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Initialize Volatile value by cshung · Pull Request #74959 · dotnet/runtime · GitHub
Skip to content

Initialize Volatile value - #74959

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation
Sep 20, 2022
Merged

Initialize Volatile value#74959
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation

Conversation

@cshung

@cshungcshung commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

In #74363, we changed the code in gc.cpp such that memset (&last_ephemeral_gc_info, 0, sizeof (last_ephemeral_gc_info)); is replaced by last_ephemeral_gc_info = {};

last_ephemeral_gc_info has type Volatile<size_t> and the default constructor of Volatile leave the underlying value uninitialized, this is causing the issue. In the old code, the volatile field was zero-initialized but in the new code, it isn't.

This change added back the default initialization for the volatile field and the issue associated with #74631 is fixed.

All GC code related to this change is reviewed and I confirm volatile is the only issue.

@cshungcshung self-assigned this Sep 1, 2022
@ghost

ghost commented Sep 1, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Adding some instrumentation so that we can debug #74631.

Author:cshung
Assignees:cshung
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/test-instrumentation branch 5 times, most recently from 0cd0d9a to b5250dfCompareSeptember 16, 2022 23:15
@cshung
cshungforce-pushed the public/test-instrumentation branch from b5250df to 6b4aee0CompareSeptember 19, 2022 19:04
@cshungcshung changed the title Test instrumentationInitialize Volatile valueSep 19, 2022
@cshung
cshung requested review from am11 and jkotas and removed request for am11September 19, 2022 19:52
@jkotas

Copy link
Copy Markdown
Member

We also have a copy for local GC here:

https://github.com/dotnet/runtime/blob/main/src/coreclr/gc/env/volatile.h

@cshungcshung linked an issue Sep 19, 2022 that may be closed by this pull request
@cshung
cshung merged commit 001c1b1 into dotnet:mainSep 20, 2022
@cshung
cshung deleted the public/test-instrumentation branch September 20, 2022 05:08
@ghostghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Tests.GCExtendedTests.GetGCMemoryInfo Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd

3 participants

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

Initialize Volatile value - #74959

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation
Sep 20, 2022
Merged

Initialize Volatile value#74959
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation

Conversation

@cshung

@cshungcshung commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

In #74363, we changed the code in gc.cpp such that memset (&last_ephemeral_gc_info, 0, sizeof (last_ephemeral_gc_info)); is replaced by last_ephemeral_gc_info = {};

last_ephemeral_gc_info has type Volatile<size_t> and the default constructor of Volatile leave the underlying value uninitialized, this is causing the issue. In the old code, the volatile field was zero-initialized but in the new code, it isn't.

This change added back the default initialization for the volatile field and the issue associated with #74631 is fixed.

All GC code related to this change is reviewed and I confirm volatile is the only issue.

@cshungcshung self-assigned this Sep 1, 2022
@ghost

ghost commented Sep 1, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Adding some instrumentation so that we can debug #74631.

Author:cshung
Assignees:cshung
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/test-instrumentation branch 5 times, most recently from 0cd0d9a to b5250dfCompareSeptember 16, 2022 23:15
@cshung
cshungforce-pushed the public/test-instrumentation branch from b5250df to 6b4aee0CompareSeptember 19, 2022 19:04
@cshungcshung changed the title Test instrumentationInitialize Volatile valueSep 19, 2022
@cshung
cshung requested review from am11 and jkotas and removed request for am11September 19, 2022 19:52
@jkotas

Copy link
Copy Markdown
Member

We also have a copy for local GC here:

https://github.com/dotnet/runtime/blob/main/src/coreclr/gc/env/volatile.h

@cshungcshung linked an issue Sep 19, 2022 that may be closed by this pull request
@cshung
cshung merged commit 001c1b1 into dotnet:mainSep 20, 2022
@cshung
cshung deleted the public/test-instrumentation branch September 20, 2022 05:08
@ghostghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Tests.GCExtendedTests.GetGCMemoryInfo Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd

3 participants

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

Initialize Volatile value - #74959

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation
Sep 20, 2022
Merged

Initialize Volatile value#74959
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation

Conversation

@cshung

@cshungcshung commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

In #74363, we changed the code in gc.cpp such that memset (&last_ephemeral_gc_info, 0, sizeof (last_ephemeral_gc_info)); is replaced by last_ephemeral_gc_info = {};

last_ephemeral_gc_info has type Volatile<size_t> and the default constructor of Volatile leave the underlying value uninitialized, this is causing the issue. In the old code, the volatile field was zero-initialized but in the new code, it isn't.

This change added back the default initialization for the volatile field and the issue associated with #74631 is fixed.

All GC code related to this change is reviewed and I confirm volatile is the only issue.

@cshungcshung self-assigned this Sep 1, 2022
@ghost

ghost commented Sep 1, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Adding some instrumentation so that we can debug #74631.

Author:cshung
Assignees:cshung
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/test-instrumentation branch 5 times, most recently from 0cd0d9a to b5250dfCompareSeptember 16, 2022 23:15
@cshung
cshungforce-pushed the public/test-instrumentation branch from b5250df to 6b4aee0CompareSeptember 19, 2022 19:04
@cshungcshung changed the title Test instrumentationInitialize Volatile valueSep 19, 2022
@cshung
cshung requested review from am11 and jkotas and removed request for am11September 19, 2022 19:52
@jkotas

Copy link
Copy Markdown
Member

We also have a copy for local GC here:

https://github.com/dotnet/runtime/blob/main/src/coreclr/gc/env/volatile.h

@cshungcshung linked an issue Sep 19, 2022 that may be closed by this pull request
@cshung
cshung merged commit 001c1b1 into dotnet:mainSep 20, 2022
@cshung
cshung deleted the public/test-instrumentation branch September 20, 2022 05:08
@ghostghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Tests.GCExtendedTests.GetGCMemoryInfo Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd

3 participants

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

Initialize Volatile value - #74959

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation
Sep 20, 2022
Merged

Initialize Volatile value#74959
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation

Conversation

@cshung

@cshungcshung commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

In #74363, we changed the code in gc.cpp such that memset (&last_ephemeral_gc_info, 0, sizeof (last_ephemeral_gc_info)); is replaced by last_ephemeral_gc_info = {};

last_ephemeral_gc_info has type Volatile<size_t> and the default constructor of Volatile leave the underlying value uninitialized, this is causing the issue. In the old code, the volatile field was zero-initialized but in the new code, it isn't.

This change added back the default initialization for the volatile field and the issue associated with #74631 is fixed.

All GC code related to this change is reviewed and I confirm volatile is the only issue.

@cshungcshung self-assigned this Sep 1, 2022
@ghost

ghost commented Sep 1, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Adding some instrumentation so that we can debug #74631.

Author:cshung
Assignees:cshung
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/test-instrumentation branch 5 times, most recently from 0cd0d9a to b5250dfCompareSeptember 16, 2022 23:15
@cshung
cshungforce-pushed the public/test-instrumentation branch from b5250df to 6b4aee0CompareSeptember 19, 2022 19:04
@cshungcshung changed the title Test instrumentationInitialize Volatile valueSep 19, 2022
@cshung
cshung requested review from am11 and jkotas and removed request for am11September 19, 2022 19:52
@jkotas

Copy link
Copy Markdown
Member

We also have a copy for local GC here:

https://github.com/dotnet/runtime/blob/main/src/coreclr/gc/env/volatile.h

@cshungcshung linked an issue Sep 19, 2022 that may be closed by this pull request
@cshung
cshung merged commit 001c1b1 into dotnet:mainSep 20, 2022
@cshung
cshung deleted the public/test-instrumentation branch September 20, 2022 05:08
@ghostghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Tests.GCExtendedTests.GetGCMemoryInfo Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd

3 participants

@cshung@jkotas@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Initialize Volatile value by cshung · Pull Request #74959 · dotnet/runtime · GitHub
Skip to content

Initialize Volatile value - #74959

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation
Sep 20, 2022
Merged

Initialize Volatile value#74959
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation

Conversation

@cshung

@cshungcshung commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

In #74363, we changed the code in gc.cpp such that memset (&last_ephemeral_gc_info, 0, sizeof (last_ephemeral_gc_info)); is replaced by last_ephemeral_gc_info = {};

last_ephemeral_gc_info has type Volatile<size_t> and the default constructor of Volatile leave the underlying value uninitialized, this is causing the issue. In the old code, the volatile field was zero-initialized but in the new code, it isn't.

This change added back the default initialization for the volatile field and the issue associated with #74631 is fixed.

All GC code related to this change is reviewed and I confirm volatile is the only issue.

@cshungcshung self-assigned this Sep 1, 2022
@ghost

ghost commented Sep 1, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Adding some instrumentation so that we can debug #74631.

Author:cshung
Assignees:cshung
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/test-instrumentation branch 5 times, most recently from 0cd0d9a to b5250dfCompareSeptember 16, 2022 23:15
@cshung
cshungforce-pushed the public/test-instrumentation branch from b5250df to 6b4aee0CompareSeptember 19, 2022 19:04
@cshungcshung changed the title Test instrumentationInitialize Volatile valueSep 19, 2022
@cshung
cshung requested review from am11 and jkotas and removed request for am11September 19, 2022 19:52
@jkotas

Copy link
Copy Markdown
Member

We also have a copy for local GC here:

https://github.com/dotnet/runtime/blob/main/src/coreclr/gc/env/volatile.h

@cshungcshung linked an issue Sep 19, 2022 that may be closed by this pull request
@cshung
cshung merged commit 001c1b1 into dotnet:mainSep 20, 2022
@cshung
cshung deleted the public/test-instrumentation branch September 20, 2022 05:08
@ghostghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Tests.GCExtendedTests.GetGCMemoryInfo Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd

3 participants

@cshung@jkotas@AaronRobinsonMSFT
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Initialize Volatile value by cshung · Pull Request #74959 · dotnet/runtime · GitHub
Skip to content

Initialize Volatile value - #74959

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation
Sep 20, 2022
Merged

Initialize Volatile value#74959
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation

Conversation

@cshung

@cshungcshung commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

In #74363, we changed the code in gc.cpp such that memset (&last_ephemeral_gc_info, 0, sizeof (last_ephemeral_gc_info)); is replaced by last_ephemeral_gc_info = {};

last_ephemeral_gc_info has type Volatile<size_t> and the default constructor of Volatile leave the underlying value uninitialized, this is causing the issue. In the old code, the volatile field was zero-initialized but in the new code, it isn't.

This change added back the default initialization for the volatile field and the issue associated with #74631 is fixed.

All GC code related to this change is reviewed and I confirm volatile is the only issue.

@cshungcshung self-assigned this Sep 1, 2022
@ghost

ghost commented Sep 1, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Adding some instrumentation so that we can debug #74631.

Author:cshung
Assignees:cshung
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/test-instrumentation branch 5 times, most recently from 0cd0d9a to b5250dfCompareSeptember 16, 2022 23:15
@cshung
cshungforce-pushed the public/test-instrumentation branch from b5250df to 6b4aee0CompareSeptember 19, 2022 19:04
@cshungcshung changed the title Test instrumentationInitialize Volatile valueSep 19, 2022
@cshung
cshung requested review from am11 and jkotas and removed request for am11September 19, 2022 19:52
@jkotas

Copy link
Copy Markdown
Member

We also have a copy for local GC here:

https://github.com/dotnet/runtime/blob/main/src/coreclr/gc/env/volatile.h

@cshungcshung linked an issue Sep 19, 2022 that may be closed by this pull request
@cshung
cshung merged commit 001c1b1 into dotnet:mainSep 20, 2022
@cshung
cshung deleted the public/test-instrumentation branch September 20, 2022 05:08
@ghostghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Tests.GCExtendedTests.GetGCMemoryInfo Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd

3 participants

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

Initialize Volatile value - #74959

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation
Sep 20, 2022
Merged

Initialize Volatile value#74959
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/test-instrumentation

Conversation

@cshung

@cshungcshung commented Sep 1, 2022

Copy link
Copy Markdown
Contributor

In #74363, we changed the code in gc.cpp such that memset (&last_ephemeral_gc_info, 0, sizeof (last_ephemeral_gc_info)); is replaced by last_ephemeral_gc_info = {};

last_ephemeral_gc_info has type Volatile<size_t> and the default constructor of Volatile leave the underlying value uninitialized, this is causing the issue. In the old code, the volatile field was zero-initialized but in the new code, it isn't.

This change added back the default initialization for the volatile field and the issue associated with #74631 is fixed.

All GC code related to this change is reviewed and I confirm volatile is the only issue.

@cshungcshung self-assigned this Sep 1, 2022
@ghost

ghost commented Sep 1, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Issue Details

Adding some instrumentation so that we can debug #74631.

Author:cshung
Assignees:cshung
Labels:

area-GC-coreclr

Milestone:-

@cshung
cshungforce-pushed the public/test-instrumentation branch 5 times, most recently from 0cd0d9a to b5250dfCompareSeptember 16, 2022 23:15
@cshung
cshungforce-pushed the public/test-instrumentation branch from b5250df to 6b4aee0CompareSeptember 19, 2022 19:04
@cshungcshung changed the title Test instrumentationInitialize Volatile valueSep 19, 2022
@cshung
cshung requested review from am11 and jkotas and removed request for am11September 19, 2022 19:52
@jkotas

Copy link
Copy Markdown
Member

We also have a copy for local GC here:

https://github.com/dotnet/runtime/blob/main/src/coreclr/gc/env/volatile.h

@cshungcshung linked an issue Sep 19, 2022 that may be closed by this pull request
@cshung
cshung merged commit 001c1b1 into dotnet:mainSep 20, 2022
@cshung
cshung deleted the public/test-instrumentation branch September 20, 2022 05:08
@ghostghost locked as resolved and limited conversation to collaborators Oct 20, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Tests.GCExtendedTests.GetGCMemoryInfo Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd

3 participants

@cshung@jkotas@AaronRobinsonMSFT