[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS - #100122

Merged
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging
Apr 30, 2024
Merged

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS#100122
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Mar 22, 2024

Copy link
Copy Markdown
Member

Backport of #92185 to release/8.0-staging

Customer Impact

  • Customer reported
  • Found internally

This has been reported by 2 customers. The first report was in September 2023 by @ForNeVeR in #91958, and they also provided the fix and tests. The issue was reported again by @huntharo in February with #98121. In that issue, @huntharo requested that this fix be backported to 8.0. As @huntharo stated:

On Mac OS X, with Apple Silicon, the System.Diagnostics.Process class returns incorrect (substantially underreported - ms when it should be seconds) CPU usage times for the process (and probably for the thread-level stats too).

I discovered this while writing a function to tune the max # of worker threads in the ThreadPool as it reduces CPU usage from 700% to 120% while increasing throughput for my project (pwrdrvr/lambda-dispatch#109). I was always computing that 0 threads would be needed because CPU usage was only going up by 20 ms per 5 seconds despite using 700% CPU.

Regression

  • Yes
  • No

While this isn't a product regression, it's behavior that became incorrect with Apple Silicon.

Testing

@ForNeVeR included a new test with their fix that is part of this backport. The test performs a native call and compares the API's results against the native results. As stated in the original PR:

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

Risk

Low. This is a targeted fix that only affects PrivilegedProcessorTime, TotalProcessorTime, and UserProcessorTime on macOS. While the fix had been in main since September, a race condition bug in the original fix was found during the review of this backport. That bug was fixed in main on March 25 and cherry-picked to this backport.

The automatic backport of this fix failed due to a merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs. That conflict arose because the preceding line to the change here was removed in 20a3350. Otherwise, the rest of the merge was clean.

@jeffhandleyjeffhandley added Servicing-consider Issue for next servicing release review area-System.Diagnostics.Process os-mac-os-x macOS aka OSX labels Mar 22, 2024
@jeffhandleyjeffhandley added this to the 8.0.x milestone Mar 22, 2024
@jeffhandleyjeffhandley self-assigned this Mar 22, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@adamsitnikadamsitnik 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, thank you for backporting the fix @jeffhandley !

@jeffhandleyjeffhandley removed the Servicing-consider Issue for next servicing release review label Mar 26, 2024
Without this fix, it was possible for another thread to see an incorrect
(zero) value of s_timeBase_numer because of a race condition.
@jozkeejozkee added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2024
@jozkee

Copy link
Copy Markdown
Member

@jeffhandley should this be marked as servicing consider now that #100122 (comment) got resolved? If so, do we need to email tactics?

@carlossanlop

Copy link
Copy Markdown
Contributor

@jozkee I didn't find a Tactics email, so assuming this is ready (pending @jeffhandley 's or @adamsitnik 's confirmation), then yes, please send an email to Tactics and add the servicing-consider label.

@jeffhandleyjeffhandley added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@jeffhandley
jeffhandley requested a review from jkotasApril 30, 2024 18:51
@jeffhandleyjeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@jeffhandley

Copy link
Copy Markdown
MemberAuthor

This was approved in email and needs an approval for merge. @jkotas -- you can ignore the re-review request as this can now be merged.

@jeffhandley
jeffhandley merged commit 8acc1b5 into dotnet:release/8.0-stagingApr 30, 2024
@jeffhandley
jeffhandley deleted the jeffhandley/backport-92185/8.0-staging branch April 30, 2024 18:55
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Processos-mac-os-xmacOS aka OSXServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS - #100122

Merged
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging
Apr 30, 2024
Merged

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS#100122
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Mar 22, 2024

Copy link
Copy Markdown
Member

Backport of #92185 to release/8.0-staging

Customer Impact

  • Customer reported
  • Found internally

This has been reported by 2 customers. The first report was in September 2023 by @ForNeVeR in #91958, and they also provided the fix and tests. The issue was reported again by @huntharo in February with #98121. In that issue, @huntharo requested that this fix be backported to 8.0. As @huntharo stated:

On Mac OS X, with Apple Silicon, the System.Diagnostics.Process class returns incorrect (substantially underreported - ms when it should be seconds) CPU usage times for the process (and probably for the thread-level stats too).

I discovered this while writing a function to tune the max # of worker threads in the ThreadPool as it reduces CPU usage from 700% to 120% while increasing throughput for my project (pwrdrvr/lambda-dispatch#109). I was always computing that 0 threads would be needed because CPU usage was only going up by 20 ms per 5 seconds despite using 700% CPU.

Regression

  • Yes
  • No

While this isn't a product regression, it's behavior that became incorrect with Apple Silicon.

Testing

@ForNeVeR included a new test with their fix that is part of this backport. The test performs a native call and compares the API's results against the native results. As stated in the original PR:

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

Risk

Low. This is a targeted fix that only affects PrivilegedProcessorTime, TotalProcessorTime, and UserProcessorTime on macOS. While the fix had been in main since September, a race condition bug in the original fix was found during the review of this backport. That bug was fixed in main on March 25 and cherry-picked to this backport.

The automatic backport of this fix failed due to a merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs. That conflict arose because the preceding line to the change here was removed in 20a3350. Otherwise, the rest of the merge was clean.

@jeffhandleyjeffhandley added Servicing-consider Issue for next servicing release review area-System.Diagnostics.Process os-mac-os-x macOS aka OSX labels Mar 22, 2024
@jeffhandleyjeffhandley added this to the 8.0.x milestone Mar 22, 2024
@jeffhandleyjeffhandley self-assigned this Mar 22, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@adamsitnikadamsitnik 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, thank you for backporting the fix @jeffhandley !

@jeffhandleyjeffhandley removed the Servicing-consider Issue for next servicing release review label Mar 26, 2024
Without this fix, it was possible for another thread to see an incorrect
(zero) value of s_timeBase_numer because of a race condition.
@jozkeejozkee added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2024
@jozkee

Copy link
Copy Markdown
Member

@jeffhandley should this be marked as servicing consider now that #100122 (comment) got resolved? If so, do we need to email tactics?

@carlossanlop

Copy link
Copy Markdown
Contributor

@jozkee I didn't find a Tactics email, so assuming this is ready (pending @jeffhandley 's or @adamsitnik 's confirmation), then yes, please send an email to Tactics and add the servicing-consider label.

@jeffhandleyjeffhandley added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@jeffhandley
jeffhandley requested a review from jkotasApril 30, 2024 18:51
@jeffhandleyjeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@jeffhandley

Copy link
Copy Markdown
MemberAuthor

This was approved in email and needs an approval for merge. @jkotas -- you can ignore the re-review request as this can now be merged.

@jeffhandley
jeffhandley merged commit 8acc1b5 into dotnet:release/8.0-stagingApr 30, 2024
@jeffhandley
jeffhandley deleted the jeffhandley/backport-92185/8.0-staging branch April 30, 2024 18:55
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Processos-mac-os-xmacOS aka OSXServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS - #100122

Merged
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging
Apr 30, 2024
Merged

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS#100122
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Mar 22, 2024

Copy link
Copy Markdown
Member

Backport of #92185 to release/8.0-staging

Customer Impact

  • Customer reported
  • Found internally

This has been reported by 2 customers. The first report was in September 2023 by @ForNeVeR in #91958, and they also provided the fix and tests. The issue was reported again by @huntharo in February with #98121. In that issue, @huntharo requested that this fix be backported to 8.0. As @huntharo stated:

On Mac OS X, with Apple Silicon, the System.Diagnostics.Process class returns incorrect (substantially underreported - ms when it should be seconds) CPU usage times for the process (and probably for the thread-level stats too).

I discovered this while writing a function to tune the max # of worker threads in the ThreadPool as it reduces CPU usage from 700% to 120% while increasing throughput for my project (pwrdrvr/lambda-dispatch#109). I was always computing that 0 threads would be needed because CPU usage was only going up by 20 ms per 5 seconds despite using 700% CPU.

Regression

  • Yes
  • No

While this isn't a product regression, it's behavior that became incorrect with Apple Silicon.

Testing

@ForNeVeR included a new test with their fix that is part of this backport. The test performs a native call and compares the API's results against the native results. As stated in the original PR:

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

Risk

Low. This is a targeted fix that only affects PrivilegedProcessorTime, TotalProcessorTime, and UserProcessorTime on macOS. While the fix had been in main since September, a race condition bug in the original fix was found during the review of this backport. That bug was fixed in main on March 25 and cherry-picked to this backport.

The automatic backport of this fix failed due to a merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs. That conflict arose because the preceding line to the change here was removed in 20a3350. Otherwise, the rest of the merge was clean.

@jeffhandleyjeffhandley added Servicing-consider Issue for next servicing release review area-System.Diagnostics.Process os-mac-os-x macOS aka OSX labels Mar 22, 2024
@jeffhandleyjeffhandley added this to the 8.0.x milestone Mar 22, 2024
@jeffhandleyjeffhandley self-assigned this Mar 22, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@adamsitnikadamsitnik 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, thank you for backporting the fix @jeffhandley !

@jeffhandleyjeffhandley removed the Servicing-consider Issue for next servicing release review label Mar 26, 2024
Without this fix, it was possible for another thread to see an incorrect
(zero) value of s_timeBase_numer because of a race condition.
@jozkeejozkee added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2024
@jozkee

Copy link
Copy Markdown
Member

@jeffhandley should this be marked as servicing consider now that #100122 (comment) got resolved? If so, do we need to email tactics?

@carlossanlop

Copy link
Copy Markdown
Contributor

@jozkee I didn't find a Tactics email, so assuming this is ready (pending @jeffhandley 's or @adamsitnik 's confirmation), then yes, please send an email to Tactics and add the servicing-consider label.

@jeffhandleyjeffhandley added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@jeffhandley
jeffhandley requested a review from jkotasApril 30, 2024 18:51
@jeffhandleyjeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@jeffhandley

Copy link
Copy Markdown
MemberAuthor

This was approved in email and needs an approval for merge. @jkotas -- you can ignore the re-review request as this can now be merged.

@jeffhandley
jeffhandley merged commit 8acc1b5 into dotnet:release/8.0-stagingApr 30, 2024
@jeffhandley
jeffhandley deleted the jeffhandley/backport-92185/8.0-staging branch April 30, 2024 18:55
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Processos-mac-os-xmacOS aka OSXServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS - #100122

Merged
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging
Apr 30, 2024
Merged

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS#100122
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Mar 22, 2024

Copy link
Copy Markdown
Member

Backport of #92185 to release/8.0-staging

Customer Impact

  • Customer reported
  • Found internally

This has been reported by 2 customers. The first report was in September 2023 by @ForNeVeR in #91958, and they also provided the fix and tests. The issue was reported again by @huntharo in February with #98121. In that issue, @huntharo requested that this fix be backported to 8.0. As @huntharo stated:

On Mac OS X, with Apple Silicon, the System.Diagnostics.Process class returns incorrect (substantially underreported - ms when it should be seconds) CPU usage times for the process (and probably for the thread-level stats too).

I discovered this while writing a function to tune the max # of worker threads in the ThreadPool as it reduces CPU usage from 700% to 120% while increasing throughput for my project (pwrdrvr/lambda-dispatch#109). I was always computing that 0 threads would be needed because CPU usage was only going up by 20 ms per 5 seconds despite using 700% CPU.

Regression

  • Yes
  • No

While this isn't a product regression, it's behavior that became incorrect with Apple Silicon.

Testing

@ForNeVeR included a new test with their fix that is part of this backport. The test performs a native call and compares the API's results against the native results. As stated in the original PR:

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

Risk

Low. This is a targeted fix that only affects PrivilegedProcessorTime, TotalProcessorTime, and UserProcessorTime on macOS. While the fix had been in main since September, a race condition bug in the original fix was found during the review of this backport. That bug was fixed in main on March 25 and cherry-picked to this backport.

The automatic backport of this fix failed due to a merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs. That conflict arose because the preceding line to the change here was removed in 20a3350. Otherwise, the rest of the merge was clean.

@jeffhandleyjeffhandley added Servicing-consider Issue for next servicing release review area-System.Diagnostics.Process os-mac-os-x macOS aka OSX labels Mar 22, 2024
@jeffhandleyjeffhandley added this to the 8.0.x milestone Mar 22, 2024
@jeffhandleyjeffhandley self-assigned this Mar 22, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@adamsitnikadamsitnik 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, thank you for backporting the fix @jeffhandley !

@jeffhandleyjeffhandley removed the Servicing-consider Issue for next servicing release review label Mar 26, 2024
Without this fix, it was possible for another thread to see an incorrect
(zero) value of s_timeBase_numer because of a race condition.
@jozkeejozkee added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2024
@jozkee

Copy link
Copy Markdown
Member

@jeffhandley should this be marked as servicing consider now that #100122 (comment) got resolved? If so, do we need to email tactics?

@carlossanlop

Copy link
Copy Markdown
Contributor

@jozkee I didn't find a Tactics email, so assuming this is ready (pending @jeffhandley 's or @adamsitnik 's confirmation), then yes, please send an email to Tactics and add the servicing-consider label.

@jeffhandleyjeffhandley added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@jeffhandley
jeffhandley requested a review from jkotasApril 30, 2024 18:51
@jeffhandleyjeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@jeffhandley

Copy link
Copy Markdown
MemberAuthor

This was approved in email and needs an approval for merge. @jkotas -- you can ignore the re-review request as this can now be merged.

@jeffhandley
jeffhandley merged commit 8acc1b5 into dotnet:release/8.0-stagingApr 30, 2024
@jeffhandley
jeffhandley deleted the jeffhandley/backport-92185/8.0-staging branch April 30, 2024 18:55
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Processos-mac-os-xmacOS aka OSXServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS - #100122

Merged
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging
Apr 30, 2024
Merged

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS#100122
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Mar 22, 2024

Copy link
Copy Markdown
Member

Backport of #92185 to release/8.0-staging

Customer Impact

  • Customer reported
  • Found internally

This has been reported by 2 customers. The first report was in September 2023 by @ForNeVeR in #91958, and they also provided the fix and tests. The issue was reported again by @huntharo in February with #98121. In that issue, @huntharo requested that this fix be backported to 8.0. As @huntharo stated:

On Mac OS X, with Apple Silicon, the System.Diagnostics.Process class returns incorrect (substantially underreported - ms when it should be seconds) CPU usage times for the process (and probably for the thread-level stats too).

I discovered this while writing a function to tune the max # of worker threads in the ThreadPool as it reduces CPU usage from 700% to 120% while increasing throughput for my project (pwrdrvr/lambda-dispatch#109). I was always computing that 0 threads would be needed because CPU usage was only going up by 20 ms per 5 seconds despite using 700% CPU.

Regression

  • Yes
  • No

While this isn't a product regression, it's behavior that became incorrect with Apple Silicon.

Testing

@ForNeVeR included a new test with their fix that is part of this backport. The test performs a native call and compares the API's results against the native results. As stated in the original PR:

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

Risk

Low. This is a targeted fix that only affects PrivilegedProcessorTime, TotalProcessorTime, and UserProcessorTime on macOS. While the fix had been in main since September, a race condition bug in the original fix was found during the review of this backport. That bug was fixed in main on March 25 and cherry-picked to this backport.

The automatic backport of this fix failed due to a merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs. That conflict arose because the preceding line to the change here was removed in 20a3350. Otherwise, the rest of the merge was clean.

@jeffhandleyjeffhandley added Servicing-consider Issue for next servicing release review area-System.Diagnostics.Process os-mac-os-x macOS aka OSX labels Mar 22, 2024
@jeffhandleyjeffhandley added this to the 8.0.x milestone Mar 22, 2024
@jeffhandleyjeffhandley self-assigned this Mar 22, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@adamsitnikadamsitnik 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, thank you for backporting the fix @jeffhandley !

@jeffhandleyjeffhandley removed the Servicing-consider Issue for next servicing release review label Mar 26, 2024
Without this fix, it was possible for another thread to see an incorrect
(zero) value of s_timeBase_numer because of a race condition.
@jozkeejozkee added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2024
@jozkee

Copy link
Copy Markdown
Member

@jeffhandley should this be marked as servicing consider now that #100122 (comment) got resolved? If so, do we need to email tactics?

@carlossanlop

Copy link
Copy Markdown
Contributor

@jozkee I didn't find a Tactics email, so assuming this is ready (pending @jeffhandley 's or @adamsitnik 's confirmation), then yes, please send an email to Tactics and add the servicing-consider label.

@jeffhandleyjeffhandley added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@jeffhandley
jeffhandley requested a review from jkotasApril 30, 2024 18:51
@jeffhandleyjeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@jeffhandley

Copy link
Copy Markdown
MemberAuthor

This was approved in email and needs an approval for merge. @jkotas -- you can ignore the re-review request as this can now be merged.

@jeffhandley
jeffhandley merged commit 8acc1b5 into dotnet:release/8.0-stagingApr 30, 2024
@jeffhandley
jeffhandley deleted the jeffhandley/backport-92185/8.0-staging branch April 30, 2024 18:55
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Processos-mac-os-xmacOS aka OSXServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS - #100122

Merged
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging
Apr 30, 2024
Merged

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS#100122
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Mar 22, 2024

Copy link
Copy Markdown
Member

Backport of #92185 to release/8.0-staging

Customer Impact

  • Customer reported
  • Found internally

This has been reported by 2 customers. The first report was in September 2023 by @ForNeVeR in #91958, and they also provided the fix and tests. The issue was reported again by @huntharo in February with #98121. In that issue, @huntharo requested that this fix be backported to 8.0. As @huntharo stated:

On Mac OS X, with Apple Silicon, the System.Diagnostics.Process class returns incorrect (substantially underreported - ms when it should be seconds) CPU usage times for the process (and probably for the thread-level stats too).

I discovered this while writing a function to tune the max # of worker threads in the ThreadPool as it reduces CPU usage from 700% to 120% while increasing throughput for my project (pwrdrvr/lambda-dispatch#109). I was always computing that 0 threads would be needed because CPU usage was only going up by 20 ms per 5 seconds despite using 700% CPU.

Regression

  • Yes
  • No

While this isn't a product regression, it's behavior that became incorrect with Apple Silicon.

Testing

@ForNeVeR included a new test with their fix that is part of this backport. The test performs a native call and compares the API's results against the native results. As stated in the original PR:

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

Risk

Low. This is a targeted fix that only affects PrivilegedProcessorTime, TotalProcessorTime, and UserProcessorTime on macOS. While the fix had been in main since September, a race condition bug in the original fix was found during the review of this backport. That bug was fixed in main on March 25 and cherry-picked to this backport.

The automatic backport of this fix failed due to a merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs. That conflict arose because the preceding line to the change here was removed in 20a3350. Otherwise, the rest of the merge was clean.

@jeffhandleyjeffhandley added Servicing-consider Issue for next servicing release review area-System.Diagnostics.Process os-mac-os-x macOS aka OSX labels Mar 22, 2024
@jeffhandleyjeffhandley added this to the 8.0.x milestone Mar 22, 2024
@jeffhandleyjeffhandley self-assigned this Mar 22, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@adamsitnikadamsitnik 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, thank you for backporting the fix @jeffhandley !

@jeffhandleyjeffhandley removed the Servicing-consider Issue for next servicing release review label Mar 26, 2024
Without this fix, it was possible for another thread to see an incorrect
(zero) value of s_timeBase_numer because of a race condition.
@jozkeejozkee added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2024
@jozkee

Copy link
Copy Markdown
Member

@jeffhandley should this be marked as servicing consider now that #100122 (comment) got resolved? If so, do we need to email tactics?

@carlossanlop

Copy link
Copy Markdown
Contributor

@jozkee I didn't find a Tactics email, so assuming this is ready (pending @jeffhandley 's or @adamsitnik 's confirmation), then yes, please send an email to Tactics and add the servicing-consider label.

@jeffhandleyjeffhandley added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@jeffhandley
jeffhandley requested a review from jkotasApril 30, 2024 18:51
@jeffhandleyjeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@jeffhandley

Copy link
Copy Markdown
MemberAuthor

This was approved in email and needs an approval for merge. @jkotas -- you can ignore the re-review request as this can now be merged.

@jeffhandley
jeffhandley merged commit 8acc1b5 into dotnet:release/8.0-stagingApr 30, 2024
@jeffhandley
jeffhandley deleted the jeffhandley/backport-92185/8.0-staging branch April 30, 2024 18:55
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Processos-mac-os-xmacOS aka OSXServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS - #100122

Merged
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging
Apr 30, 2024
Merged

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS#100122
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Mar 22, 2024

Copy link
Copy Markdown
Member

Backport of #92185 to release/8.0-staging

Customer Impact

  • Customer reported
  • Found internally

This has been reported by 2 customers. The first report was in September 2023 by @ForNeVeR in #91958, and they also provided the fix and tests. The issue was reported again by @huntharo in February with #98121. In that issue, @huntharo requested that this fix be backported to 8.0. As @huntharo stated:

On Mac OS X, with Apple Silicon, the System.Diagnostics.Process class returns incorrect (substantially underreported - ms when it should be seconds) CPU usage times for the process (and probably for the thread-level stats too).

I discovered this while writing a function to tune the max # of worker threads in the ThreadPool as it reduces CPU usage from 700% to 120% while increasing throughput for my project (pwrdrvr/lambda-dispatch#109). I was always computing that 0 threads would be needed because CPU usage was only going up by 20 ms per 5 seconds despite using 700% CPU.

Regression

  • Yes
  • No

While this isn't a product regression, it's behavior that became incorrect with Apple Silicon.

Testing

@ForNeVeR included a new test with their fix that is part of this backport. The test performs a native call and compares the API's results against the native results. As stated in the original PR:

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

Risk

Low. This is a targeted fix that only affects PrivilegedProcessorTime, TotalProcessorTime, and UserProcessorTime on macOS. While the fix had been in main since September, a race condition bug in the original fix was found during the review of this backport. That bug was fixed in main on March 25 and cherry-picked to this backport.

The automatic backport of this fix failed due to a merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs. That conflict arose because the preceding line to the change here was removed in 20a3350. Otherwise, the rest of the merge was clean.

@jeffhandleyjeffhandley added Servicing-consider Issue for next servicing release review area-System.Diagnostics.Process os-mac-os-x macOS aka OSX labels Mar 22, 2024
@jeffhandleyjeffhandley added this to the 8.0.x milestone Mar 22, 2024
@jeffhandleyjeffhandley self-assigned this Mar 22, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@adamsitnikadamsitnik 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, thank you for backporting the fix @jeffhandley !

@jeffhandleyjeffhandley removed the Servicing-consider Issue for next servicing release review label Mar 26, 2024
Without this fix, it was possible for another thread to see an incorrect
(zero) value of s_timeBase_numer because of a race condition.
@jozkeejozkee added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2024
@jozkee

Copy link
Copy Markdown
Member

@jeffhandley should this be marked as servicing consider now that #100122 (comment) got resolved? If so, do we need to email tactics?

@carlossanlop

Copy link
Copy Markdown
Contributor

@jozkee I didn't find a Tactics email, so assuming this is ready (pending @jeffhandley 's or @adamsitnik 's confirmation), then yes, please send an email to Tactics and add the servicing-consider label.

@jeffhandleyjeffhandley added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@jeffhandley
jeffhandley requested a review from jkotasApril 30, 2024 18:51
@jeffhandleyjeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@jeffhandley

Copy link
Copy Markdown
MemberAuthor

This was approved in email and needs an approval for merge. @jkotas -- you can ignore the re-review request as this can now be merged.

@jeffhandley
jeffhandley merged commit 8acc1b5 into dotnet:release/8.0-stagingApr 30, 2024
@jeffhandley
jeffhandley deleted the jeffhandley/backport-92185/8.0-staging branch April 30, 2024 18:55
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Processos-mac-os-xmacOS aka OSXServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

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

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS - #100122

Merged
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging
Apr 30, 2024
Merged

[release/8.0-staging] Fix #91958: use mach_timebase_info to determine process time coefficient on macOS#100122
jeffhandley merged 8 commits into
dotnet:release/8.0-stagingfrom
jeffhandley:jeffhandley/backport-92185/8.0-staging

Conversation

@jeffhandley

@jeffhandleyjeffhandley commented Mar 22, 2024

Copy link
Copy Markdown
Member

Backport of #92185 to release/8.0-staging

Customer Impact

  • Customer reported
  • Found internally

This has been reported by 2 customers. The first report was in September 2023 by @ForNeVeR in #91958, and they also provided the fix and tests. The issue was reported again by @huntharo in February with #98121. In that issue, @huntharo requested that this fix be backported to 8.0. As @huntharo stated:

On Mac OS X, with Apple Silicon, the System.Diagnostics.Process class returns incorrect (substantially underreported - ms when it should be seconds) CPU usage times for the process (and probably for the thread-level stats too).

I discovered this while writing a function to tune the max # of worker threads in the ThreadPool as it reduces CPU usage from 700% to 120% while increasing throughput for my project (pwrdrvr/lambda-dispatch#109). I was always computing that 0 threads would be needed because CPU usage was only going up by 20 ms per 5 seconds despite using 700% CPU.

Regression

  • Yes
  • No

While this isn't a product regression, it's behavior that became incorrect with Apple Silicon.

Testing

@ForNeVeR included a new test with their fix that is part of this backport. The test performs a native call and compares the API's results against the native results. As stated in the original PR:

I have compared the results of the process timing functions to the results of ps on my M2 MacBook device, and they seem to work properly after the changes.

The new tests were properly failing before the change (since we were returning values 42 times lower than the native results), and are, of course, green after the changes.

Risk

Low. This is a targeted fix that only affects PrivilegedProcessorTime, TotalProcessorTime, and UserProcessorTime on macOS. While the fix had been in main since September, a race condition bug in the original fix was found during the review of this backport. That bug was fixed in main on March 25 and cherry-picked to this backport.

The automatic backport of this fix failed due to a merge conflict in src/libraries/Common/src/Interop/OSX/Interop.Libraries.cs. That conflict arose because the preceding line to the change here was removed in 20a3350. Otherwise, the rest of the merge was clean.

@jeffhandleyjeffhandley added Servicing-consider Issue for next servicing release review area-System.Diagnostics.Process os-mac-os-x macOS aka OSX labels Mar 22, 2024
@jeffhandleyjeffhandley added this to the 8.0.x milestone Mar 22, 2024
@jeffhandleyjeffhandley self-assigned this Mar 22, 2024
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@adamsitnikadamsitnik 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, thank you for backporting the fix @jeffhandley !

@jeffhandleyjeffhandley removed the Servicing-consider Issue for next servicing release review label Mar 26, 2024
Without this fix, it was possible for another thread to see an incorrect
(zero) value of s_timeBase_numer because of a race condition.
@jozkeejozkee added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Apr 29, 2024
@jozkee

Copy link
Copy Markdown
Member

@jeffhandley should this be marked as servicing consider now that #100122 (comment) got resolved? If so, do we need to email tactics?

@carlossanlop

Copy link
Copy Markdown
Contributor

@jozkee I didn't find a Tactics email, so assuming this is ready (pending @jeffhandley 's or @adamsitnik 's confirmation), then yes, please send an email to Tactics and add the servicing-consider label.

@jeffhandleyjeffhandley added the Servicing-consider Issue for next servicing release review label Apr 30, 2024
@jeffhandley
jeffhandley requested a review from jkotasApril 30, 2024 18:51
@jeffhandleyjeffhandley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 30, 2024
@jeffhandley

Copy link
Copy Markdown
MemberAuthor

This was approved in email and needs an approval for merge. @jkotas -- you can ignore the re-review request as this can now be merged.

@jeffhandley
jeffhandley merged commit 8acc1b5 into dotnet:release/8.0-stagingApr 30, 2024
@jeffhandley
jeffhandley deleted the jeffhandley/backport-92185/8.0-staging branch April 30, 2024 18:55
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 31, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Processos-mac-os-xmacOS aka OSXServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@jeffhandley@jozkee@carlossanlop@ForNeVeR@adamsitnik@jkotas