Skip to content

Total time in GC counter - #88699

Merged
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/total-counter
Jul 14, 2023
Merged

Total time in GC counter#88699
cshung merged 2 commits into
dotnet:mainfrom
cshung:public/total-counter

Conversation

@cshung

@cshungcshung commented Jul 11, 2023

Copy link
Copy Markdown
Contributor

Fixes#69324

This change exposes a new event counter for the total amount of time the process is paused by the GC.

@ghostghost assigned cshungJul 11, 2023

@tommcdontommcdon 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, thanks!

@davidfowl
davidfowl requested a review from noahfalkJuly 11, 2023 22:54
@reyang

Copy link
Copy Markdown
Contributor

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

I suggested an adjusted name, otherwise LGTM!

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

:shipit:

…acing/RuntimeEventSource.cs
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
_committedCounter ??= new PollingCounter("gc-committed", this, () => ((double)GC.GetGCMemoryInfo().TotalCommittedBytes / 1_000_000)) { DisplayName = "GC Committed Bytes", DisplayUnits = "MB" };
_exceptionCounter ??= new IncrementingPollingCounter("exception-count", this, () => Exception.GetExceptionCount()) { DisplayName = "Exception Count", DisplayRateTimeScale = new TimeSpan(0, 0, 1) };
_gcTimeCounter ??= new PollingCounter("time-in-gc", this, () => GC.GetLastGCPercentTimeInGC()) { DisplayName = "% Time in GC since last GC", DisplayUnits = "%" };
_totalGcTimeCounter ??= new IncrementingPollingCounter("total-time-in-gc", this, () => GC.GetTotalPauseDuration().TotalMilliseconds) { DisplayName = "Time spent in GC", DisplayUnits = "ms" };

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.

Is "Time spent in GC" the right description? I think of "pause duration" as being the time when nothing is making forward progress in the app other than GC work. Is it not that, and instead actually the total time spent doing any GC-related work, regardless of whether it's stop-the-world or not?

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.

@cshung could you please change this as we discussed in the CR? the point @stephentoub brought up is exactly what I mentioned as well. this should be pause time, instead of "time" which is a vague term that doesn't say if it's the CPU time or pause time.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

How about this?

_totalGcPauseTimeCounter??=newIncrementingPollingCounter("total-pause-time-in-gc",this,()=>GC.GetTotalPauseDuration().TotalMilliseconds){DisplayName="Time paused by GC",DisplayUnits="ms"};

@noahfalk

@ghostghost locked as resolved and limited conversation to collaborators Aug 17, 2023
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.

should expose the accumulative GC pause duration as a counter

7 participants

@cshung@reyang@davidfowl@stephentoub@noahfalk@Maoni0@tommcdon