Skip to content

Conversation

@cshung
Copy link
Contributor

@cshung cshung commented Jun 5, 2023

Backport of #68835 to release/6.0-staging

Customer Impact

One of our first-party customers reported that they need GC.GetTotalPauseDuration to help with their autoscaling. Here is what he said:

We need GC.GetTotalPauseDuration in order to accurately measure the impact of GC in our services and in order for us to have actionable signals to manage autoscaling policies. We have faced numerous customer-impacting Sev2 incidents where a .NET process was CPU-limited yet not consuming 100% CPU, which in several distinct cases we concluded was due to Workstation GC pauses. With GC.GetTotalPauseDuration, we will be able to accurately measure how close we are to hitting CPU limits, which enables us to configure appropriate autoscaling policies that would help us avoid customer impact. While the existing EventCounter “time-in-gc” helps to some extent, it is a noisy and unreliable source because it doesn’t measure the actual average time in GC, just the average time in GC for the most recent GC, which is skewed since not all GC’s are comparable (different gen’s, compacting Vs. not, etc.)

Testing

  • GC Perf Sim
    • Normal Server
    • Normal Workstation
    • Large Pages Server
    • Large Pages Workstation
    • Low Memory Container
    • High Memory Load
  • Microbenchmarks
    • V8 Test
    • Top 20 Microbenchmarks
  • ASPNet Benchmarks
    • JsonMin - Windows

Risk

Low, this is an additional API. Any existing code that does not call the new API should not be affected at all.

@ghost
Copy link

ghost commented Jun 5, 2023

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned cshung Jun 5, 2023
@cshung cshung marked this pull request as draft June 5, 2023 20:52
@ghost
Copy link

ghost commented Jun 5, 2023

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

Issue Details

null

Author: cshung
Assignees: cshung
Labels:

area-GC-coreclr, new-api-needs-documentation

Milestone: -

@carlossanlop
Copy link
Contributor

@cshung I see this is still a draft. Today is code complete for the July release. If you want this change to be included in that release, please get a tactics approval, a sign-off, confirm the CI failures are unrelated, and merge it before EOD today.

@cshung cshung marked this pull request as ready for review July 7, 2023 21:57
@carlossanlop
Copy link
Contributor

@cshung I see this is still a draft. Reminder - Tomorrow Monday 10th is Code Complete for the August Release. If you intend to get this fix included in that servicing release, please make sure to get a Tactics approval and merge the PR before 4pm, because that's when I close the branches to start merging staging into internal.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved. please get a code review. we will take for consideration in 6.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Jul 10, 2023
@cshung cshung force-pushed the public/backport/pr-68835-to-release/6.0-staging branch from 2037e28 to 6cca632 Compare July 10, 2023 21:06
Co-authored-by: Eric StJohn <ericstj@microsoft.com>
@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 10, 2023
@jeffschwMSFT jeffschwMSFT added this to the 7.0.10 milestone Jul 10, 2023
@jeffschwMSFT jeffschwMSFT added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 10, 2023
@jeffschwMSFT
Copy link
Member

no-merge can be removed once review feedback addressed

Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cshung cshung merged commit f059c7e into dotnet:release/6.0-staging Jul 11, 2023
@cshung cshung deleted the public/backport/pr-68835-to-release/6.0-staging branch July 11, 2023 02:26
@cshung cshung removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) new-api-needs-documentation labels Jul 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-GC-coreclr Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants