Skip to content

Conversation

@davmason
Copy link
Contributor

Summary

dotnet/runtime#34549 added new GC events to the runtime and this PR updates the docs to include them. I did not include private events that the prior versions are not inlcuded in the docs (BGCOverflow and GCGenerationRange)

Fixes #17995

@davmason davmason requested a review from a team as a code owner July 15, 2020 00:19
@davmason davmason self-assigned this Jul 15, 2020
@dotnet-bot dotnet-bot added this to the July 2020 milestone Jul 15, 2020
@davmason davmason requested review from a team, noahfalk and sywhang July 15, 2020 00:37
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

👍

Woot! Love to see docs : )

Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

It's great to see events getting updated. Just a couple of comments:

  • The current path will make it look like this is applicable to .NET Framework, which is a bit confusing/unfortunate. Can't think of a better path under core. Maybe add a remark on the events only existing in .NET 5+?
  • The date and description on the metadata header for the doc should be updated.

@noahfalk
Copy link
Member

The current path will make it look like this is applicable to .NET Framework

Excellent catch : ) @sywhang did you ever chat with the doc team about the idea of splitting up these events .Net Core vs. desktop or somehow adding versioning information similar to what we have for APIs? No worries if not but didn't want us to duplicate work if there was already a plan.

@sywhang
Copy link
Contributor

sywhang commented Jul 15, 2020

@noahfalk Yes I did. I believe the suggestion I received from the docs team was to add the documentation on new runtime events to https://docs.microsoft.com/en-us/dotnet/core/diagnostics/logging-tracing.

@gewarren
Copy link
Contributor

@noahfalk Yes I did. I believe the suggestion I received from the docs team was to add the documentation on new runtime events to https://docs.microsoft.com/en-us/dotnet/core/diagnostics/logging-tracing.

Another thing we could consider is added a tracing section under /dotnet/standard that could apply to both Framework and Core, and we could annotate each event with the version it applies to. Thoughts @BillWagner? I think we have a new Performance section planned for the .NET 5 revamp too, correct?

@noahfalk
Copy link
Member

Thanks!

Having a location that is applicable across multiple runtime implementations (Framework, Core, Mono) would be ideal as many of the events will be relevant for more than one runtime. We could then document which events are supported where similar to what we do for APIs. /dotnet/standard/... seems like it could fit that bill.

For all the content currently under dotnet/core, is there any plan to move or forward it after .NET 5 removes the term "core" from the branding? I'm trying to figure out if it would be odd to put content referencing .Net Framework support for many events under what is currently the https://docs.microsoft.com/en-us/dotnet/core/diagnostics/... area?

I think we have a new Performance section planned for the .NET 5 revamp too, correct?

FWIW instrumentation events often get associated with performance investigations, but they also can have a lot of utility for non-performance related diagnostic purposes.

@gewarren
Copy link
Contributor

For all the content currently under dotnet/core, is there any plan to move or forward it after .NET 5 removes the term "core" from the branding? I'm trying to figure out if it would be odd to put content referencing .Net Framework support for many events under what is currently the https://docs.microsoft.com/en-us/dotnet/core/diagnostics/... area?

Yes, we do plan to change the folder structure.

@noahfalk
Copy link
Member

Thanks @gewarren!

Yes, we do plan to change the folder structure.

Is it written down somewhere or could you describe what changes are coming? Other than it being different I don't know what to expect yet : )

@gewarren
Copy link
Contributor

@noahfalk Bill Wagner wrote up a detailed plan here. If you have any feedback, we'd love to hear it.

@noahfalk
Copy link
Member

Thanks, if I understand the plan correctly our current https://docs.microsoft.com/en-us/dotnet/core/diagnostics/... area would move under Fundamentals/Environment and Runtime/Tools and productivity/Diagnostics and Instrumentation/. That seems like a very reasonable place for the event definitions to end up.

In terms of lining that up, is our best bet to copy the docs rooted under https://github.com/dotnet/docs/blob/9c9f8b35f08fc56cfebd37cdeb02ccbabc9f1bd1/docs/framework/performance/etw-events.md over to https://docs.microsoft.com/en-us/dotnet/core/diagnostics/ and then start making modifications to indicate which events are supported on which runtime SKU/versions?

@gewarren
Copy link
Contributor

In terms of lining that up, is our best bet to copy the docs rooted under https://github.com/dotnet/docs/blob/9c9f8b35f08fc56cfebd37cdeb02ccbabc9f1bd1/docs/framework/performance/etw-events.md over to https://docs.microsoft.com/en-us/dotnet/core/diagnostics/ and then start making modifications to indicate which events are supported on which runtime SKU/versions?

I don't think it's necessary to copy those .NET Framework files over to /core/diagnostics, since they will end up being moved/renamed again soon, and then we'd need to add redirects. If it works for you, I'd just add the .NET Core events in the existing docs (like you've done in this PR), and we'll make sure to incorporate these in the new Fundamentals/Environment and Runtime/Tools and productivity/Diagnostics and Instrumentation/ section.

@noahfalk
Copy link
Member

If it works for you...

That sounds even easier, thanks! : )

@davmason
Copy link
Contributor Author

Sorry, got distracted on other things. It sounds like the end result of the conversation above is that we're going to check this in as is and then deal with core versus desktop later?

I addressed the one issue @gewarren identified, are there any other requests outstanding?

@noahfalk
Copy link
Member

It sounds like the end result of the conversation above is that we're going to check this in as is and then deal with core versus desktop later?

Yep, that was my understanding : )

|FinalizationPromotedSize|win:UInt64|The total size, in bytes, of the objects that are ready for finalization.|
|FinalizationPromotedCount|win:UInt64|The number of objects that are ready for finalization.|
|PinnedObjectCount|win:UInt32|The number of pinned (unmovable) objects.|
|SinkBlockCount|win:UInt32|The number of synchronization blocks in use.|

Choose a reason for hiding this comment

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

@davmason
Should rather be SyncBlockCount, but from looking at the code looks like it the name is cut in stone
https://github.com/dotnet/runtime/blob/921bca0ae0d07a7dfd83d5d70eaf49759ab8463b/src/coreclr/vm/gctoclreventsink.h#L29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document GCHeapStats_V2 Runtime Event

8 participants